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 <[email protected]>
---
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
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 <[email protected]>
/alessandro
On Fri, 23 Oct 2009 21:28:17 -0200
Thadeu Lima de Souza Cascardo <[email protected]> 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.
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
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 <[email protected]> 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.
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 <[email protected]> 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
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?
On Mon, 9 Nov 2009 21:29:25 -0200
Thadeu Lima de Souza Cascardo <[email protected]> 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.
> 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
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 <[email protected]> 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.
> 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)
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.