[PATCH] misc: use a proper range for minor number dynamic allocation

[Thadeu Lima de Souza Cascardo]

The current dynamic allocation of minor number for misc devices has some
drawbacks.

First of all, the range for dynamic numbers include some statically
allocated numbers. It goes from 63 to 0, and we have numbers in the
range from 1 to 15 already allocated. Although, it gives priority to the
higher and not allocated numbers, we may end up in a situation where we
must reject registering a driver which got a static number because a
driver got its number with dynamic allocation. Considering fs/dlm/user.c
allocates as many misc devices as lockspaces are created, and that we
have more than 50 users around, it's not unreasonable to reach that
situation.

The proposed solution uses the not yet reserved range from 64 to 127. If
more devices are needed, we may push 64 to 16. Moreover, if we don't
need to give priority to the higher numbers anymore, we can start using
the bitmap/bitops functions.

Finally, if there's a failure creating the device (because there's
already one with the same name, for example), the current implementation
does not clear the bit for the allocated minor and that number is lost
for future allocations.

Signed-off-by: Thadeu Lima de Souza Cascardo <casc...@holoscopio.com>
---
drivers/char/misc.c | 25 ++++++++++++-------------
1 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index afc8e26..6bc4f44 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -60,8 +60,9 @@ static DEFINE_MUTEX(misc_mtx);
/*
* Assigned numbers, used for dynamic minors
*/
+#define DYNAMIC_MINOR_BASE 64
#define DYNAMIC_MINORS 64 /* like dynamic majors */
-static unsigned char misc_minors[DYNAMIC_MINORS / 8];
+static DECLARE_BITMAP(misc_minors, DYNAMIC_MINORS);

#ifdef CONFIG_PROC_FS
static void *misc_seq_start(struct seq_file *seq, loff_t *pos)
@@ -199,24 +200,23 @@ int misc_register(struct miscdevice * misc)
}

if (misc->minor == MISC_DYNAMIC_MINOR) {
- int i = DYNAMIC_MINORS;
- while (--i >= 0)
- if ( (misc_minors[i>>3] & (1 << (i&7))) == 0)
- break;
- if (i<0) {
+ int i = find_first_zero_bit(misc_minors, DYNAMIC_MINORS);
+ if (i >= DYNAMIC_MINORS) {
mutex_unlock(&misc_mtx);
return -EBUSY;
}
- misc->minor = i;
+ misc->minor = DYNAMIC_MINOR_BASE + i;
+ set_bit(i, misc_minors);
}

- if (misc->minor < DYNAMIC_MINORS)
- misc_minors[misc->minor >> 3] |= 1 << (misc->minor & 7);
dev = MKDEV(MISC_MAJOR, misc->minor);

misc->this_device = device_create(misc_class, misc->parent, dev,
misc, "%s", misc->name);
if (IS_ERR(misc->this_device)) {
+ int i = misc->minor - DYNAMIC_MINOR_BASE;
+ if (i >= 0 && i < DYNAMIC_MINORS)
+ clear_bit(i, misc_minors);
err = PTR_ERR(misc->this_device);
goto out;
}
@@ -243,7 +243,7 @@ int misc_register(struct miscdevice * misc)

int misc_deregister(struct miscdevice *misc)
{
- int i = misc->minor;
+ int i = misc->minor - DYNAMIC_MINOR_BASE;

if (list_empty(&misc->list))
return -EINVAL;
@@ -251,9 +251,8 @@ int misc_deregister(struct miscdevice *misc)
mutex_lock(&misc_mtx);
list_del(&misc->list);
device_destroy(misc_class, MKDEV(MISC_MAJOR, misc->minor));
- if (i < DYNAMIC_MINORS && i>0) {
- misc_minors[i>>3] &= ~(1 << (misc->minor & 7));
- }
+ if (i >= 0 && i < DYNAMIC_MINORS)
+ clear_bit(i, misc_minors);
mutex_unlock(&misc_mtx);
return 0;
}
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Alessandro Rubini]

Hello.
Please forgive my delay.

> First of all, the range for dynamic numbers include some statically
> allocated numbers. It goes from 63 to 0, and we have numbers in the
> range from 1 to 15 already allocated.

Yes, I wrote it ages ago. The "same as major numbers" testifies it.

I think your changes are sane, howver "git am" complains that
"patch doesn't apply". However, plain "patch -p1" liked it, so
I could test the result.

I think you need to rebase on current upstream, but apart from
that

Acked-By: Alessandro Rubini <rub...@vision.unipv.it>

/alessandro

[Andrew Morton]

On Fri, 23 Oct 2009 21:28:17 -0200
Thadeu Lima de Souza Cascardo <casc...@holoscopio.com> wrote:

> The current dynamic allocation of minor number for misc devices has some
> drawbacks.
>
> First of all, the range for dynamic numbers include some statically
> allocated numbers. It goes from 63 to 0, and we have numbers in the
> range from 1 to 15 already allocated. Although, it gives priority to the
> higher and not allocated numbers, we may end up in a situation where we
> must reject registering a driver which got a static number because a
> driver got its number with dynamic allocation. Considering fs/dlm/user.c
> allocates as many misc devices as lockspaces are created, and that we
> have more than 50 users around, it's not unreasonable to reach that
> situation.

What is this DLM behaviour of which you speak? It sounds broken.

> The proposed solution uses the not yet reserved range from 64 to 127. If
> more devices are needed, we may push 64 to 16. Moreover, if we don't
> need to give priority to the higher numbers anymore, we can start using
> the bitmap/bitops functions.

So... misc minors 64 to 127 are presently unused?

> Finally, if there's a failure creating the device (because there's
> already one with the same name, for example), the current implementation
> does not clear the bit for the allocated minor and that number is lost
> for future allocations.
>

Is that a bugfix for the existing code?

If so, please split that out into a separate patch which we can review
and apply promptly while we consider the broader problem which you've
identified.

[H. Peter Anvin]

On 11/09/2009 01:28 PM, Andrew Morton wrote:
>
>> The proposed solution uses the not yet reserved range from 64 to 127. If
>> more devices are needed, we may push 64 to 16. Moreover, if we don't
>> need to give priority to the higher numbers anymore, we can start using
>> the bitmap/bitops functions.
>
> So... misc minors 64 to 127 are presently unused?
>

Perhaps we should just start at 256 and go upward, or even 1048575 and
go downward.

-hpa

[David Teigland]

On Mon, Nov 09, 2009 at 01:28:36PM -0800, Andrew Morton wrote:
> On Fri, 23 Oct 2009 21:28:17 -0200
> Thadeu Lima de Souza Cascardo <casc...@holoscopio.com> wrote:
>
> > The current dynamic allocation of minor number for misc devices has some
> > drawbacks.
> >
> > First of all, the range for dynamic numbers include some statically
> > allocated numbers. It goes from 63 to 0, and we have numbers in the
> > range from 1 to 15 already allocated. Although, it gives priority to the
> > higher and not allocated numbers, we may end up in a situation where we
> > must reject registering a driver which got a static number because a
> > driver got its number with dynamic allocation. Considering fs/dlm/user.c
> > allocates as many misc devices as lockspaces are created, and that we
> > have more than 50 users around, it's not unreasonable to reach that
> > situation.
>
> What is this DLM behaviour of which you speak? It sounds broken.

One for each userland lockspace, I know of three userland apps using dlm:
1. rgmanager which is at the end of its life
2. clvmd which is switching to a different lock manager
3. ocfs2 tools, where the userland portion is transient; it only exists
while the tool executes.

That said, it shouldn't be a problem to switch to a single device in the
next version of the interface.

Dave

[Thadeu Lima de Souza Cascardo]

On Mon, Nov 09, 2009 at 01:28:36PM -0800, Andrew Morton wrote:

> On Fri, 23 Oct 2009 21:28:17 -0200
> Thadeu Lima de Souza Cascardo <casc...@holoscopio.com> wrote:
>
> > The current dynamic allocation of minor number for misc devices has some
> > drawbacks.
> >
> > First of all, the range for dynamic numbers include some statically
> > allocated numbers. It goes from 63 to 0, and we have numbers in the
> > range from 1 to 15 already allocated. Although, it gives priority to the
> > higher and not allocated numbers, we may end up in a situation where we
> > must reject registering a driver which got a static number because a
> > driver got its number with dynamic allocation. Considering fs/dlm/user.c
> > allocates as many misc devices as lockspaces are created, and that we
> > have more than 50 users around, it's not unreasonable to reach that
> > situation.
>
> What is this DLM behaviour of which you speak? It sounds broken.
>

I took a quick look at it, so I may be wrong. The code path is the
following: userspace writes command DLM_USER_CREATE_LOCKSPACE, which
calls device_create_lockspace, which calls dlm_device_register, which
does the following:

ls->ls_device.fops = &device_fops;
ls->ls_device.minor = MISC_DYNAMIC_MINOR;

error = misc_register(&ls->ls_device);
if (error) {
kfree(ls->ls_device.name);
}
fail:
return error;

So, it will probably return EBUSY right now with about 64 devices at the
most. My patch does not fix this, but avoids conflicts with drivers with
statically allocated minors which come in late in the init process.

> > The proposed solution uses the not yet reserved range from 64 to 127. If
> > more devices are needed, we may push 64 to 16. Moreover, if we don't
> > need to give priority to the higher numbers anymore, we can start using
> > the bitmap/bitops functions.
>
> So... misc minors 64 to 127 are presently unused?
>

As documented at Documentation/devices.txt, yes.

10 char Non-serial mice, misc features
[...]
15 = /dev/touchscreen/mk712 MK712 touchscreen
128 = /dev/beep Fancy beep device

> > Finally, if there's a failure creating the device (because there's
> > already one with the same name, for example), the current implementation
> > does not clear the bit for the allocated minor and that number is lost
> > for future allocations.
> >
>
> Is that a bugfix for the existing code?
>
> If so, please split that out into a separate patch which we can review
> and apply promptly while we consider the broader problem which you've
> identified.

We could consider buggy the caller which asks for the same device name
more than once, without unregistering the first device. But better safe
than sorry: we should protect the correct drivers from the buggy ones
and avoid a depletion of the minor numbers. And, in case the driver core
returns another error for another reason (from device_create), we do the
right thing.

I will send it right now.

Regards,
Cascardo.

[Thadeu Lima de Souza Cascardo]

On Mon, Nov 09, 2009 at 08:02:57PM -0200, Thadeu Lima de Souza Cascardo wrote:
> >
> > Is that a bugfix for the existing code?
> >
> > If so, please split that out into a separate patch which we can review
> > and apply promptly while we consider the broader problem which you've
> > identified.
>
> We could consider buggy the caller which asks for the same device name
> more than once, without unregistering the first device. But better safe
> than sorry: we should protect the correct drivers from the buggy ones
> and avoid a depletion of the minor numbers. And, in case the driver core
> returns another error for another reason (from device_create), we do the
> right thing.
>
> I will send it right now.
>
> Regards,
> Cascardo.

I've just tried to create a single and separate patch, but that would
let lots of related bugs around.

First of all, the current code does not use the bitmap idiom. Should I
use it on my fix and let all the other bitmap manipulations as is, or
should I use the current and less readable style?

Second, this single fix would match the same test currently in
misc_deregister, which is broken, since it does not test for the 0
minor.

Thus, I am sending a patch which fixes those two issues using the
current style, a fix for the style itself, and a change from the current
range to something that could have its range easily fixed. However,
regarding that last change, it will still use bitmaps, which may not be
appropriate for large ranges.

Perhaps, using a idr instead of the list and bitmap couple, may be
sensible. What do you think about it?

[Andrew Morton]

On Mon, 9 Nov 2009 21:29:25 -0200
Thadeu Lima de Souza Cascardo <casc...@holoscopio.com> wrote:

> On Mon, Nov 09, 2009 at 08:02:57PM -0200, Thadeu Lima de Souza Cascardo wrote:
> > >
> > > Is that a bugfix for the existing code?
> > >
> > > If so, please split that out into a separate patch which we can review
> > > and apply promptly while we consider the broader problem which you've
> > > identified.
> >
> > We could consider buggy the caller which asks for the same device name
> > more than once, without unregistering the first device. But better safe
> > than sorry: we should protect the correct drivers from the buggy ones
> > and avoid a depletion of the minor numbers. And, in case the driver core
> > returns another error for another reason (from device_create), we do the
> > right thing.
> >
> > I will send it right now.
> >
> > Regards,
> > Cascardo.
>
> I've just tried to create a single and separate patch, but that would
> let lots of related bugs around.
>
> First of all, the current code does not use the bitmap idiom. Should I
> use it on my fix and let all the other bitmap manipulations as is, or
> should I use the current and less readable style?

If you think that the fix is needed in 2.6.32 and perhaps -stable then
yes please, something minimal is desired.

If you think that the bug is sufficiently minor that that the fix xcan
be deferred into 2.6.33 then no intermediate patch is needed. Bear in
mind that others might disagree with your designation of the priority,
and they might want a fix which they can backport into their
2.6.29/2.6.30/etc kernels.

> Second, this single fix would match the same test currently in
> misc_deregister, which is broken, since it does not test for the 0
> minor.

I don't know what that means.

> Thus, I am sending a patch which fixes those two issues using the
> current style, a fix for the style itself, and a change from the current
> range to something that could have its range easily fixed.

Sounds good.

> However,
> regarding that last change, it will still use bitmaps, which may not be
> appropriate for large ranges.
>
> Perhaps, using a idr instead of the list and bitmap couple, may be
> sensible. What do you think about it?

miscdev registration/deregistration is hardly a high-frequency
operation. I'd opt for simplicity and compactness over speed here.

[H. Peter Anvin]

On 11/09/2009 04:30 PM, Thadeu Lima de Souza Cascardo wrote:
> The current dynamic allocation of minor number for misc devices has some
> drawbacks.
>
> First of all, the range for dynamic numbers include some statically
> allocated numbers. It goes from 63 to 0, and we have numbers in the
> range from 1 to 15 already allocated. Although, it gives priority to the
> higher and not allocated numbers, we may end up in a situation where we
> must reject registering a driver which got a static number because a
> driver got its number with dynamic allocation. Considering fs/dlm/user.c
> allocates as many misc devices as lockspaces are created, and that we
> have more than 50 users around, it's not unreasonable to reach that
> situation.
>

> The proposed solution uses the not yet reserved range from 64 to 127. If
> more devices are needed, we may push 64 to 16.
>

Again, why not push these up above 256?

-hpa

[Thadeu Lima de Souza Cascardo]

Use DECLARE_BITMAP, find_first_zero_bit, set_bit and clear_bit instead
of rewriting code to do it with the minor number dynamic allocation
bitmap.

We need to invert the bit position to keep the code behaviour of using
the last minor numbers first, since we don't have a find_last_zero_bit.

Signed-off-by: Thadeu Lima de Souza Cascardo <casc...@holoscopio.com>
---
drivers/char/misc.c | 22 +++++++++-------------
1 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 95d1282..59fff30 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -61,7 +61,7 @@ static DEFINE_MUTEX(misc_mtx);

* Assigned numbers, used for dynamic minors
*/

#define DYNAMIC_MINORS 64 /* like dynamic majors */
-static unsigned char misc_minors[DYNAMIC_MINORS / 8];
+static DECLARE_BITMAP(misc_minors, DYNAMIC_MINORS);

extern int pmu_device_init(void);

@@ -201,27 +201,23 @@ int misc_register(struct miscdevice * misc)

}

if (misc->minor == MISC_DYNAMIC_MINOR) {
- int i = DYNAMIC_MINORS;
- while (--i >= 0)
- if ( (misc_minors[i>>3] & (1 << (i&7))) == 0)
- break;
- if (i<0) {
+ int i = find_first_zero_bit(misc_minors, DYNAMIC_MINORS);
+ if (i >= DYNAMIC_MINORS) {
mutex_unlock(&misc_mtx);
return -EBUSY;
}
- misc->minor = i;

+ misc->minor = DYNAMIC_MINORS - i - 1;

+ set_bit(i, misc_minors);
}

- if (misc->minor < DYNAMIC_MINORS)
- misc_minors[misc->minor >> 3] |= 1 << (misc->minor & 7);
dev = MKDEV(MISC_MAJOR, misc->minor);

misc->this_device = device_create(misc_class, misc->parent, dev,
misc, "%s", misc->name);
if (IS_ERR(misc->this_device)) {

- int i = misc->minor;

+ int i = DYNAMIC_MINORS - misc->minor - 1;
if (i < DYNAMIC_MINORS && i >= 0)
- misc_minors[i>>3] &= ~(1 << (i & 7));

+ clear_bit(i, misc_minors);
err = PTR_ERR(misc->this_device);
goto out;
}

@@ -248,7 +244,7 @@ int misc_register(struct miscdevice * misc)

int misc_deregister(struct miscdevice *misc)
{
- int i = misc->minor;

+ int i = DYNAMIC_MINORS - misc->minor - 1;

if (list_empty(&misc->list))
return -EINVAL;

@@ -257,7 +253,7 @@ int misc_deregister(struct miscdevice *misc)

list_del(&misc->list);
device_destroy(misc_class, MKDEV(MISC_MAJOR, misc->minor));

if (i < DYNAMIC_MINORS && i >= 0)
- misc_minors[i>>3] &= ~(1 << (i & 7));

+ clear_bit(i, misc_minors);
mutex_unlock(&misc_mtx);
return 0;
}
--
1.6.3.3

--

[Thadeu Lima de Souza Cascardo]

The current dynamic allocation of minor number for misc devices has some
drawbacks.

First of all, the range for dynamic numbers include some statically
allocated numbers. It goes from 63 to 0, and we have numbers in the
range from 1 to 15 already allocated. Although, it gives priority to the
higher and not allocated numbers, we may end up in a situation where we
must reject registering a driver which got a static number because a
driver got its number with dynamic allocation. Considering fs/dlm/user.c
allocates as many misc devices as lockspaces are created, and that we
have more than 50 users around, it's not unreasonable to reach that
situation.

The proposed solution uses the not yet reserved range from 64 to 127. If
more devices are needed, we may push 64 to 16.

Signed-off-by: Thadeu Lima de Souza Cascardo <casc...@holoscopio.com>
---
drivers/char/misc.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 59fff30..5681d3c 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -60,6 +60,7 @@ static DEFINE_MUTEX(misc_mtx);
/*

* Assigned numbers, used for dynamic minors
*/

+#define DYNAMIC_MINOR_BASE 64

#define DYNAMIC_MINORS 64 /* like dynamic majors */

static DECLARE_BITMAP(misc_minors, DYNAMIC_MINORS);

@@ -206,7 +207,7 @@ int misc_register(struct miscdevice * misc)
mutex_unlock(&misc_mtx);
return -EBUSY;
}
- misc->minor = DYNAMIC_MINORS - i - 1;

+ misc->minor = DYNAMIC_MINOR_BASE + i;

set_bit(i, misc_minors);
}

@@ -215,7 +216,7 @@ int misc_register(struct miscdevice * misc)

misc->this_device = device_create(misc_class, misc->parent, dev,
misc, "%s", misc->name);
if (IS_ERR(misc->this_device)) {

- int i = DYNAMIC_MINORS - misc->minor - 1;
+ int i = misc->minor - DYNAMIC_MINOR_BASE;

if (i < DYNAMIC_MINORS && i >= 0)

clear_bit(i, misc_minors);
err = PTR_ERR(misc->this_device);

@@ -244,7 +245,7 @@ int misc_register(struct miscdevice * misc)

int misc_deregister(struct miscdevice *misc)
{
- int i = DYNAMIC_MINORS - misc->minor - 1;
+ int i = misc->minor - DYNAMIC_MINOR_BASE;

if (list_empty(&misc->list))
return -EINVAL;

[Alan Cox]

> > The proposed solution uses the not yet reserved range from 64 to 127. If
> > more devices are needed, we may push 64 to 16.
> >
>
> Again, why not push these up above 256?

misc was always intended for one off devices not something creating 40 or
50 misc entries. DLM should be moved out of misc IMHO, or 256+ used for
misc "ranges" such as DLM.

Pushing the existing stuff above minor 256 risks breaking some old setups

[Alan Cox]

> We could consider buggy the caller which asks for the same device name
> more than once, without unregistering the first device. But better safe

If they ask for the same name we certainly should. Probably we should
error that request and use WARN_ON() to shame the offender in
kerneloops.org.

> than sorry: we should protect the correct drivers from the buggy ones
> and avoid a depletion of the minor numbers. And, in case the driver core
> returns another error for another reason (from device_create), we do the
> right thing.

Agreed we need to protect the working drivers.

Alan

[Steven Whitehouse]

Hi,

On Mon, 2009-11-09 at 17:03 -0600, David Teigland wrote:
> On Mon, Nov 09, 2009 at 01:28:36PM -0800, Andrew Morton wrote:
> > On Fri, 23 Oct 2009 21:28:17 -0200
> > Thadeu Lima de Souza Cascardo <casc...@holoscopio.com> wrote:
> >
> > > The current dynamic allocation of minor number for misc devices has some
> > > drawbacks.
> > >
> > > First of all, the range for dynamic numbers include some statically
> > > allocated numbers. It goes from 63 to 0, and we have numbers in the
> > > range from 1 to 15 already allocated. Although, it gives priority to the
> > > higher and not allocated numbers, we may end up in a situation where we
> > > must reject registering a driver which got a static number because a
> > > driver got its number with dynamic allocation. Considering fs/dlm/user.c
> > > allocates as many misc devices as lockspaces are created, and that we
> > > have more than 50 users around, it's not unreasonable to reach that
> > > situation.
> >
> > What is this DLM behaviour of which you speak? It sounds broken.
>
> One for each userland lockspace, I know of three userland apps using dlm:
> 1. rgmanager which is at the end of its life
> 2. clvmd which is switching to a different lock manager
> 3. ocfs2 tools, where the userland portion is transient; it only exists
> while the tool executes.
>
> That said, it shouldn't be a problem to switch to a single device in the
> next version of the interface.
>
> Dave
>

As well as the per-userland lockspace misc devices there are also the
misc devices of which there are only one instance shared between all
lock spaces:

dlm_lock - Used for userland communication with posix locks
dlm-monitor - Used to only to check that dlm_controld is running (so far
as I can tell)
dlm-control - Used to create/remove userland dlm lockspaces

I also had a look at other methods used by the dlm to communicate with
userspace, and this is what I've come up with so far:

configfs - Used to set up lockspaces
debugfs - Used to get lock state information for debugging
netlink - Used only to notify lock timeouts to dlm_controld
sysfs - Used to implement a wait for a userland event (wait for write to
a sysfs file)
uevents - Used to trigger dlm_controld into performing an action which
results in the write to sysfs mentioned above. This is
netlink again, but with a layer over the top of it.

If a change to the misc devices is planned, I'm wondering if it would be
possible to merge some of the other functions into a single interface to
simplify things a bit. In particular the netlink interface looks dubious
to me since I think it should be doing a broadcast rather than the
rather strange (and possibly a security issue with any process able to
send messages to it and set their own pid so far as I can see). I have
to say that I didn't test that, but there is no obvious check for privs
that I can see in the dlm netlink code.

Steve.

[Alan Cox]

> If a change to the misc devices is planned, I'm wondering if it would be
> possible to merge some of the other functions into a single interface to

Three is not a problem but the original report is for a lot more - so
something funny is going on and that wants debugging first before
anything gets changed or name/number spaces allocated.

So LANANA firstly wants to know *why* the report is occuring and if the
whole issue is arising due to a bug in something not to a real need.

Alan
(with a LANANA hat on)

[Thadeu Lima de Souza Cascardo]

On Mon, Nov 09, 2009 at 04:34:07PM -0800, H. Peter Anvin wrote:
> On 11/09/2009 04:30 PM, Thadeu Lima de Souza Cascardo wrote:
> > The current dynamic allocation of minor number for misc devices has some
> > drawbacks.
> >
> > First of all, the range for dynamic numbers include some statically
> > allocated numbers. It goes from 63 to 0, and we have numbers in the
> > range from 1 to 15 already allocated. Although, it gives priority to the
> > higher and not allocated numbers, we may end up in a situation where we
> > must reject registering a driver which got a static number because a
> > driver got its number with dynamic allocation. Considering fs/dlm/user.c
> > allocates as many misc devices as lockspaces are created, and that we
> > have more than 50 users around, it's not unreasonable to reach that
> > situation.
> >
> > The proposed solution uses the not yet reserved range from 64 to 127. If
> > more devices are needed, we may push 64 to 16.
> >
>
> Again, why not push these up above 256?
>
> -hpa

I don't have a problem with this. However, as Alan has pointed out,
there may be old setups that rely on minor numbers below 256. I have
thought of that, but I'm sure there's argument against it. I've decided
to submit it more closely to what is done today, that is, 64 devices. As
I said, there's still space for 48 more devices, and, besides DLM, I
couldn't find at first glance any other user that requests more than one
single device.

Anyway, the only thing that worries me when pushing it above 256 is
that, right now, the bitmap is statically allocated, and I think a
single page would be pretty more than we have and we need right now.
Otherwise, I would suggest using a dynamic allocation system, which
would be less simple than what we have right now, and Andrew has pointed
out.

Best Regards,
Cascardo.

[Thadeu Lima de Souza Cascardo]

On Tue, Nov 10, 2009 at 11:09:42AM +0000, Alan Cox wrote:
> > We could consider buggy the caller which asks for the same device name
> > more than once, without unregistering the first device. But better safe
>
> If they ask for the same name we certainly should. Probably we should
> error that request and use WARN_ON() to shame the offender in
> kerneloops.org.
>

The current code returns an error. It does not clear the bit in the
allocation bitmap, which is a bug in misc, which my first patch in the
series fixes now.

If it uses the same name, device_create is the responsible for failing.
It already logs that, but it uses no WARN right now. I think this WARN
should be in the driver core, not in misc, so we catch other offenders
as well.

> > than sorry: we should protect the correct drivers from the buggy ones
> > and avoid a depletion of the minor numbers. And, in case the driver core
> > returns another error for another reason (from device_create), we do the
> > right thing.
>
> Agreed we need to protect the working drivers.
>
> Alan

So, do you think this should be in 2.6.32 or even go down to stable?

Regards,
Cascardo.

[Andrew Morton]

On Mon, 09 Nov 2009 16:34:07 -0800
"H. Peter Anvin" <h...@zytor.com> wrote:

> On 11/09/2009 04:30 PM, Thadeu Lima de Souza Cascardo wrote:
> > The current dynamic allocation of minor number for misc devices has some
> > drawbacks.
> >
> > First of all, the range for dynamic numbers include some statically
> > allocated numbers. It goes from 63 to 0, and we have numbers in the
> > range from 1 to 15 already allocated. Although, it gives priority to the
> > higher and not allocated numbers, we may end up in a situation where we
> > must reject registering a driver which got a static number because a
> > driver got its number with dynamic allocation. Considering fs/dlm/user.c
> > allocates as many misc devices as lockspaces are created, and that we
> > have more than 50 users around, it's not unreasonable to reach that
> > situation.
> >
> > The proposed solution uses the not yet reserved range from 64 to 127. If
> > more devices are needed, we may push 64 to 16.
> >
>
> Again, why not push these up above 256?
>

I merged this patch, but made a note-to-self that there are remaining
open issues..

[Andrew Morton]

On Wed, 11 Nov 2009 15:36:32 -0800
Andrew Morton <ak...@linux-foundation.org> wrote:

> On Mon, 09 Nov 2009 16:34:07 -0800
> "H. Peter Anvin" <h...@zytor.com> wrote:
>
> > On 11/09/2009 04:30 PM, Thadeu Lima de Souza Cascardo wrote:
> > > The current dynamic allocation of minor number for misc devices has some
> > > drawbacks.
> > >
> > > First of all, the range for dynamic numbers include some statically
> > > allocated numbers. It goes from 63 to 0, and we have numbers in the
> > > range from 1 to 15 already allocated. Although, it gives priority to the
> > > higher and not allocated numbers, we may end up in a situation where we
> > > must reject registering a driver which got a static number because a
> > > driver got its number with dynamic allocation. Considering fs/dlm/user.c
> > > allocates as many misc devices as lockspaces are created, and that we
> > > have more than 50 users around, it's not unreasonable to reach that
> > > situation.
> > >
> > > The proposed solution uses the not yet reserved range from 64 to 127. If
> > > more devices are needed, we may push 64 to 16.
> > >
> >
> > Again, why not push these up above 256?
> >
>
> I merged this patch, but made a note-to-self that there are remaining
> open issues..

And nothing else happened. Can we revisit this please?

From: Thadeu Lima de Souza Cascardo <casc...@holoscopio.com>

The current dynamic allocation of minor number for misc devices has some
drawbacks.

First of all, the range for dynamic numbers include some statically
allocated numbers. It goes from 63 to 0, and we have numbers in the range
from 1 to 15 already allocated. Although, it gives priority to the higher
and not allocated numbers, we may end up in a situation where we must
reject registering a driver which got a static number because a driver got
its number with dynamic allocation. Considering fs/dlm/user.c allocates
as many misc devices as lockspaces are created, and that we have more than
50 users around, it's not unreasonable to reach that situation.

The proposed solution uses the not yet reserved range from 64 to 127. If
more devices are needed, we may push 64 to 16.

Signed-off-by: Thadeu Lima de Souza Cascardo <casc...@holoscopio.com>
Cc: "H. Peter Anvin" <h...@zytor.com>
Cc: Alan Cox <al...@lxorguk.ukuu.org.uk>
Signed-off-by: Andrew Morton <ak...@linux-foundation.org>
---

drivers/char/misc.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff -puN drivers/char/misc.c~drivers-char-miscc-use-a-proper-range-for-minor-number-dynamic-allocation drivers/char/misc.c
--- a/drivers/char/misc.c~drivers-char-miscc-use-a-proper-range-for-minor-number-dynamic-allocation
+++ a/drivers/char/misc.c
@@ -59,6 +59,7 @@ static DEFINE_MUTEX(misc_mtx);

/*
* Assigned numbers, used for dynamic minors
*/
+#define DYNAMIC_MINOR_BASE 64
#define DYNAMIC_MINORS 64 /* like dynamic majors */
static DECLARE_BITMAP(misc_minors, DYNAMIC_MINORS);

@@ -201,7 +202,7 @@ int misc_register(struct miscdevice * mi

mutex_unlock(&misc_mtx);
return -EBUSY;
}
- misc->minor = DYNAMIC_MINORS - i - 1;
+ misc->minor = DYNAMIC_MINOR_BASE + i;
set_bit(i, misc_minors);
}

@@ -210,7 +211,7 @@ int misc_register(struct miscdevice * mi

misc->this_device = device_create(misc_class, misc->parent, dev,
misc, "%s", misc->name);
if (IS_ERR(misc->this_device)) {
- int i = DYNAMIC_MINORS - misc->minor - 1;
+ int i = misc->minor - DYNAMIC_MINOR_BASE;
if (i < DYNAMIC_MINORS && i >= 0)
clear_bit(i, misc_minors);
err = PTR_ERR(misc->this_device);

@@ -239,7 +240,7 @@ int misc_register(struct miscdevice * mi

int misc_deregister(struct miscdevice *misc)
{
- int i = DYNAMIC_MINORS - misc->minor - 1;
+ int i = misc->minor - DYNAMIC_MINOR_BASE;

if (list_empty(&misc->list))
return -EINVAL;

_

[H. Peter Anvin]

On 12/15/2009 02:34 PM, Andrew Morton wrote:
>>>>
>>>> The proposed solution uses the not yet reserved range from 64 to 127. If
>>>> more devices are needed, we may push 64 to 16.
>>>>
>>>
>>> Again, why not push these up above 256?
>>>
>>
>> I merged this patch, but made a note-to-self that there are remaining
>> open issues..
>
> And nothing else happened. Can we revisit this please?
>

There seem to be people still worried about breaking userspace with
majors/minors >= 256. I'm starting to think it is time to actually
break userspace, and dynamic majors/minors seem as good as any place to
start, especially since they by definition has to be managed by
something like udev. We have had large dev_t for something like six
years now, and most pieces of software isn't affected at all -- only the
stuff that manages /dev.

-hpa

[Alan Cox]

> > I merged this patch, but made a note-to-self that there are remaining
> > open issues..
>
> And nothing else happened. Can we revisit this please?

The last I saw was a request for an explanation of the users
configuration and how he was getting so many misc devices. Its a bug in
dlm if the dlm is doing that but I didn't see any follow up to the DLM
folks on the subject from the reporter, so it stays NACKed and conflicts
with the device registry.

We need to know from the DLM folks/Thaddeu what is actually going on with
that system, especially as it seems to be producing multiple
registrations of the same misc device name which is completely broken and
should probably be made to error anyway.

Whatever is going on this is the wrong "fix". If DLM should only register
it once (as seems the intent) then DLM needs fixing or the users config
or both. If it can register many then DLM needs fixing to not eat misc
devices.

<eviltwin>Possibly we need to make it BUG_ON() adding the same name twice, then
perhaps we'd get more feedback</eviltwin>

Alan

[Andrew Morton]

On Tue, 15 Dec 2009 23:37:38 +0000
Alan Cox <al...@lxorguk.ukuu.org.uk> wrote:

> > > I merged this patch, but made a note-to-self that there are remaining
> > > open issues..
> >
> > And nothing else happened. Can we revisit this please?
>
> The last I saw was a request for an explanation of the users
> configuration and how he was getting so many misc devices. Its a bug in
> dlm if the dlm is doing that but I didn't see any follow up to the DLM
> folks on the subject from the reporter, so it stays NACKed and conflicts
> with the device registry.
>
> We need to know from the DLM folks/Thaddeu what is actually going on with
> that system, especially as it seems to be producing multiple
> registrations of the same misc device name which is completely broken and
> should probably be made to error anyway.
>
> Whatever is going on this is the wrong "fix". If DLM should only register
> it once (as seems the intent) then DLM needs fixing or the users config
> or both. If it can register many then DLM needs fixing to not eat misc
> devices.
>
> <eviltwin>Possibly we need to make it BUG_ON() adding the same name twice, then
> perhaps we'd get more feedback</eviltwin>
>

OK, thanks, I dropped it. We can always reappyly it (or a variant)
once this is all sorted out.

[Greg KH]

On Tue, Dec 15, 2009 at 02:42:30PM -0800, H. Peter Anvin wrote:
> On 12/15/2009 02:34 PM, Andrew Morton wrote:
> >>>>
> >>>> The proposed solution uses the not yet reserved range from 64 to 127. If
> >>>> more devices are needed, we may push 64 to 16.
> >>>>
> >>>
> >>> Again, why not push these up above 256?
> >>>
> >>
> >> I merged this patch, but made a note-to-self that there are remaining
> >> open issues..
> >
> > And nothing else happened. Can we revisit this please?
> >
>
> There seem to be people still worried about breaking userspace with
> majors/minors >= 256. I'm starting to think it is time to actually
> break userspace, and dynamic majors/minors seem as good as any place to
> start, especially since they by definition has to be managed by
> something like udev. We have had large dev_t for something like six
> years now, and most pieces of software isn't affected at all -- only the
> stuff that manages /dev.

No objection from me, as long as the patch actually works.

But you might want to provide a config option so that loons that still
are running Fedora 3 PPC machines who keep insisting on updating to the
latest kernel version still keep their machines limping along
successfully :)

thanks,

greg k-h

[Alan Cox]

What I have been doing in response to requests for misc device
allocations is to suggest people simply go and allocate some dynamic
device space. There are very very few cases a misc device needs a
specific minor and the large dynamic device space has really made misc a
"historical" entity which should be treated that way.

If you want 2048 minors for a random new device, the kernel will just
magic them for you, udev will magic the device nodes, and the tmpfs will
handle it nicely

(and if your udev isn't on its own private file system then I hope its an
old one, because the current one has a hole in it, which doesn't seem to
have been fixed yet)

Alan

[David Teigland]

On Tue, Dec 15, 2009 at 11:37:38PM +0000, Alan Cox wrote:
> > > I merged this patch, but made a note-to-self that there are remaining
> > > open issues..
> >
> > And nothing else happened. Can we revisit this please?
>
> The last I saw was a request for an explanation of the users
> configuration and how he was getting so many misc devices. Its a bug in
> dlm if the dlm is doing that but I didn't see any follow up to the DLM
> folks on the subject from the reporter, so it stays NACKed and conflicts
> with the device registry.

I never saw an indication that the dlm ate up all of someone's misc
devices. Can we ask the reporter what they were running?

> We need to know from the DLM folks/Thaddeu what is actually going on with
> that system, especially as it seems to be producing multiple
> registrations of the same misc device name which is completely broken and
> should probably be made to error anyway.

I don't believe the dlm will ever register multiple devices with the same
name.

> Whatever is going on this is the wrong "fix". If DLM should only register
> it once (as seems the intent) then DLM needs fixing or the users config
> or both. If it can register many then DLM needs fixing to not eat misc
> devices.

I explained here http://lkml.org/lkml/2009/11/9/379 that the dlm does not
use as many misc devices as has been implied. It starts with 3, and adds
one for each *userspace* lockspace. There are very few applications (I
know of 3) that create userspace lockspaces, and they each create about
one each.

That said, I still intend to rework the dlm to use a single device for all
lockspaces.

Dave

[H. Peter Anvin]

On 12/16/2009 10:01 AM, David Teigland wrote:
>
> I explained here http://lkml.org/lkml/2009/11/9/379 that the dlm does not
> use as many misc devices as has been implied. It starts with 3, and adds
> one for each *userspace* lockspace. There are very few applications (I
> know of 3) that create userspace lockspaces, and they each create about
> one each.
>
> That said, I still intend to rework the dlm to use a single device for all
> lockspaces.
>

Still seems to make more sense to simply move it to a major and/or a
dynamic allocation. misc devices were always meant to be *one of a
kind* devices, which simply didn't need anything but a single allocation.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

[casc...@holoscopio.com]

On Tue, Dec 15, 2009 at 02:42:30PM -0800, H. Peter Anvin wrote:

> On 12/15/2009 02:34 PM, Andrew Morton wrote:
> >>>>
> >>>> The proposed solution uses the not yet reserved range from 64 to 127. If
> >>>> more devices are needed, we may push 64 to 16.
> >>>>
> >>>
> >>> Again, why not push these up above 256?
> >>>
> >>
> >> I merged this patch, but made a note-to-self that there are remaining
> >> open issues..
> >
> > And nothing else happened. Can we revisit this please?
> >
>
> There seem to be people still worried about breaking userspace with
> majors/minors >= 256. I'm starting to think it is time to actually
> break userspace, and dynamic majors/minors seem as good as any place to
> start, especially since they by definition has to be managed by
> something like udev. We have had large dev_t for something like six
> years now, and most pieces of software isn't affected at all -- only the
> stuff that manages /dev.
>
> -hpa

I see no problem in this. Can we make this configurable like the random
minor for block devices was done? Or, perhaps, before making a decision,
we can check what kind of devices are using misc dynamic minor, which
was what I did before writing this patch to count the number of current
in-tree users.

I am currently downloading source code from git since I've lost my disk
recently. That's why I'm late replying. I may send a list of current
users if that's interesting for a better analysis.

Regards,
Cascardo.

[casc...@holoscopio.com]

On Tue, Dec 15, 2009 at 11:37:38PM +0000, Alan Cox wrote:

> > > I merged this patch, but made a note-to-self that there are remaining
> > > open issues..
> >
> > And nothing else happened. Can we revisit this please?
>
> The last I saw was a request for an explanation of the users
> configuration and how he was getting so many misc devices. Its a bug in
> dlm if the dlm is doing that but I didn't see any follow up to the DLM
> folks on the subject from the reporter, so it stays NACKed and conflicts
> with the device registry.
>

I don't recall an explanation fro my DLM configuration. Probably my
mistake, sorry.

However, I haven't hit any DLM problem because I am no DLM user. My
concern was that the current code has overlapping ranges for the dynamic
range of device numbers and the first static device numbers. They do not
conflict at first because the dynamic range grows downards, and I kept
that behaviour in the other patch.

When I realized this, I decided to check what code did use misc dynamic
minors. I've accounted about 40 users and one in special caught my
attention. That was DLM, because it could potentially create many
devices.

> We need to know from the DLM folks/Thaddeu what is actually going on with
> that system, especially as it seems to be producing multiple
> registrations of the same misc device name which is completely broken and
> should probably be made to error anyway.
>
> Whatever is going on this is the wrong "fix". If DLM should only register
> it once (as seems the intent) then DLM needs fixing or the users config
> or both. If it can register many then DLM needs fixing to not eat misc
> devices.
>

It will only create that many devices if we request it to do so. But
requesting it is simply a matter of doing an ioctl. I think going for a
dynamic major of its own would be a better solution for DLM in the long
term.

> <eviltwin>Possibly we need to make it BUG_ON() adding the same name twice, then
> perhaps we'd get more feedback</eviltwin>

That was an unrelated problem, that made me look at the misc code at
first. I was playing with registering and unregistering misc devices.
And, then, I've hit the bug: after registering two devices with the same
name and unregistering both of them afterwards, a minor number got
unavailable for any further registering. That was fixed in the first
patch in the series.

I think a BUG_ON at driver core is not that bad an idea at all. Let me
try it. This bug would be hit even with the misc fix applied, since misc
would still try to register the device, and, then (now with the patch)
release the allocated minor number, returning error to the registerer.

>
> Alan

Regards,
Cascardo.

[casc...@holoscopio.com]

On Tue, Dec 15, 2009 at 11:56:39PM +0000, Alan Cox wrote:
> What I have been doing in response to requests for misc device
> allocations is to suggest people simply go and allocate some dynamic
> device space. There are very very few cases a misc device needs a
> specific minor and the large dynamic device space has really made misc a
> "historical" entity which should be treated that way.
>
> If you want 2048 minors for a random new device, the kernel will just
> magic them for you, udev will magic the device nodes, and the tmpfs will
> handle it nicely
>
> (and if your udev isn't on its own private file system then I hope its an
> old one, because the current one has a hole in it, which doesn't seem to
> have been fixed yet)
>
> Alan

I have one "issue" to raise about using anything else: misc devices are
easy to register: they have a class of its own, you don't need to
allocate a range and, then, create the device with its respective file
operations and all that. For one-shot devices, they are really a good
thing.

Does it make sense to create an easier and simpler API for creating
character devices that does not allocate a major with minor 0 and 255
minors which also creates your own class?

I think many device driver writers go for proc filesystem these days
because you just have to call a function with your file name and file
operations and that's it. Most of these people are damn lazy people, I
would agree. But, perhaps, we should make it easier for these lazy
people to write better interfaces. Or perhaps this would spoil them and
they would not do their homework and use the right interface anyway.

I've seen code that should have been using the input subsystem and was
using proc files. And that's why I'm saying they could be spoiled with
such an easy interface. They would still use the wrong interface anyway,
with character devices instead of using the input subsystem.

Well, I'll have a go for such an interface and request comments. It
would not be much different from the misc system anyway, but would allow
more than one device, any (dynamic) major to be used, and a class name
for each registerer.

Regards,
Cascardo.

[casc...@holoscopio.com]

On Wed, Dec 16, 2009 at 12:01:56PM -0600, David Teigland wrote:
> On Tue, Dec 15, 2009 at 11:37:38PM +0000, Alan Cox wrote:
> > > > I merged this patch, but made a note-to-self that there are remaining
> > > > open issues..
> > >
> > > And nothing else happened. Can we revisit this please?
> >
> > The last I saw was a request for an explanation of the users
> > configuration and how he was getting so many misc devices. Its a bug in
> > dlm if the dlm is doing that but I didn't see any follow up to the DLM
> > folks on the subject from the reporter, so it stays NACKed and conflicts
> > with the device registry.
>
> I never saw an indication that the dlm ate up all of someone's misc
> devices. Can we ask the reporter what they were running?
>

That was not such ocurrence. I don't use DLM. Sorry for all the fuss.
I've simply noticed that DLM was using misc devices to create devices
dynamically after an ioctl. I consider that that ioctl can be called as
many times as I want my userspace program to call, am I right?

> > We need to know from the DLM folks/Thaddeu what is actually going on with
> > that system, especially as it seems to be producing multiple
> > registrations of the same misc device name which is completely broken and
> > should probably be made to error anyway.
>
> I don't believe the dlm will ever register multiple devices with the same
> name.
>

No. That was another issue, not related to DLM at all. Again, sorry for
any confusion.

> > Whatever is going on this is the wrong "fix". If DLM should only register
> > it once (as seems the intent) then DLM needs fixing or the users config
> > or both. If it can register many then DLM needs fixing to not eat misc
> > devices.
>
> I explained here http://lkml.org/lkml/2009/11/9/379 that the dlm does not
> use as many misc devices as has been implied. It starts with 3, and adds
> one for each *userspace* lockspace. There are very few applications (I
> know of 3) that create userspace lockspaces, and they each create about
> one each.
>

The raised issue was that of the device for *each* lock. An application
could request as many as it wanted. If there's no such usecase, I don't
think that's a problem as long as that call is privileged (which may be
easily achieved with the simple chown/chmod).

> That said, I still intend to rework the dlm to use a single device for all
> lockspaces.

That would break current userspace implementations. Or do you have any
idea on solving this without this breakage?

>
> Dave
>

Regards and sorry for anything,
Cascardo.