Re: [PATCH 2/9] driver core: prevent device_add() during system shutdown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] driver core: device_shutdown() calls synchronize_srcu() after blocking probes
- [High] driver core: device_add() blocks nested device_link_add() during shutdown
--

commit fff836fc21ae90945fdc03da97b97959ca2c2584
Author: Tarun Sahu <tarunsahu@google.com>

driver core: prevent device_add() during system shutdown

This commit prevents new devices from being registered during system
shutdown by checking system_is_shutting_down() in device_add() and
synchronizing in-flight device_add() calls in device_shutdown() using SRCU.

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 83263e3fa5d44..bce555dd74f69 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c

[ ... ]

> @@ -3647,13 +3666,20 @@ int device_add(struct device *dev)
>  	struct device *parent;
>  	struct kobject *kobj;
>  	struct class_interface *class_intf;
> -	int error = -EINVAL;
> +	int idx, error = -EINVAL;
>  	struct kobject *glue_dir = NULL;
>  
> +	idx = srcu_read_lock(&device_add_srcu);
> +
>  	dev = get_device(dev);
>  	if (!dev)
>  		goto done;
>  
> +	if (unlikely(system_is_shutting_down())) {
> +		error = -ESHUTDOWN;
> +		goto done;
> +	}

[Severity: High]
Can this check in device_add() break nested device_link_add() calls for an
in-flight supplier registration when system_state transitions to a shutdown
state?

Suppose a consumer device is already registered in devices_kset->list, and
its supplier device is being registered in thread A:

Thread A:
device_add(sup_dev)
  srcu_read_lock(&device_add_srcu)
  system_is_shutting_down() -> false
  kobject_add(&sup_dev->kobj, ...)
    -> appends sup_dev to the tail of devices_kset->list (after con_dev)

If thread B now initiates shutdown, sets system_state to SYSTEM_RESTART,
SYSTEM_HALT, or SYSTEM_POWER_OFF, and waits in device_shutdown() at
synchronize_srcu(&device_add_srcu), thread A continues in device_add():

  if (dev->fwnode && !dev->fwnode->dev) {
          dev->fwnode->dev = dev;
          fw_devlink_link_device(dev);
  }

fw_devlink_link_device() walks the fwnode links and calls
device_link_add(), which registers the link device via device_register():

drivers/base/core.c:device_link_add() {
    ...
	if (device_register(&link->link_dev)) {
		put_device(&link->link_dev);
		link = NULL;
		goto out;
	}
    ...
reorder:
	device_reorder_to_tail(consumer, NULL);
    ...
}

Because system_is_shutting_down() is now true, the nested
device_register(&link->link_dev) -> device_add(&link->link_dev) fails with
-ESHUTDOWN.

As a result, device_link_add() returns NULL without creating the device
link and without calling device_reorder_to_tail(consumer, NULL) to move the
consumer after the supplier in devices_kset->list (and
__fw_devlink_link_to_consumers() / __fw_devlink_link_to_suppliers() drop
the fwnode_link when fw_devlink_create_devlink() returns -EINVAL).

When synchronize_srcu(&device_add_srcu) completes and device_shutdown()
walks devices_kset->list backward from tail to head, wouldn't the supplier
sup_dev be shut down before its consumer con_dev?

> +
>  	if (!dev->p) {
>  		error = device_private_init(dev);
>  		if (error)
> @@ -3803,6 +3829,7 @@ int device_add(struct device *dev)
>  	}
>  done:
>  	put_device(dev);
> +	srcu_read_unlock(&device_add_srcu, idx);
>  	return error;
>   SysEntryError:
>  	if (MAJOR(dev->devt))
> @@ -4877,6 +4904,7 @@ void device_shutdown(void)
>  
>  	wait_for_device_probe();
>  	device_block_probing();
> +	synchronize_srcu(&device_add_srcu);

[Severity: High]
Should synchronize_srcu(&device_add_srcu) be called before
wait_for_device_probe() and device_block_probing() instead of after them?

If thread A is inside device_add(dev) and has already passed
system_is_shutting_down() before system_state is updated, but has not yet
reached bus_probe_device(dev), thread B can run wait_for_device_probe() and
device_block_probing() to completion and then block in
synchronize_srcu(&device_add_srcu).

While thread B is waiting in synchronize_srcu(&device_add_srcu), thread A
reaches:

device_add(dev)
  bus_probe_device(dev)
    device_initial_probe(dev)
      __device_attach(dev, true)

In drivers/base/dd.c:__device_attach(), if dev matches an asynchronous
driver on the bus, __device_attach_driver() sets data.have_async = true and
returns 0 before reaching driver_probe_device() / really_probe() (where
defer_all_probes is checked), and __device_attach() then schedules async
probing without checking defer_all_probes:

drivers/base/dd.c:__device_attach() {
    ...
		ret = bus_for_each_drv(dev->bus, NULL, &data,
					__device_attach_driver);
		if (!ret && allow_async && data.have_async) {
			dev_dbg(dev, "scheduling asynchronous probe\n");
			get_device(dev);
			async = true;
		} else {
			pm_request_idle(dev);
		}

		if (dev->parent)
			pm_runtime_put(dev->parent);
	}
out_unlock:
	device_unlock(dev);
	if (async)
		async_schedule_dev(__device_attach_async_helper, dev);
    ...
}

Similarly, if dev->driver was already set prior to device_add(dev),
__device_attach() calls device_bind_driver(dev) -> driver_bound(dev) ->
driver_deferred_probe_trigger(), queuing deferred_probe_work on
system_dfl_wq.

Once thread A calls srcu_read_unlock(&device_add_srcu, idx),
synchronize_srcu(&device_add_srcu) in device_shutdown() returns without
waiting for probes again. Can this allow __device_attach_async_helper()
(which performs runtime PM calls on dev and dev->parent) or
deferred_probe_work_func() (which reorders devices_kset->list via
device_pm_move_to_tail()) to run concurrently with device_shutdown()?

>  
>  	cpufreq_suspend();
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020164435.80499-1-djeffery@redhat.com?part=2