Re: [PATCH v19 4/7] driver core: do not always lock parent in shutdown

David Jeffery <[email protected]> writes:

> On Thu, Jul 16, 2026 at 10:59=E2=80=AFPM <[email protected]> wrote:
>>
>> Thank you for your contribution! Sashiko AI review found 2 potential iss=
ue(s) to consider:
>>
>> New issues:
>> - [High] Unprotected read of dev->parent introduces a Use-After-Free rac=
e with device_move() during reference acquisition.
>>
>> Pre-existing issues:
>> - [High] Dropping and re-acquiring device_lock(dev) allows dev->parent t=
o change, resulting in locking an obsolete parent.
>
> We could add additional locking around device_move to exclude moves
> during these times to quiet both the AI tool's concerns. However, both
> these concerns are already not possible in reality. The only user of
> need_parent_lock in the kernel which will use this code path is usb
> devices, and none of the rare devices which use device_move will move
> a usb device which has need_parent_lock set. Thus, there is no actual
> device_move which can cause these conditions the AI tool complains
> about.
>
> The AI tool is finding a non-existent issue by applying a very
> specific special state as a general condition. If really wanted, I
> could add a patch which locks out moves during these times as a form
> of future-proofing, but from my view of the code all it would be doing
> is creating additional code and complexity to the driver core while
> currently fixing nothing.
>

I agree, it is very special case. device_move is associated with virtual
devices only and need_parent_lock is used with phsyical bus (Only with
USB) to prevent port reset during device probe/unbind. Even in future,
I dont see a need where a virtual device using device_move will need
need_parent_lock ever.

But I suggest, We can add the comment like:

/*
 * device_move() should not be used on devices with need_parent_lock
 * set, as concurrent reparenting during device_shutdown() introduces
 * a potential race when locking the parent device.
 */

WDYT?

~Tarun


>> --
>>
>> --- Patch [4]: [PATCH v19 4/7] driver core: do not always lock parent in=
 shutdown ---
>> commit 6be56e5bea0fe214a5199c9f806c49e62e60127f
>> Author: David Jeffery <[email protected]>
>>
>> driver core: do not always lock parent in shutdown
>>
>> This patch modifies the device shutdown process to only lock a parent de=
vice
>> if it is strictly needed, rather than unconditionally. It also defers
>> acquiring a reference to the parent device until the point where the par=
ent
>> needs to be locked.
>>
>> > diff --git a/drivers/base/core.c b/drivers/base/core.c
>> > index 82a7af1f3ba2e..31f95e86856e0 100644
>> > --- a/drivers/base/core.c
>> > +++ b/drivers/base/core.c
>>
>> [ ... ]
>>
>> > @@ -4925,12 +4921,32 @@ static void shutdown_one_device(struct device =
*dev)
>> >  static void shutdown_one_device(struct device *dev)
>> >  {
>> > +     struct device *parent;
>> > +
>> > +     device_lock(dev);
>> > +
>> > +     /* use parent lock if needed to avoid race with probe/release */
>> > +     if (dev->bus && dev->bus->need_parent_lock && dev->p && !dev->p-=
>dead &&
>> > +         (parent =3D get_device(dev->parent))) {
>>
>> [Severity: High]
>> Does moving the parent reference acquisition out of the list_lock critic=
al
>> section introduce a race condition where the parent could be freed?
>>
>> In device_shutdown(), get_device(dev->parent) was previously called whil=
e
>> holding devices_kset->list_lock. Now that it is evaluated locklessly in
>> shutdown_one_device():
>>
>> CPU1
>> shutdown_one_device()
>>     evaluates dev->parent, but is preempted before get_device() is calle=
d
>>
>> CPU2
>> device_move()
>>     changes dev->parent and calls put_device(old_parent)
>>     dropping the last reference and freeing the memory
>>
>> When CPU1 resumes, could it pass the freed pointer into get_device() and
>> dereference it?
>>
>> > +             /* the parent lock needs to be acquired first, so re-loc=
k */
>> > +             device_unlock(dev);
>> > +
>> > +             device_lock(parent);
>> > +             device_lock(dev);
>> > +
>> > +             __shutdown_one_device(dev);
>>
>> [Severity: High]
>> This is a pre-existing issue, but does dropping and re-acquiring the dev=
ice
>> lock here allow the topology to change?
>>
>> If a concurrent device_move() reparents the device while device_lock(dev=
)
>> is dropped, it appears we might lock the obsolete parent. When we re-loc=
k
>> dev and call __shutdown_one_device(), we haven't verified that dev->pare=
nt
>> still matches the locked parent.
>>
>> Could this defeat the need_parent_lock synchronization and allow a
>> concurrent probe or release on the new parent to race with the shutdown?
>>
>> > +             device_unlock(dev);
>> >               device_unlock(parent);
>> > +             put_device(parent);
>> > +     } else {
>> > +             __shutdown_one_device(dev);
>> > +             device_unlock(dev);
>> > +     }
>> >
>> >       put_device(dev);
>> >  }
>>
>> --
>> Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260716230411.2=
[email protected]?part=3D4
>>