Re: [PATCH v1] driver core: Take parent lock in async device attach

[PETER Mario]

Hi Alan,

On 10/6/26 22:45, Alan Stern wrote: 
> 
> On Tue, Oct 06, 2026 at 03:29:48PM +0000, PETER Mario wrote:
>> Hi Greg,
>>
>> On 10/6/26 16:38, Greg Kroah-Hartman wrote:
>>
>>> On Tue, Oct 06, 2026 at 01:23:35PM +0000, Mario Peter wrote:
>>>> On buses with need_parent_lock set (only USB), probe() must run with the
>>>> parent device locked. A synchronous attach after device_add() gets this
>>>> from the caller, e.g. usb_set_configuration() holds the udev lock while
>>>> adding interfaces. __device_attach_async_helper() runs the probe from an
>>>> async worker instead, where that lock isn't held, and only takes
>>>> device_lock(dev).
>>>>
>>>> With async probing enabled for USB drivers (driver_async_probe=*,
>>>> module.async_probe=1), hub_probe() of a multi-TT hub then races with
>>>> usb_set_configuration() and both create the interface's endpoint
>>>> devices. On an i.MX8MM board this hit 11 of 100 boots:
>>>>
>>>>   sysfs: cannot create duplicate filename '.../1-1/1-1:1.0/ep_81'
>>>>   Call trace:
>>>>    sysfs_warn_dup
>>>>    usb_create_ep_devs
>>>>    create_intf_ep_devs
>>>>    usb_set_interface
>>>>    hub_probe
>>>>    usb_probe_interface
>>>>    really_probe
>>>>    __device_attach_async_helper
>>>>    async_run_entry_fn
>>>>
>>>> Take the parent lock there as well, like __driver_attach_async_helper()
>>>> does, and move __device_driver_lock/unlock() up for that. With this the
>>>> warning was gone in 300 boots.
>>>>
>>>> Fixes: 765230b5f084 ("driver-core: add asynchronous probing support for drivers")
>>>> Assisted-by: LLM
>>>> Signed-off-by: Mario Peter <mario.peter@xxxxxxxxxxxxxxxxxxxx>
>>>> ---
>>>> Seen and tested on 6.16.y, which has the same code here. On mainline
>>>> only build-tested.
>>>
>>> Please verify this on the latest tree, 6.16.y is _VERY_ old and
>>> obsolete, lots has changed in the year it was released.
>>
>> Moving this board to the latest kernel for a test isn't that easy, as
>> its board support isn't upstream.
> 
> It should be fairly simple to run the verification on a standard PC
> using an up-to-date kernel.  Nothing in the bug description or fix is
> specific to i.MX8MM.

Right. I reproduced it on v7.3-rc6 in QEMU, booted with
driver_async_probe=hub, with dummy_hcd and a small raw-gadget program
that emulates a multi-TT hub. A loop writes 0 and 1 to the hub's
"authorized" attribute. Each time, usb_set_configuration() runs in the
writing task with the udev lock held, and hub_probe() runs in the
async worker, the same two paths as in the trace from the board.

The window is small: usb_set_configuration() has to stall after it
has created ep_81 but before it sets ep_devs_created, for longer than
hub_probe() needs to get to usb_set_interface(). To make that happen
often enough, a SCHED_FIFO task preempts the writing task at random
points for 3 ms. With that, the warning hit 29 of 6000 iterations
without the patch, with the same call trace as on the board, and 0 of
6000 with it.

Independent of the timing, a debug-only device_lock_assert(&udev->dev)
in usb_probe_interface() fired on every async probe of a hub interface
without the patch (52 of 52) and never with it (0 of 102).

I can post the emulator and the test scripts if that helps.

>> But the affected code is still the
>> same in v7.3-rc6. In dd.c, __device_attach_async_helper(),
>> __device_attach() and __driver_attach_async_helper() are unchanged
>> since v6.16. On the USB side, usb_set_interface() and
>> create_intf_ep_devs() are unchanged, and usb_set_configuration() and
>> hub_configure() only got the kmalloc_obj() conversions.
>>
>>>> Not covered: deferred_probe_work_func() also re-probes via
>>>> __device_attach() without the parent lock.
>>>>
>>>> Analysis and patch done with the help of Claude Code.
>>>
>>> This feels really wrong, what bus is the host controller on for these?
>>
>> The platform bus, it's the ChipIdea controller of the i.MX8MM:
>>    /sys/devices/platform/soc@0/32c00000.bus/32e50000.usb/ci_hdrc.1/usb1/1-1/1-1:1.0
>>
>> 1-1 is an onboard USB2514 hub (multi-TT). The host controller isn't
> 
> Does the fact that the onboard hub is multi-TT have any connection with
> the bug?  Not as far as I can see -- but I had to waste a minute
> thinking about it.  If you agree, please remove that irrelevant detail
> from the patch description.

Only with this symptom. For multi-TT hubs, hub_configure() calls
usb_set_interface(hdev, 0, 1) to select the TT-per-port altsetting,
and that's the call that creates the endpoint devices a second time
from hub_probe(). A single-TT hub doesn't make that call, so it won't
show this warning, but its probe still runs without the udev lock.

The missing lock itself isn't hub specific, so in v2 I'll say why the
hub has to be multi-TT instead of just naming it.

>> part of the race, both sides are in the USB core, for that one hub:
>>
>> - usb_generic_driver_probe() of 1-1 calls usb_set_configuration(),
>>   which holds the 1-1 lock, does device_add() for 1-1:1.0 and then
>>   create_intf_ep_devs() for it.
>>
>> - device_add() only queues the probe of 1-1:1.0. The async worker runs
>>   hub_probe() -> usb_set_interface(hdev, 0, 1) -> create_intf_ep_devs()
>>   with only the 1-1:1.0 lock held.
>>
>> create_intf_ep_devs() checks and sets intf->ep_devs_created without a
>> lock of its own, so both create ep_81. With a synchronous probe,
>> hub_probe() runs inside device_add() with the 1-1 lock held, and this
>> can't happen.
> 
> You should describe this race in more detail (like you just did here) in
> the patch description.  It will help explain exactly what it is you are
> fixing.

Will do in v2.

>> The USB core relies on that lock. need_parent_lock is documented as
>> "When probing or removing a device on this bus, the device core should
>> lock the device's parent", and usb_driver_claim_interface() says
>> "Callers must own the device lock, so driver probe() entries don't need
>> extra locking". __driver_attach_async_helper() takes the parent lock,
>> __device_attach_async_helper() doesn't.
> 
> FWIW, I agree that this is a real bug and your solution is the right
> approach for fixing it.
> 
> Alan Stern

Thanks for the review.  
Mario