From: Zijun Hu <zijun.hu@oss.qualcomm.com>

misc_deregister() frees dynamic minor @misc->minor but does not
reset it to macro MISC_DYNAMIC_MINOR, so 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

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

On Thu, Jul 10, 2025 at 07:56:48PM +0800, Zijun Hu wrote:
> From: Zijun Hu <zijun.hu@oss.qualcomm.com>
> 
> misc_deregister() frees dynamic minor @misc->minor but does not
> reset it to macro MISC_DYNAMIC_MINOR, so 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
> 
> Fix by resetting @misc->minor to MISC_DYNAMIC_MINOR in misc_deregister()
> as error handling of misc_register() does.
> 

Adding a failing test and then fixing the code does not seem the best way
to justify this change. I would rather add the fix with a proper
justification and then add the test.

On the other hand, I have found real cases where this might happen, some by
code inspection only, but I also managed to reproduce the issue here,
where:

1) wmi/dell-smbios registered minor 122, acpi_thermal_rel registered minor
123.
2) unbind "int3400 thermal" driver from its device, this will unregister
acpi_thermal_rel
3) remove dell_smbios module
4) reinstall dell_smbios module, now wmi/dell-smbios is using misc 123
5) bind the device to "int3400 thermal" driver again, acpi_thermal_rel
fails to register

I think we have a few options to fix these bugs:

1) Apply your suggested fix.
2) Fix all the buggy drivers.
3) Change API and have the minor be a misc_register parameter.

The advantage of your option is that it is simple and contained and easy to
backport.

Changing API would require changing a lot of code and hard to backport, but
I find it less error-prone than requiring the minor member to be reset, if
we end up deciding about fixing the drivers.

As for fixing individual drivers, one helpful feature is applying your
previous patch [1], but perhaps with stronger message, maybe a WARN_ON.

[1] char: misc: Disallow registering miscdevice whose minor > MISC_DYNAMIC_MINOR

I am leaning towards your suggested fix, but with different wording, and
before adding the test case.

Something like:

Some drivers may reuse the miscdevice structure after they are
deregistered. If the intention is to allocate a dynamic minor, if the minor
number is not reset to MISC_DYNAMIC_MINOR before calling misc_register, it
will try to register a previously dynamically allocated minor number, which
may have been registered by a different driver.

One such case is the acpi_thermal_rel misc device, registered by the
int3400 thermal driver. If the device is unbound from the driver and later
bound, if there was another dynamic misc device registered in between, it
would fail to register the acpi_thermal_rel misc device. Other drivers
behave similarly.

Instead of fixing all the drivers, just reset the minor member to
MISC_DYNAMIC_MINOR when calling misc_deregister in case it was a
dynamically allocated minor number.

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

On 2025/7/11 02:15, Thadeu Lima de Souza Cascardo wrote:
> Adding a failing test and then fixing the code does not seem the best way
> to justify this change. I would rather add the fix with a proper
> justification and then add the test.
>
may need to only correct commit message. the order about unit test and
fix may be right as last reply.

> On the other hand, I have found real cases where this might happen, some by
> code inspection only, but I also managed to reproduce the issue here,
> where:
> 
> 1) wmi/dell-smbios registered minor 122, acpi_thermal_rel registered minor
> 123.
> 2) unbind "int3400 thermal" driver from its device, this will unregister
> acpi_thermal_rel
> 3) remove dell_smbios module
> 4) reinstall dell_smbios module, now wmi/dell-smbios is using misc 123
> 5) bind the device to "int3400 thermal" driver again, acpi_thermal_rel
> fails to register
> 

above issue should not happen with current char-misc tree since fixed
minor have no such reentry issue:

for any fixed minor fixed_A in range [0, 255): ".minor = fixed_A" ->
registered -> ".minor = fixed_A" -> de-registered -> ".minor = fixed_A"
, namely, for fixed minor, it is always un-changed about registering
and de-registering.


> I think we have a few options to fix these bugs:
> 
> 1) Apply your suggested fix.
> 2) Fix all the buggy drivers.
> 3) Change API and have the minor be a misc_register parameter.
> 
> The advantage of your option is that it is simple and contained and easy to
> backport.
> 
> Changing API would require changing a lot of code and hard to backport, but
> I find it less error-prone than requiring the minor member to be reset, if
> we end up deciding about fixing the drivers.
> 
> As for fixing individual drivers, one helpful feature is applying your
> previous patch [1], but perhaps with stronger message, maybe a WARN_ON.
> 
> [1] char: misc: Disallow registering miscdevice whose minor > MISC_DYNAMIC_MINOR
> 
> I am leaning towards your suggested fix, but with different wording, and
> before adding the test case.
> 
> Something like:
> 
> Some drivers may reuse the miscdevice structure after they are
> deregistered. If the intention is to allocate a dynamic minor, if the minor
> number is not reset to MISC_DYNAMIC_MINOR before calling misc_register, it
> will try to register a previously dynamically allocated minor number, which
> may have been registered by a different driver.
> 

let me correct commit message based on this suggestions.
thank you.

> One such case is the acpi_thermal_rel misc device, registered by the
> int3400 thermal driver. If the device is unbound from the driver and later
> bound, if there was another dynamic misc device registered in between, it
> would fail to register the acpi_thermal_rel misc device. Other drivers
> behave similarly.
> 
> Instead of fixing all the drivers, just reset the minor member to
> MISC_DYNAMIC_MINOR when calling misc_deregister in case it was a
> dynamically allocated minor number.

On Mon, Jul 14, 2025 at 09:02:09AM +0800, Zijun Hu wrote:
> On 2025/7/11 02:15, Thadeu Lima de Souza Cascardo wrote:
> > Adding a failing test and then fixing the code does not seem the best way
> > to justify this change. I would rather add the fix with a proper
> > justification and then add the test.
> >
> may need to only correct commit message. the order about unit test and
> fix may be right as last reply.
> 
> > On the other hand, I have found real cases where this might happen, some by
> > code inspection only, but I also managed to reproduce the issue here,
> > where:
> > 
> > 1) wmi/dell-smbios registered minor 122, acpi_thermal_rel registered minor
> > 123.
> > 2) unbind "int3400 thermal" driver from its device, this will unregister
> > acpi_thermal_rel
> > 3) remove dell_smbios module
> > 4) reinstall dell_smbios module, now wmi/dell-smbios is using misc 123
> > 5) bind the device to "int3400 thermal" driver again, acpi_thermal_rel
> > fails to register
> > 
> 
> above issue should not happen with current char-misc tree since fixed
> minor have no such reentry issue:
> 
> for any fixed minor fixed_A in range [0, 255): ".minor = fixed_A" ->
> registered -> ".minor = fixed_A" -> de-registered -> ".minor = fixed_A"
> , namely, for fixed minor, it is always un-changed about registering
> and de-registering.
> 

I am running an older tree, where the misc range is still below 128, but
notice those numbers are in the dynamic range. Those two drivers are using
the dynamic misc. I am just showing that what you are trying to fix here is
a real issue. And, below, I suggested a paragraph in the commit message
mentioning it.

Cascardo.

> 
> > I think we have a few options to fix these bugs:
> > 
> > 1) Apply your suggested fix.
> > 2) Fix all the buggy drivers.
> > 3) Change API and have the minor be a misc_register parameter.
> > 
> > The advantage of your option is that it is simple and contained and easy to
> > backport.
> > 
> > Changing API would require changing a lot of code and hard to backport, but
> > I find it less error-prone than requiring the minor member to be reset, if
> > we end up deciding about fixing the drivers.
> > 
> > As for fixing individual drivers, one helpful feature is applying your
> > previous patch [1], but perhaps with stronger message, maybe a WARN_ON.
> > 
> > [1] char: misc: Disallow registering miscdevice whose minor > MISC_DYNAMIC_MINOR
> > 
> > I am leaning towards your suggested fix, but with different wording, and
> > before adding the test case.
> > 
> > Something like:
> > 
> > Some drivers may reuse the miscdevice structure after they are
> > deregistered. If the intention is to allocate a dynamic minor, if the minor
> > number is not reset to MISC_DYNAMIC_MINOR before calling misc_register, it
> > will try to register a previously dynamically allocated minor number, which
> > may have been registered by a different driver.
> > 
> 
> let me correct commit message based on this suggestions.
> thank you.
> 
> > One such case is the acpi_thermal_rel misc device, registered by the
> > int3400 thermal driver. If the device is unbound from the driver and later
> > bound, if there was another dynamic misc device registered in between, it
> > would fail to register the acpi_thermal_rel misc device. Other drivers
> > behave similarly.
> > 
> > Instead of fixing all the drivers, just reset the minor member to
> > MISC_DYNAMIC_MINOR when calling misc_deregister in case it was a
> > dynamically allocated minor number.
>