linux-kernel - Re: [PATCH v5 5/8] char: misc: Fix kunit test case miscdev_test_dynamic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aHTpaoI8IfkSKm3O@quatroqueijos.cascardo.eti.br>
Date: Mon, 14 Jul 2025 08:26:34 -0300
From: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
To: Zijun Hu <zijun_hu@...oud.com>
Cc: Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
	Helge Deller <deller@....de>,
	"David S. Miller" <davem@...emloft.net>,
	Andreas Larsson <andreas@...sler.com>, linux-kernel@...r.kernel.org,
	linux-parisc@...r.kernel.org, sparclinux@...r.kernel.org,
	Zijun Hu <zijun.hu@....qualcomm.com>
Subject: Re: [PATCH v5 5/8] char: misc: Fix kunit test case
 miscdev_test_dynamic_reentry() failure

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