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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+-xHTEMM09PXgWyKX4h48diUxxGnSSrDowh5Gt=Y+EVhHL-_Q@mail.gmail.com>
Date: Thu, 12 Sep 2024 10:30:23 -0400
From: David Jeffery <djeffery@...hat.com>
To: stuart hayes <stuart.w.hayes@...il.com>
Cc: Jan Kiszka <jan.kiszka@...mens.com>, linux-kernel@...r.kernel.org, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J . Wysocki" <rafael@...nel.org>, 
	Martin Belanger <Martin.Belanger@...l.com>, "Oliver O'Halloran" <oohall@...il.com>, 
	Daniel Wagner <dwagner@...e.de>, Keith Busch <kbusch@...nel.org>, Lukas Wunner <lukas@...ner.de>, 
	Jeremy Allison <jallison@....com>, Jens Axboe <axboe@...com>, Christoph Hellwig <hch@....de>, 
	Sagi Grimberg <sagi@...mberg.me>, linux-nvme@...ts.infradead.org, 
	Nathan Chancellor <nathan@...nel.org>
Subject: Re: [PATCH v8 3/4] driver core: shut down devices asynchronously

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