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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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