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

[tarunsahu@google.com]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Race condition in device_add() abort path during system shutdown leading to Use-After-Free.
> --
>
> --- Patch [2]: [PATCH 2/9] driver core: prevent device_add() during system shutdown ---
> commit 17748fb46ccfdb0adadd634adbfe70a9fd601b9a
> Author: Tarun Sahu <tarunsahu@google.com>
>
> driver core: prevent device_add() during system shutdown
>
> This commit adds system state checks to device_add() to prevent new
> devices from being added while the system is halting, powering off, or
> restarting. It re-checks the state under devices_kset->list_lock to
> avoid a TOCTOU race window with device_shutdown().
>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
> [ ... ]
>> @@ -3699,6 +3720,18 @@ int device_add(struct device *dev)
>>  		goto Error;
>>  	}
>>  
>> +	/*
>> +	 * Check system_state again under list_lock to prevent a TOCTOU race
>> +	 * where device_shutdown() runs concurrently and misses this device.
>> +	 */
>> +	spin_lock(&devices_kset->list_lock);
>> +	if (unlikely(system_is_shutting_down())) {
>> +		spin_unlock(&devices_kset->list_lock);
>> +		error = -ESHUTDOWN;
>> +		goto ShutdownError;
>
> [Severity: High]
> Does this error path introduce a use-after-free race with device_shutdown()?
>
> If device_add() successfully links the device to devices_kset->list via
> kobject_add(), device_shutdown() could concurrently run and pull the
> newly added device from the list:
>
> device_shutdown() {
>     ...
>     spin_lock(&devices_kset->list_lock);
>     ...
>     dev = list_entry(devices_kset->list.prev, struct device, kobj.entry);
>     ...
>     list_del_init(&dev->kobj.entry);
>     spin_unlock(&devices_kset->list_lock);
>     ...
> }
>
> If device_add() then resumes, acquires the list lock, and sees
> system_is_shutting_down(), it will jump to ShutdownError and return
> -ESHUTDOWN.
>
> The caller of device_add() will typically treat this as a failure and
> free the device and its associated private data. Meanwhile,
> device_shutdown() is already outside the lock and actively executing the
> shutdown callbacks on that exact same device:
>
> device_shutdown() {
>     ...
>     if (dev->bus && dev->bus->shutdown) {
>         ...
>         dev->bus->shutdown(dev);
>     }
>     ...
> }
>
> Can this lead to device_shutdown() accessing freed device memory?

We can make device_add and device_shutdown synchronise like

+device_block_adding();
+wait_for_device_add();

calling these two in device_shutdown similiarly we have for probing.

~Tarun

>
>> +	}
>> +	spin_unlock(&devices_kset->list_lock);
>> +
>>  	/* notify platform of device entry */
>>  	device_platform_notify(dev);
>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260821142414.150892-1-djeffery@redhat.com?part=2