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

[Thadeu Lima de Souza Cascardo <cascardo@igalia.com>]

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