Re: [PATCH v19 3/7] driver core: separate function to shutdown one device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The refactored `device_shutdown` discards the parent device pointer returned by `get_device(dev->parent)` and re-reads `dev->parent` inside `shutdown_one_device()`, creating a TOCTOU race condition that can lead to reference leaks and use-after-free.
--

--- Patch [3]: [PATCH v19 3/7] driver core: separate function to shutdown one device ---
commit c3cdb15ac9b6efba6077cade25c652c124eaa9c7
Author: David Jeffery <djeffery@redhat.com>

driver core: separate function to shutdown one device

This commit refactors device_shutdown() by extracting the shutdown logic for a
single device into a new separate function, shutdown_one_device(). This change
is intended as a preparatory step for supporting asynchronous device shutdown.

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 78b90326addb6..82a7af1f3ba2e 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4898,12 +4898,48 @@ int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
>  	return error;
>  }
>  
> +static void shutdown_one_device(struct device *dev)
> +{
> +	struct device *parent = dev->parent;

[Severity: High]
Does this lockless read of dev->parent introduce a race condition?

In the old code, the parent device pointer was captured and pinned in
device_shutdown() using parent = get_device(dev->parent).

With this refactoring, dev->parent is re-read here. If the parent changes
concurrently (for example, via device_move()), could we end up operating on a
parent device that we do not hold a reference to?

> +
> +	/* hold lock to avoid race with probe/release */
> +	if (parent)
> +		device_lock(parent);

[ ... ]

> @@ -4925,7 +4961,7 @@ void device_shutdown(void)
>  		 * prevent it from being freed because parent's
>  		 * lock is to be held
>  		 */
> -		parent = get_device(dev->parent);
> +		get_device(dev->parent);

[Severity: High]
Is there a reference leak here since the return value of get_device() is now
discarded?

If dev->parent changes right after this call, the original parent's reference
will be leaked. Later, shutdown_one_device() will lock and drop a reference
to the new parent instead, potentially leading to unbalanced reference counts
and use-after-free.

Should we pass the pinned parent device pointer into shutdown_one_device()
as an argument to ensure we lock and release the exact same device we took a
reference to?

>  		get_device(dev);
>  		/*
>  		 * Make sure the device is off the kset list, in the

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716230411.2767394-1-tarunsahu@google.com?part=3