[PATCH RFC] char: misc: Enforce simple minor space division

[PATCH RFC] char: misc: Enforce simple minor space division

[Zijun Hu]

From: Zijun Hu <zijun.hu@oss.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

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.

Side effects:
It will be refused to register device whose fixed minor > 255.

Signed-off-by: Zijun Hu <zijun.hu@oss.qualcomm.com>
---
 drivers/char/misc.c        | 16 +++++++++++++---
 include/linux/miscdevice.h |  8 ++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index e5ea36bbf6b3d1313eb35d3259617bf90c55727d..e6a5907bbf915b4dd64522ada3024ba163cb7aa3 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -132,7 +132,8 @@ static int misc_open(struct inode *inode, struct file *file)
 		break;
 	}
 
-	if (!new_fops) {
+	/* Only request module for fixed minor code */
+	if (!new_fops && minor < MISC_DYNAMIC_MINOR) {
 		mutex_unlock(&misc_mtx);
 		request_module("char-major-%d-%d", MISC_MAJOR, minor);
 		mutex_lock(&misc_mtx);
@@ -144,10 +145,11 @@ static int misc_open(struct inode *inode, struct file *file)
 			new_fops = fops_get(iter->fops);
 			break;
 		}
-		if (!new_fops)
-			goto fail;
 	}
 
+	if (!new_fops)
+		goto fail;
+
 	/*
 	 * Place the miscdevice in the file's
 	 * private_data so it can be used by the
@@ -210,6 +212,12 @@ int misc_register(struct miscdevice *misc)
 	int err = 0;
 	bool is_dynamic = (misc->minor == MISC_DYNAMIC_MINOR);
 
+	if (misc->minor > MISC_DYNAMIC_MINOR) {
+		pr_err("Invalid fixed minor code %d for misc char device '%s'\n",
+		       misc->minor, misc->name);
+		return -EINVAL;
+	}
+
 	INIT_LIST_HEAD(&misc->list);
 
 	mutex_lock(&misc_mtx);
@@ -282,6 +290,8 @@ void misc_deregister(struct miscdevice *misc)
 	list_del(&misc->list);
 	device_destroy(&misc_class, MKDEV(MISC_MAJOR, misc->minor));
 	misc_minor_free(misc->minor);
+	if (misc->minor > MISC_DYNAMIC_MINOR)
+		misc->minor = MISC_DYNAMIC_MINOR;
 	mutex_unlock(&misc_mtx);
 }
 EXPORT_SYMBOL(misc_deregister);
diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
index 3e6deb00fc8535a7571f85489c74979e18385bad..9e19bce981f065ea612da0a330c529e9c044c996 100644
--- a/include/linux/miscdevice.h
+++ b/include/linux/miscdevice.h
@@ -71,6 +71,14 @@
 #define USERIO_MINOR		240
 #define VHOST_VSOCK_MINOR	241
 #define RFKILL_MINOR		242
+
+/*
+ * Misc char device minor code space division related to below macro:
+ *
+ * <  255  : Fixed minor codes
+ * == 255  : Indicator to request dynamic minor code
+ * >  255  : Dynamic minor codes requested
+ */
 #define MISC_DYNAMIC_MINOR	255
 
 struct miscdevice {

---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250620-rfc_miscdev-788bf18bba5d
prerequisite-change-id: 20250620-fix_mischar-794de4259592:v1
prerequisite-patch-id: 775e6edc4107a26a1fa30fd36315e4862dbc0cde
prerequisite-patch-id: e32024c7470a34eb019af4bf18c08f67c3589d7e
prerequisite-patch-id: 70045b4ad94130f051c314339a58217baed9190a

Best regards,
-- 
Zijun Hu <zijun.hu@oss.qualcomm.com>

Re: [PATCH RFC] char: misc: Enforce simple minor space division

[Greg Kroah-Hartman]

On Fri, Jun 20, 2025 at 10:53:32PM +0800, Zijun Hu wrote:
> From: Zijun Hu <zijun.hu@oss.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?

> 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?

And with the recent changes in the last dev cycle in this code area, is
it still an issue?

> Side effects:
> It will be refused to register device whose fixed minor > 255.

Do we have any in-kernel users that are > 255?

thanks,

greg k-h

Re: [PATCH RFC] char: misc: Enforce simple minor space division

[Zijun Hu]

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@oss.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.

>> 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

Re: [PATCH RFC] char: misc: Enforce simple minor space division

[Thadeu Lima de Souza Cascardo]

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@oss.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
> 

Re: [PATCH RFC] char: misc: Enforce simple minor space division

[Zijun Hu]

On 2025/6/26 20:37, Thadeu Lima de Souza Cascardo wrote:
>> 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.
> 

agree.

> I would suggest applying only the last hunk of your patched
> drivers/char/misc.c with a separate commit. Then, when misc_deregister

agree.

> 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.
> 

now. will disallow statically allocating a minor above 255 since below
disadvantages:

1) if the minor was requested by other dynamic request. requesting a
fixed minor which > 255 will be failed.

2) both fixed minors and dynamic minors interleave int the same space.


> 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.
> 

this patch is to solve such reentry issue.


> 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.
> 

I will split this RFC patch and modify test cases.

welcome to get your comments at any time. thank you (^^).

> 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.