Re: [PATCH v19 3/7] driver core: separate function to shutdown one device

On Thu, Jul 16, 2026 at 10:50 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The refactored `device_shutdown` discards the parent device pointer returned by `get_device(dev->parent)` and re-reads `dev->parent` inside `shutdown_one_device()`, creating a TOCTOU race condition that can lead to reference leaks and use-after-free.

This gets cleaned up in the next patch, but I'll rework it so that
this patch has no functional code changes and is only a code move into
a separate function.

> --
>
> --- Patch [3]: [PATCH v19 3/7] driver core: separate function to shutdown one device ---
> commit c3cdb15ac9b6efba6077cade25c652c124eaa9c7
> Author: David Jeffery <[email protected]>
>
> driver core: separate function to shutdown one device
>
> This commit refactors device_shutdown() by extracting the shutdown logic for a
> single device into a new separate function, shutdown_one_device(). This change
> is intended as a preparatory step for supporting asynchronous device shutdown.
>
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 78b90326addb6..82a7af1f3ba2e 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -4898,12 +4898,48 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> >       return error;
> >  }
> >
> > +static void shutdown_one_device(struct device *dev)
> > +{
> > +     struct device *parent = dev->parent;
>
> [Severity: High]
> Does this lockless read of dev->parent introduce a race condition?
>
> In the old code, the parent device pointer was captured and pinned in
> device_shutdown() using parent = get_device(dev->parent).
>
> With this refactoring, dev->parent is re-read here. If the parent changes
> concurrently (for example, via device_move()), could we end up operating on a
> parent device that we do not hold a reference to?
>
> > +
> > +     /* hold lock to avoid race with probe/release */
> > +     if (parent)
> > +             device_lock(parent);
>
> [ ... ]
>
> > @@ -4925,7 +4961,7 @@ void device_shutdown(void)
> >                * prevent it from being freed because parent's
> >                * lock is to be held
> >                */
> > -             parent = get_device(dev->parent);
> > +             get_device(dev->parent);
>
> [Severity: High]
> Is there a reference leak here since the return value of get_device() is now
> discarded?
>
> If dev->parent changes right after this call, the original parent's reference
> will be leaked. Later, shutdown_one_device() will lock and drop a reference
> to the new parent instead, potentially leading to unbalanced reference counts
> and use-after-free.
>
> Should we pass the pinned parent device pointer into shutdown_one_device()
> as an argument to ensure we lock and release the exact same device we took a
> reference to?
>
> >               get_device(dev);
> >               /*
> >                * Make sure the device is off the kset list, in the
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
>