Re: [PATCH v19 4/7] driver core: do not always lock parent in shutdown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unprotected read of dev->parent introduces a Use-After-Free race with device_move() during reference acquisition.

Pre-existing issues:
- [High] Dropping and re-acquiring device_lock(dev) allows dev->parent to change, resulting in locking an obsolete parent.
--

--- Patch [4]: [PATCH v19 4/7] driver core: do not always lock parent in shutdown ---
commit 6be56e5bea0fe214a5199c9f806c49e62e60127f
Author: David Jeffery <djeffery@redhat.com>

driver core: do not always lock parent in shutdown

This patch modifies the device shutdown process to only lock a parent device
if it is strictly needed, rather than unconditionally. It also defers
acquiring a reference to the parent device until the point where the parent
needs to be locked.

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 82a7af1f3ba2e..31f95e86856e0 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c

[ ... ]

> @@ -4925,12 +4921,32 @@ static void shutdown_one_device(struct device *dev)
>  static void shutdown_one_device(struct device *dev)
>  {
> +	struct device *parent;
> +
> +	device_lock(dev);
> +
> +	/* use parent lock if needed to avoid race with probe/release */
> +	if (dev->bus && dev->bus->need_parent_lock && dev->p && !dev->p->dead &&
> +	    (parent = get_device(dev->parent))) {

[Severity: High]
Does moving the parent reference acquisition out of the list_lock critical
section introduce a race condition where the parent could be freed?

In device_shutdown(), get_device(dev->parent) was previously called while
holding devices_kset->list_lock. Now that it is evaluated locklessly in
shutdown_one_device():

CPU1
shutdown_one_device()
    evaluates dev->parent, but is preempted before get_device() is called

CPU2
device_move()
    changes dev->parent and calls put_device(old_parent)
    dropping the last reference and freeing the memory

When CPU1 resumes, could it pass the freed pointer into get_device() and
dereference it?

> +		/* the parent lock needs to be acquired first, so re-lock */
> +		device_unlock(dev);
> +
> +		device_lock(parent);
> +		device_lock(dev);
> +
> +		__shutdown_one_device(dev);

[Severity: High]
This is a pre-existing issue, but does dropping and re-acquiring the device
lock here allow the topology to change?

If a concurrent device_move() reparents the device while device_lock(dev)
is dropped, it appears we might lock the obsolete parent. When we re-lock
dev and call __shutdown_one_device(), we haven't verified that dev->parent
still matches the locked parent.

Could this defeat the need_parent_lock synchronization and allow a
concurrent probe or release on the new parent to race with the shutdown?

> +		device_unlock(dev);
>  		device_unlock(parent);
> +		put_device(parent);
> +	} else {
> +		__shutdown_one_device(dev);
> +		device_unlock(dev);
> +	}
>  
>  	put_device(dev);
>  }

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