1. Patchew
  2. linux
  3. [PATCH v4 0/8] char: misc: Various cleanup for miscdevice
  4. View patch

[PATCH v4 5/8] char: misc: Make registering miscdevice reentry who wants dynamic minor

Zijun Hu posted 8 patches 7 months, 1 week ago
There is a newer version of this series
[PATCH v4 5/8] char: misc: Make registering miscdevice reentry who wants dynamic minor
Posted by Zijun Hu 7 months, 1 week ago 
From: Zijun Hu <zijun.hu@oss.qualcomm.com>

misc_deregister() frees dynamic minor @misc->minor but does not reset it
and cause kunit test case miscdev_test_dynamic_reentry() failure:

| Invalid fixed minor 257 for miscdevice 'miscdyn_a'
| #miscdev_test_dynamic_reentry: ASSERTION FAILED at misc_minor_kunit.c:639
| Expected ret == 0, but
| ret == -22 (0xffffffffffffffea)
| [FAILED] miscdev_test_dynamic_reentry

misc_register()/misc_deregister() are sometimes invoked by driver's
probe()/remove() separately, which has reentry requirement.

Fix by resetting @misc->minor to MISC_DYNAMIC_MINOR in misc_deregister()
as error handling of misc_register() does.

Signed-off-by: Zijun Hu <zijun.hu@oss.qualcomm.com>
---
 drivers/char/misc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index b8e66466184fa21fb66d968db7950e0b5669ac43..96ed343cf5c8509a09855020049a9af82a3ede95 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -288,6 +288,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);

-- 
2.34.1
Re: [PATCH v4 5/8] char: misc: Make registering miscdevice reentry who wants dynamic minor
Posted by Greg Kroah-Hartman 7 months ago 
The subject does not make much sense, can you please reword it?

On Fri, Jul 04, 2025 at 09:26:03PM +0800, Zijun Hu wrote:
> From: Zijun Hu <zijun.hu@oss.qualcomm.com>
> 
> misc_deregister() frees dynamic minor @misc->minor but does not reset it
> and cause kunit test case miscdev_test_dynamic_reentry() failure:
> 
> | Invalid fixed minor 257 for miscdevice 'miscdyn_a'
> | #miscdev_test_dynamic_reentry: ASSERTION FAILED at misc_minor_kunit.c:639
> | Expected ret == 0, but
> | ret == -22 (0xffffffffffffffea)
> | [FAILED] miscdev_test_dynamic_reentry
> 
> misc_register()/misc_deregister() are sometimes invoked by driver's
> probe()/remove() separately, which has reentry requirement.

What do you mean?  Why is it required that this is reentrant?  What
in-kernel drivers require this?

> Fix by resetting @misc->minor to MISC_DYNAMIC_MINOR in misc_deregister()
> as error handling of misc_register() does.
> 
> Signed-off-by: Zijun Hu <zijun.hu@oss.qualcomm.com>
> ---
>  drivers/char/misc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> index b8e66466184fa21fb66d968db7950e0b5669ac43..96ed343cf5c8509a09855020049a9af82a3ede95 100644
> --- a/drivers/char/misc.c
> +++ b/drivers/char/misc.c
> @@ -288,6 +288,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);

misc is being unregistered here, so why are you changing the minor
field?  It's now invalid as it is not registered, so this value should
never be relied on at all, neither is anything else in this structure.

thanks,

greg k-h
Re: [PATCH v4 5/8] char: misc: Make registering miscdevice reentry who wants dynamic minor
Posted by Zijun Hu 7 months ago 
On 2025/7/6 16:55, Greg Kroah-Hartman wrote:
>> | Invalid fixed minor 257 for miscdevice 'miscdyn_a'
>> | #miscdev_test_dynamic_reentry: ASSERTION FAILED at misc_minor_kunit.c:639
>> | Expected ret == 0, but
>> | ret == -22 (0xffffffffffffffea)
>> | [FAILED] miscdev_test_dynamic_reentry
>>
>> misc_register()/misc_deregister() are sometimes invoked by driver's
>> probe()/remove() separately, which has reentry requirement.
> What do you mean?  Why is it required that this is reentrant?  What
> in-kernel drivers require this?
> 

miscdevice APIs are sometimes used by the following way, "drv_probe()->
drv_remove()->drv_probe()" is possible, so "misc_register()->
misc_deregister()->misc_register()" is possible and considered by
previous patch's test case, which needs to register misc_dev again
without any reinitialization, namely, reentry.

actually, several in-kernel codes have such usages.

static struct miscdevice misc_dev = {
	...
		.minor = MISC_DYNAMIC_MINOR,
	...
};

int drv_probe() {
...
    // static misc_dev is not initialized before registering.
	misc_register(&misc_dev);
...
}

void drv_remove() {
	...
	misc_deregister(&misc_dev);
	...
}

struct device_driver driver = {
	.probe = drv_probe,
	.remove = drv_remove,
};

>> Fix by resetting @misc->minor to MISC_DYNAMIC_MINOR in misc_deregister()
>> as error handling of misc_register() does.
>>
>> Signed-off-by: Zijun Hu <zijun.hu@oss.qualcomm.com>
>> ---
>>  drivers/char/misc.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
>> index b8e66466184fa21fb66d968db7950e0b5669ac43..96ed343cf5c8509a09855020049a9af82a3ede95 100644
>> --- a/drivers/char/misc.c
>> +++ b/drivers/char/misc.c
>> @@ -288,6 +288,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);
> misc is being unregistered here, so why are you changing the minor
> field?  It's now invalid as it is not registered, so this value should
> never be relied on at all, neither is anything else in this structure.

reset its minor code is to register it again without re-initialization
successfully

the other members are handed by misc_deregister() properly and don't
effect re-registering.
Re: [PATCH v4 5/8] char: misc: Make registering miscdevice reentry who wants dynamic minor
Posted by Greg Kroah-Hartman 7 months ago 
On Wed, Jul 09, 2025 at 08:41:36PM +0800, Zijun Hu wrote:
> On 2025/7/6 16:55, Greg Kroah-Hartman wrote:
> >> | Invalid fixed minor 257 for miscdevice 'miscdyn_a'
> >> | #miscdev_test_dynamic_reentry: ASSERTION FAILED at misc_minor_kunit.c:639
> >> | Expected ret == 0, but
> >> | ret == -22 (0xffffffffffffffea)
> >> | [FAILED] miscdev_test_dynamic_reentry
> >>
> >> misc_register()/misc_deregister() are sometimes invoked by driver's
> >> probe()/remove() separately, which has reentry requirement.
> > What do you mean?  Why is it required that this is reentrant?  What
> > in-kernel drivers require this?
> > 
> 
> miscdevice APIs are sometimes used by the following way, "drv_probe()->
> drv_remove()->drv_probe()" is possible, so "misc_register()->
> misc_deregister()->misc_register()" is possible and considered by
> previous patch's test case, which needs to register misc_dev again
> without any reinitialization, namely, reentry.
> 
> actually, several in-kernel codes have such usages.
> 
> static struct miscdevice misc_dev = {
> 	...
> 		.minor = MISC_DYNAMIC_MINOR,
> 	...
> };
> 
> int drv_probe() {
> ...
>     // static misc_dev is not initialized before registering.
> 	misc_register(&misc_dev);
> ...
> }
> 
> void drv_remove() {
> 	...
> 	misc_deregister(&misc_dev);
> 	...
> }
> 
> struct device_driver driver = {
> 	.probe = drv_probe,
> 	.remove = drv_remove,
> };
> 
> >> Fix by resetting @misc->minor to MISC_DYNAMIC_MINOR in misc_deregister()
> >> as error handling of misc_register() does.
> >>
> >> Signed-off-by: Zijun Hu <zijun.hu@oss.qualcomm.com>
> >> ---
> >>  drivers/char/misc.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> >> index b8e66466184fa21fb66d968db7950e0b5669ac43..96ed343cf5c8509a09855020049a9af82a3ede95 100644
> >> --- a/drivers/char/misc.c
> >> +++ b/drivers/char/misc.c
> >> @@ -288,6 +288,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);
> > misc is being unregistered here, so why are you changing the minor
> > field?  It's now invalid as it is not registered, so this value should
> > never be relied on at all, neither is anything else in this structure.
> 
> reset its minor code is to register it again without re-initialization
> successfully
> 
> the other members are handed by misc_deregister() properly and don't
> effect re-registering.

Then we are getting lucky, let's clear out all the fields we modified
and later rely on as this feels very fragile.

thanks,

greg k-h

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.