Re: [PATCH 3/5] driver core: async device shutdown infrastructure

[tarunsahu@google.com]

Hello,

These most of the errors are due to the device_add concurrently can add
the device and device_shutdown can access it. One of the reason I think
a device_add will be triggered while being shutdown is inserting the
physical PCI device or USB stick etc.

But During shutdown, addition of a new device is not valid. So we can
add a check something like

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 76ba02c26aa5..c3795fa1cc26 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3650,6 +3650,13 @@ int device_add(struct device *dev)
        if (!dev)
                goto done;

+       if (unlikely(system_state == SYSTEM_HALT ||
+                    system_state == SYSTEM_POWER_OFF ||
+                    system_state == SYSTEM_RESTART)) {
+               error = -ESHUTDOWN;
+               goto done;
+       }
+
        if (!dev->p) {
                error = device_private_init(dev);
                if (error)

We can also partially resolve the races partially by removing the manual
kfree(dev->p) from device_add() error path. As put_device free it
anyway when reference to devices reaches to 0. So if device_shutdown
path takes reference, dev->p will not be released. And few races will be
taken care of. But this will not resolve the deadlock and other races
like the above change.

removing the kfree(dev->p) from device_add can be a independent patch.
like:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4d026682944f..76ba02c26aa5 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2614,6 +2614,7 @@ static void device_release(struct kobject *kobj)
        struct device *dev = kobj_to_dev(kobj);
        struct device_private *p = dev->p;

+       dev->p = NULL;
        /*
         * Some platform devices are driven without driver attached
         * and managed resources may have been acquired.  Make sure
@@ -3824,8 +3825,6 @@ int device_add(struct device *dev)
 parent_error:
        put_device(parent);
 name_error:
-       kfree(dev->p);
-       dev->p = NULL;
        goto done;
 }
 EXPORT_SYMBOL_GPL(device_add);


~Tarun


sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Modifying the core.async_shutdown module parameter during shutdown causes synchronous suppliers to skip waiting for asynchronous consumers.
> - [High] Unregistered consumer devices will cause a NULL pointer dereference during async shutdown dependency waiting.
> - [High] Adding a new device with async shutdown enabled while its synchronous supplier is being processed leads to an unresolvable deadlock.
> - [High] Dereferencing `dev->p->dead` without NULL checks causes a crash if a concurrent `device_add()` fails.
> --
>
> commit cf98cdf76df805443cc885868cb3d9e13da5501c
> Author: David Jeffery <djeffery@redhat.com>
>
> driver core: async device shutdown infrastructure
>
> This commit allows devices to opt into asynchronous shutdown, executing in
> parallel with unrelated devices after their dependencies shut down. Synchronous
> shutdown devices will wait for their asynchronous dependencies. The feature is
> enabled by default and can be toggled via the core.async_shutdown parameter.
>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
> [ ... ]
>> +static bool wants_async_shutdown(struct device *dev)
>> +{
>> +	return async_shutdown && dev_async_shutdown(dev);
>> +}
>> +
>> +static int wait_for_device_shutdown(struct device *dev, void *data)
>> +{
>> +	bool async = *(bool *)data;
>> +
>> +	if (async || wants_async_shutdown(dev))
>> +		wait_for_completion(&dev->p->complete);
>> +
>> +	return 0;
>> +}
>
> [Severity: High]
> If a user-space process toggles the 0644 core.async_shutdown sysfs parameter
> from 1 to 0 during shutdown, could this cause synchronous suppliers to skip
> waiting for their asynchronous consumers?
>
> If start_async_shutdown_devices() schedules an async consumer, and then the
> parameter is toggled before the sync loop processes its supplier, the call to
> wants_async_shutdown(dev) here would evaluate to false. 
>
> This would skip the wait_for_completion(), potentially causing the supplier
> (e.g., a PCI bus) to shut down while its consumer is still active.
>
> [Severity: High]
> Could this result in a NULL pointer dereference if the consumer is not yet
> registered?
>
> The device_link_add() function allows creating links to consumer devices that
> haven't been registered yet, meaning their dev->p is NULL because
> device_private_init() hasn't run.
>
> If wait_for_shutdown_dependencies() is called from the async pool, where the
> async argument is forced to true, the wait_for_completion() will
> unconditionally evaluate &dev->p->complete, crashing on the NULL pointer.
>
> [ ... ]
>> +static void __shutdown_one_device(struct device *dev)
>>  {
>>  	if (dev->p->dead)
>>  		return;
>
> [Severity: High]
> Could this cause a NULL pointer dereference if a concurrent device_add() fails?
>
> If a concurrent device_add() fails after calling kobject_add(), its error path
> calls kobject_del() and then frees and nullifies dev->p. Evaluating
> dev->p->dead without checking if dev->p is valid could crash here.
>
> [ ... ]
>> +static void start_async_shutdown_devices(void)
>> +{
>> +	struct device *dev, *next, *ndev, *needs_put = NULL;
> [ ... ]
>> +			needs_put = ndev;
>> +			spin_lock(&devices_kset->list_lock);
>> +			/*
>> +			 * If the next device has been marked dead while the
>> +			 * spinlock was released, it may no longer be on the
>> +			 * devices_kset list. Restart the list walk to be safe
>> +			 */
>> +			if (ndev && ndev->p->dead)
>> +				goto restart;
>
> [Severity: High]
> Could ndev->p be NULL if a concurrent device_add() fails?
>
> If start_async_shutdown_devices() obtains a reference to next via
> get_device(), and concurrently device_add() fails and clears dev->p,
> reacquiring the lock and evaluating ndev->p->dead here could result in a NULL
> pointer dereference.
>
> [ ... ]
>>  void device_shutdown(void)
>>  {
> [ ... ]
>> -		shutdown_one_device(dev);
>> +		if (!wants_async_shutdown(dev) || !shutdown_device_async(dev)) {
>> +			wait_for_shutdown_dependencies(dev, false);
>> +			shutdown_one_device(dev);
>> +		}
>>  
>>  		spin_lock(&devices_kset->list_lock);
>>  	}
>>  	spin_unlock(&devices_kset->list_lock);
>
> [Severity: High]
> Can this sequence lead to an unresolvable deadlock if a new device is added
> concurrently?
>
> If a new device 'C' with DEV_FLAG_ASYNC_SHUTDOWN is added via hotplug
> after its synchronous supplier 'S' has already been popped from
> devices_kset by this loop, 'C' is appended to devices_kset.
>
> 'S' will call wait_for_shutdown_dependencies() and find 'C', blocking on
> wait_for_completion(&C->p->complete) because wants_async_shutdown(C) is true.
>
> Since 'C' was added after start_async_shutdown_devices() ran, it is not
> running in the async pool. Because the sync loop is blocked on 'S', it will
> never reach 'C' to start it, deadlocking the shutdown process permanently.
>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260701135015.81937-1-djeffery@redhat.com?part=3