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

On Thu, Jul 16, 2026 at 10:59 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Unprotected read of dev->parent introduces a Use-After-Free race with device_move() during reference acquisition.
>
> Pre-existing issues:
> - [High] Dropping and re-acquiring device_lock(dev) allows dev->parent to 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.

> --
>
> --- 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 device
> if it is strictly needed, rather than unconditionally. It also defers
> acquiring a reference to the parent device until the point where the parent
> 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 = get_device(dev->parent))) {
>
> [Severity: High]
> Does moving the parent reference acquisition out of the list_lock critical
> section introduce a race condition where the parent could be freed?
>
> In device_shutdown(), get_device(dev->parent) was previously called while
> 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 called
>
> 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-lock */
> > +             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 device
> 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-lock
> dev and call __shutdown_one_device(), we haven't verified that dev->parent
> 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 · https://sashiko.dev/#/patchset/[email protected]?part=4
>