Message-ID: <aF0-_0pHqLL39wse@quatroqueijos.cascardo.eti.br>
Date: Thu, 26 Jun 2025 09:37:19 -0300
From: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
To: Zijun Hu <zijun_hu@...oud.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Arnd Bergmann <arnd@...db.de>, linux-kernel@...r.kernel.org,
	Zijun Hu <zijun.hu@....qualcomm.com>
Subject: Re: [PATCH RFC] char: misc: Enforce simple minor space division

On Thu, Jun 26, 2025 at 07:29:56AM +0800, Zijun Hu wrote:
> On 2025/6/24 23:50, Greg Kroah-Hartman wrote:
> > On Fri, Jun 20, 2025 at 10:53:32PM +0800, Zijun Hu wrote:
> >> From: Zijun Hu <zijun.hu@....qualcomm.com>
> >>
> >> Enforce simple minor space division related to macro MISC_DYNAMIC_MINOR
> >> defined as 255 currently:
> >>
> >> <  255 : Fixed minor codes
> >> == 255 : Indicator to request dynamic minor code
> >>>  255 : Dynamic minor codes requested
> > 
> > Is this the rule today?  If so, does the now-added tests we have for
> > misc device properly test for this?
> > 
> 
> 1) yes. this simple division becomes possible with recent commits below:
> Commit: 31b636d2c416 ("char: misc: restrict the dynamic range to exclude
> reserved minors")
> Commit: c876be906ce7 ("char: misc: register chrdev region with all
> possible minors")
> 
> both available fixed and dynamic minors interleaves with narrow space
> [0, 255) before above commits.
> 
> it is easy to balance minor space division by adjusting macro
> @MISC_DYNAMIC_MINOR if required in future as well.
> 
> Also hope all fixed minors are registered with header linux/miscdevice.h
> 
> 2) no. below recent commit don't cover the simple division fully.
> Commit: 74d8361be344 ("char: misc: add test cases")
> 
> drivers/misc/misc_minor_kunit.c may need to be corrected to reflecting
> division today.
> 

Correct, those added tests do not enforce that one cannot register a static
minor above MISC_DYNAMIC_MINOR.

However, to some extent [1], it tests that a MISC_DYNAMIC_MINOR allocation
will not return a number in the range of the static numbers. See
miscdev_test_dynamic_only_range.

It also tests that if a dynamic minor is allocated and one tries to
allocate that same number statically, it will fail. It also tries [2] to
test the other way around. That is, if one minor was statically allocate in
the dynamic range, that a dynamic allocation will not return that same
number.

Those tests, named miscdev_test_conflict and miscdev_test_conflict_reverse
were written considering the current implementation, which allows for
static allocations in the "dynamic range".

If we are going to change that, you need to change the tests too.

I would suggest applying only the last hunk of your patched
drivers/char/misc.c with a separate commit. Then, when misc_deregister
would be called the minor would be restored. However, since statically
allocating a minor above 255 would still be allowed, it could "restore" it
wrongly.

As Greg has observed, if there is no known case in the kernel where minor
is not set to MISC_DYNAMIC_MINOR by the driver before it calls
misc_register a second time, then there is nothing to fix here. If there
is such a case, then the driver must be fixed. It has always been the case,
even when the ranges were different, that the minor needed to be reset to
MISC_DYNAMIC_MINOR before calling misc_register after misc_deregister has
been called.

As you point out, when misc_register fails, it already restores the minor
number.

Let me know if you want to proceed with this change and need help with the
test case. I may be slow to respond since I am going on vacation.

Regards.
Cascardo.


[1] To some extent, because due to the large dynamic range, it only tries
allocating 256 dynamic minors. And only verifies numbers below 16 are not
returned, because that was the bug that existed before.

[2] Tries, because due to the large dynamic range, instead of allocating
all numbers, it just assumes that the allocator would give the first
number, but it also picks that "first" number by doing a dynamic allocation
and freeing it.

> >> This enforcing division also solves misc_register() reentry issue below:
> >>
> >> // Suppose both static @dev_A and @dev_B want to request dynamic minors.
> >> @dev_A.minor(255) @dev_B.minor(255)
> >>
> >> // Register @dev_A then de-register it.
> >> @dev_A.minor(255) -> registered -> @dev_A.minor(500) -> de-registered
> >> -> @dev_A.minor(500)
> >>
> >> // Register @dev_B
> >> @dev_B.minor(255) -> registered -> @dev_B.minor(500)
> >>
> >> // Register @dev_A again
> >> @dev_A.minor(500) -> encounter -EBUSY error since @dev_B has got 500.
> > 
> > Does this ever really happen?
> > 
> 
> i never meet this issue. but in theory, it may happen as explained below:
> 
> misc_register()/misc_deregister() are sometimes called within driver's
> probe()/remove(), such cases have reentry requirements
> 
> actually, error handling in misc_register() also reset minor code allocated:
> 
> 	if (IS_ERR(misc->this_device)) {
> 		misc_minor_free(misc->minor);
> 		if (is_dynamic) {
> 			misc->minor = MISC_DYNAMIC_MINOR;
> 		}
> 		err = PTR_ERR(misc->this_device);
> 		goto out;
> 	}
> 
> > And with the recent changes in the last dev cycle in this code area, is
> > it still an issue?
> >
> 
> this is a different issue with the ones recent changes fix.
> 
> >> Side effects:
> >> It will be refused to register device whose fixed minor > 255.
> > 
> > Do we have any in-kernel users that are > 255?
> 
> NO. no kernel users have such usage.
> 
> Actually, if fixed minor (>255) is used to register miscdev, it may
> encounter failure since the fixed minor (>255) may be allocated for
> other dynamic requests.
> 
> > 
> > thanks,
> > 
> > greg k-h
> 

Powered by blists - more mailing lists