| 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
| ||
|
Message-ID: <CAGETcx91KahXY6ULXDpek3Xf6k4s646Y6+jn_LtfZPDDOpg7hA@mail.gmail.com>
Date: Tue, 13 Feb 2024 19:42:50 -0800
From: Saravana Kannan <saravanak@...gle.com>
To: David Jeffery <djeffery@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-nvme@...ts.infradead.org,
linux-scsi@...r.kernel.org, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J . Wysocki" <rafael@...nel.org>, Laurence Oberman <loberman@...hat.com>,
Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [RFC PATCH 1/6] minimal async shutdown infrastructure
On Wed, Feb 7, 2024 at 10:40 AM David Jeffery <djeffery@...hat.com> wrote:
>
> Adds the async_shutdown_start and async_shutdown_end calls to perform async
> shutdown. Implements a very minimalist method of async shutdown support
> within device_shutdown(). The device at the head of the shutdown list is
> checked against a list of devices under async shutdown. If the head is a
> parent of a device on the async list, all active async shutdown operations
> are completed before the parent's shutdown call is performed.
>
> The number of async operations also has a max limit to prevent the list being
> checked for a child from getting overly large.
>
> Signed-off-by: David Jeffery <djeffery@...hat.com>
> Tested-by: Laurence Oberman <loberman@...hat.com>
>
> ---
> drivers/base/core.c | 116 +++++++++++++++++++++++++++++++++-
> include/linux/device/bus.h | 8 ++-
> include/linux/device/driver.h | 7 ++
> 3 files changed, 127 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 14d46af40f9a..5bc2282c00cd 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4719,12 +4719,92 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> }
> EXPORT_SYMBOL_GPL(device_change_owner);
>
> +
> +#define MAX_ASYNC_SHUTDOWNS 32
> +static int async_shutdown_count;
> +static LIST_HEAD(async_shutdown_list);
> +
> +/**
> + * If a device has a child busy with an async shutdown or there are too many
> + * async shutdowns active, the device may not be shut down at this time.
> + */
> +static bool may_shutdown_device(struct device *dev)
> +{
> + struct device *tmp;
> +
> + if (async_shutdown_count >= MAX_ASYNC_SHUTDOWNS)
> + return false;
> +
> + list_for_each_entry(tmp, &async_shutdown_list, kobj.entry) {
> + if (tmp->parent == dev)
> + return false;
> + }
> + return true;
> +}
> +
> +/**
> + * Call and track each async shutdown call
> + */
> +static void async_shutdown_start(struct device *dev, void (*callback) (struct device *))
> +{
> + if (initcall_debug)
> + dev_info(dev, "async_shutdown_start\n");
> +
> + (*callback)(dev);
> + list_add_tail(&dev->kobj.entry, &async_shutdown_list);
> + async_shutdown_count++;
> +}
> +
> +/**
> + * Wait for all async shutdown operations currently active to complete
> + */
> +static void wait_for_active_async_shutdown(void)
> +{
> + struct device *dev, *parent;
> +
> + while (!list_empty(&async_shutdown_list)) {
> + dev = list_entry(async_shutdown_list.next, struct device,
> + kobj.entry);
> +
> + parent = dev->parent;
I didn't check the code thoroughly, but so there might be other big
issues. But you definitely need to take device links into account.
Shutdown all your consumers first similar to how you shutdown the
children devices first. Look at the async suspend/resume code for some
guidance.
> +
> + /*
> + * Make sure the device is off the list
> + */
> + list_del_init(&dev->kobj.entry);
> + if (parent)
> + device_lock(parent);
> + device_lock(dev);
> + if (dev->bus && dev->bus->async_shutdown_end) {
> + if (initcall_debug)
> + dev_info(dev,
> + "async_shutdown_end called\n");
> + dev->bus->async_shutdown_end(dev);
> + } else if (dev->driver && dev->driver->async_shutdown_end) {
> + if (initcall_debug)
> + dev_info(dev,
> + "async_shutdown_end called\n");
> + dev->driver->async_shutdown_end(dev);
> + }
> + device_unlock(dev);
> + if (parent)
> + device_unlock(parent);
> +
> + put_device(dev);
> + put_device(parent);
> + }
> + if (initcall_debug)
> + printk(KERN_INFO "device shutdown: waited for %d async shutdown callbacks\n", async_shutdown_count);
> + async_shutdown_count = 0;
> +}
> +
> /**
> * device_shutdown - call ->shutdown() on each device to shutdown.
> */
> void device_shutdown(void)
> {
> struct device *dev, *parent;
> + bool async_busy;
>
> wait_for_device_probe();
> device_block_probing();
> @@ -4741,6 +4821,8 @@ void device_shutdown(void)
> dev = list_entry(devices_kset->list.prev, struct device,
> kobj.entry);
>
> + async_busy = false;
> +
> /*
> * hold reference count of device's parent to
> * prevent it from being freed because parent's
> @@ -4748,6 +4830,17 @@ void device_shutdown(void)
> */
> parent = get_device(dev->parent);
> get_device(dev);
> +
> + if (!may_shutdown_device(dev)) {
> + put_device(dev);
> + put_device(parent);
> +
> + spin_unlock(&devices_kset->list_lock);
> + wait_for_active_async_shutdown();
> + spin_lock(&devices_kset->list_lock);
> + continue;
> + }
> +
> /*
> * Make sure the device is off the kset list, in the
> * event that dev->*->shutdown() doesn't remove it.
> @@ -4769,26 +4862,43 @@ void device_shutdown(void)
> dev_info(dev, "shutdown_pre\n");
> dev->class->shutdown_pre(dev);
> }
> - if (dev->bus && dev->bus->shutdown) {
> + if (dev->bus && dev->bus->async_shutdown_start) {
> + async_shutdown_start(dev, dev->bus->async_shutdown_start);
> + async_busy = true;
> + } else if (dev->bus && dev->bus->shutdown) {
> if (initcall_debug)
> dev_info(dev, "shutdown\n");
> dev->bus->shutdown(dev);
> + } else if (dev->driver && dev->driver->async_shutdown_start) {
> + async_shutdown_start(dev, dev->driver->async_shutdown_start);
> + async_busy = true;
> } else if (dev->driver && dev->driver->shutdown) {
> if (initcall_debug)
> dev_info(dev, "shutdown\n");
> dev->driver->shutdown(dev);
> + } else {
> + if (initcall_debug)
> + dev_info(dev, "no shutdown callback\n");
> }
>
> device_unlock(dev);
> if (parent)
> device_unlock(parent);
>
> - put_device(dev);
> - put_device(parent);
> + /* if device has an async shutdown, drop the ref when done */
> + if (!async_busy) {
> + put_device(dev);
> + put_device(parent);
> + }
>
> spin_lock(&devices_kset->list_lock);
> }
> spin_unlock(&devices_kset->list_lock);
> + /*
> + * Wait for any async shutdown still running.
> + */
> + if (!list_empty(&async_shutdown_list))
> + wait_for_active_async_shutdown();
> }
>
> /*
> diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
> index 5ef4ec1c36c3..7a4a2ff0bc23 100644
> --- a/include/linux/device/bus.h
> +++ b/include/linux/device/bus.h
> @@ -48,7 +48,11 @@ struct fwnode_handle;
> * will never get called until they do.
> * @remove: Called when a device removed from this bus.
> * @shutdown: Called at shut-down time to quiesce the device.
> - *
> + * @async_shutdown_start: Optional call to support and begin the shutdown
> + * process on the device in an asynchronous manner.
> + * @async_shutdown_end: Optional call to complete an asynchronous
> + * shutdown of the device. Must be provided if a
> + * sync_shutdown_start call is provided.
> * @online: Called to put the device back online (after offlining it).
> * @offline: Called to put the device offline for hot-removal. May fail.
> *
> @@ -87,6 +91,8 @@ struct bus_type {
> void (*sync_state)(struct device *dev);
> void (*remove)(struct device *dev);
> void (*shutdown)(struct device *dev);
> + void (*async_shutdown_start)(struct device *dev);
> + void (*async_shutdown_end)(struct device *dev);
>
> int (*online)(struct device *dev);
> int (*offline)(struct device *dev);
> diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
> index 7738f458995f..af0ad2d3687a 100644
> --- a/include/linux/device/driver.h
> +++ b/include/linux/device/driver.h
> @@ -71,6 +71,11 @@ enum probe_type {
> * @remove: Called when the device is removed from the system to
> * unbind a device from this driver.
> * @shutdown: Called at shut-down time to quiesce the device.
> + * @async_shutdown_start: Optional call to support and begin the shutdown
> + * process on the device in an asynchronous manner.
> + * @async_shutdown_end: Optional call to complete an asynchronous
> + * shutdown of the device. Must be provided if a
> + * sync_shutdown_start call is provided.
> * @suspend: Called to put the device to sleep mode. Usually to a
> * low power state.
> * @resume: Called to bring a device from sleep mode.
> @@ -110,6 +115,8 @@ struct device_driver {
> void (*sync_state)(struct device *dev);
> int (*remove) (struct device *dev);
> void (*shutdown) (struct device *dev);
> + void (*async_shutdown_start) (struct device *dev);
> + void (*async_shutdown_end) (struct device *dev);
Why not use the existing shutdown and call it from an async thread and
wait for it to finish? Similar to how async probes are handled. Also,
adding separate ops for this feels clunky and a very narrow fix. Just
use a flag to indicate the driver can support async shutdown using the
existing shutdown() op.
-Saravana
Powered by blists - more mailing lists