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

David Jeffery
2024-09-12T14:30:41Z

On Tue, Sep 10, 2024 at 8:14 PM stuart hayes <stuart.w.hayes@gmail.com> 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.