Re: [PATCH v5 5/8] char: misc: Fix kunit test case miscdev_test_dynamic_reentry() failure

[Zijun Hu <zijun_hu@icloud.com>]

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.