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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Sysfs tree corruption during rollback of device_move_class_links failure.
- [High] Unconditional cleanup on failed rollback of device_move_class_links leads to Use-After-Free of new_parent and its glue directory.
--

commit 80ab7b1ec98b82ed303226de698cbc0d137b6732
Author: David Jeffery <djeffery@redhat.com>

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

This commit adds a warning to device_move() to catch any attempts to move a
device with need_parent_lock set. Moving such a device would break the
immutable parent-child relationship required for locking.

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index bce555dd74f69..e8a62c8f0ad3d 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4705,6 +4705,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);

Later in device_move():

> 	if (dev->class) {
> 		error = device_move_class_links(dev, old_parent, new_parent);
> 		if (error) {
> 			/* We ignore errors on cleanup since we're hosed anyway... */
> 			device_move_class_links(dev, new_parent, old_parent);
> 			if (!kobject_move(&dev->kobj, &old_parent->kobj)) {
                                                      ^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but does this rollback correctly restore the
device's sysfs location?

The rollback explicitly uses &old_parent->kobj instead of the original glue
directory.

If a device uses a class glue directory, rolling back a failed move bypasses
the glue directory and places the device directly under the old parent's
kobject.

Can this break sysfs expectations for userspace and leak the original glue
directory during subsequent device removal if device_move_class_links()
fails?

> 				if (new_parent)
> 					klist_remove(&dev->p->knode_parent);
> 				dev->parent = old_parent;
> 				if (old_parent) {
> 					klist_add_tail(&dev->p->knode_parent,
> 						       &old_parent->p->klist_children);
> 					set_dev_node(dev, dev_to_node(old_parent));
> 				}
> 			}
> 			cleanup_glue_dir(dev, new_parent_kobj);
> 			put_device(new_parent);
                        ^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to unconditionally
drop these references here?

These lines drop the references to the new parent and glue directory, even if
the rollback failed and the device still holds pointers to them.

If device_move_class_links() fails and the subsequent rollback attempt via
kobject_move() also fails, the device remains partially moved with its parent
and kobj.parent pointers attached to the new parent.

Can dropping the references unconditionally leave these pointers dangling,
leading to a use-after-free or double-free?

> 			goto out;
> 		}
> 	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902170734.95504-1-djeffery@redhat.com?part=3