Re: [PATCH 3/9] driver core: warn should device_move try to move a need_parent_lock device

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

Pre-existing issues:
- [Critical] Concurrent execution of device_move() and PM runtime operations causes a Use-After-Free on the parent device pointer.
- [High] device_move() corrupts PM runtime child_count accounting, potentially causing active devices to panic when their parent is prematurely suspended.
--

--- Patch [3]: [PATCH 3/9] driver core: warn should device_move try to move a need_parent_lock device ---
commit da39f0a4e63233986f7aa69f70fc28bb25510d0a
Author: David Jeffery <[email protected]>

driver core: warn should device_move try to move a need_parent_lock device

Currently, no device has need_parent_lock set and is moved by
device_move. need_parent_lock is only set by the usb bus and very
few device types ever use device_move.

Adds a warning to device_move to catch should it ever be used on a
device with need_parent_lock set.

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 78b90326addb6..20b37ee07b5ac 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4708,6 +4708,14 @@ int device_move(struct device *dev, struct device *new_parent,
>  	if (!dev)
>  		return -EINVAL;
>  
> +	/*
> +	 * device_move() should not be used on devices with need_parent_lock
> +	 * set. Concurrent reparenting will violate the immutable
> +	 * relationship needed while locking and unlocking both parent and
> +	 * child.
> +	 */
> +	WARN_ON(dev->bus && dev->bus->need_parent_lock);
> +
>  	device_pm_lock();
>  	new_parent = get_device(new_parent);
>  	new_parent_kobj = get_device_parent(dev, new_parent);

[Severity: Critical]
This is a pre-existing issue, but does this code allow a use-after-free of
the parent pointer if rpm_resume() runs concurrently with device_move()?

In drivers/base/power/runtime.c:rpm_resume(), dev->parent is read into a
local variable while holding dev->power.lock, but the lock is released
before pm_runtime_get_noresume() takes a reference to it:

drivers/base/power/runtime.c:rpm_resume() {
    ...
	parent = dev->parent;
	if (dev->power.irq_safe)
		goto skip_parent;

	spin_unlock(&dev->power.lock);

	pm_runtime_get_noresume(parent);
    ...
}

Meanwhile, device_move() modifies dev->parent and drops the reference to
old_parent without holding dev->power.lock:

drivers/base/core.c:device_move() {
    ...
	old_parent = dev->parent;
	dev->parent = new_parent;
    ...
	put_device(old_parent);
    ...
}

If device_move() frees old_parent during the lockless window in rpm_resume(),
this could cause a use-after-free memory corruption.

[Severity: High]
This isn't a bug introduced by this patch, but when device_move() reparents
an active device, could it corrupt the PM runtime child_count accounting?

When reparenting an active device, device_move() does not transfer the PM
runtime child_count from the old parent to the new parent:

drivers/base/core.c:device_move() {
    ...
	old_parent = dev->parent;
	dev->parent = new_parent;
    ...
}

When the moved device later suspends, rpm_suspend() decrements the child_count
of the new parent:

drivers/base/power/runtime.c:rpm_suspend() {
    ...
	if (dev->parent) {
		parent = dev->parent;
		atomic_add_unless(&parent->power.child_count, -1, 0);
	}
    ...
}

If the new parent has other active children, its child_count will prematurely
reach zero. This could cause the PM core to suspend the parent while its
other children are still actively performing I/O, potentially resulting in a
kernel panic or hardware lockup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3