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

+Adding folks from the original message. Unfortunately, Sashiko drops
the people from the original message.

Tarun Sahu <[email protected]> writes:

> 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
>
>
> [email protected] 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 <[email protected]>
>>
>> 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/[email protected]?part=3