Re: [PATCH v8 3/4] driver core: shut down devices asynchronously

[David Jeffery]

On Tue, Sep 10, 2024 at 8:14 PM stuart hayes <stuart.w.hayes@xxxxxxxxx> wrote:
>
...
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index b69b82da8837..52d64b419c01 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4832,6 +4832,13 @@ static void shutdown_one_device_async(void *data, async_cookie_t cookie)
>   {
>         struct device *dev = data;
>
> +       /*
> +        * Sanity check to prevent shutdown hang in case a parent or supplier
> +        * is in devices_kset list in the wrong order
> +        */
> +       if (dev->p->shutdown_after > cookie)
> +               dev->p->shutdown_after = cookie - 1;
> +
>         async_synchronize_cookie_domain(dev->p->shutdown_after + 1, &sd_domain);
>
>         shutdown_one_device(dev);

While the race window is really small, there is a potential race with
this fixup. It's possible for the shutdown operation to write a new
value to shutdown_after in the time between the if check and
shutdown_after being re-read and used in the
async_synchronize_cookie_domain call. Such a race would allow a too
high value to be used.

Instead, could do something like:

--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4833,8 +4833,12 @@ static void shutdown_one_device(struct device *dev)
 static void shutdown_one_device_async(void *data, async_cookie_t cookie)
 {
        struct device *dev = data;
+       async_cookie_t wait = dev->p->shutdown_after + 1;

-       async_synchronize_cookie_domain(dev->p->shutdown_after + 1, &sd_domain);
+       if (wait > cookie)
+               wait = cookie;
+
+       async_synchronize_cookie_domain(wait, &sd_domain);

        shutdown_one_device(dev);
 }

This reads the shutdown_after value once and avoids the race window
where its value being changed on another CPU could still cause a
potential deadlock.