Re: [PATCH 4/4] software node: skip dying children during traversal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] software node: unsynchronized child list traversal races with release
--

commit 9feb88166afe18788cf51497c2968b7148e1d347
Author: Peng Fan <peng.fan@nxp.com>

software node: skip dying children during traversal

This commit updates software_node_get_next_child() and
software_node_get_named_child_node() to use kobject_get_unless_zero() so
that child swnodes whose kobject refcount has reached zero are skipped
during child list traversal.

> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 4d95a59a27f0a..1354447069e10 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -515,7 +515,18 @@ software_node_get_next_child(const struct fwnode_handle *fwnode,
>  		c = list_first_entry(&p->children, struct swnode, entry);
>  
>  	fwnode_handle_put(child);
> -	return fwnode_handle_get(&c->fwnode);
> +
> +	for (;;) {
> +		if (kobject_get_unless_zero(&c->kobj))
> +			return &c->fwnode;
> +
> +		if (list_is_last(&c->entry, &p->children))
> +			break;
> +
> +		c = list_next_entry(c, entry);
> +	}
> +
> +	return NULL;
>  }
>  
>  static struct fwnode_handle *
> @@ -529,10 +540,9 @@ software_node_get_named_child_node(const struct fwnode_handle *fwnode,
>  		return NULL;
>  
>  	list_for_each_entry(child, &swnode->children, entry) {
> -		if (!strcmp(childname, kobject_name(&child->kobj))) {
> -			swnode_get(child);
> +		if (!strcmp(childname, kobject_name(&child->kobj)) &&
> +		    kobject_get_unless_zero(&child->kobj))
>  			return &child->fwnode;

[Severity: High]
Can walking the parent's children list and skipping zero-refcount nodes here
race with software_node_release() and trigger a use-after-free?

Unlike swnode_kset->list, which is protected by swnode_kset->list_lock in
software_node_to_swnode_get() and software_node_find_by_name() and unlinked
under that same lock in kobj_kset_leave() before software_node_release()
runs, parent->children has no lock serializing list_add_tail() in
swnode_register(), list_del() and kfree() in software_node_release(), or the
traversal in software_node_get_next_child() and
software_node_get_named_child_node():

drivers/base/swnode.c:software_node_release() {
	struct swnode *swnode = kobj_to_swnode(kobj);

	if (swnode->parent) {
		ida_free(&swnode->parent->child_ids, swnode->id);
		list_del(&swnode->entry);
	} else {
		ida_free(&swnode_root_ids, swnode->id);
	}
	...
	kfree(swnode);
}

In software_node_get_next_child(), when kobject_get_unless_zero(&c->kobj)
returns NULL because c's refcount has already dropped to zero, nothing pins
c in memory or on p->children. A concurrent software_node_release() can run
list_del(&c->entry) (poisoning c->entry.next with LIST_POISON1) and kfree(c)
before the loop checks list_is_last(&c->entry, &p->children) and advances
via list_next_entry(c, entry), or even right after list_first_entry() or
list_next_entry() loads c.

Similarly, in software_node_get_named_child_node(), list_for_each_entry()
walks swnode->children without any lock and dereferences
kobject_name(&child->kobj) before calling kobject_get_unless_zero(), while
software_node_release() may be concurrently unlinking and freeing child.

Does traversal and modification of the parent's children list (and child_ids
in software_node_release()) need a lock to synchronize against concurrent
child release?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-swnode-race-v1-v1-0-9406a67e25be@nxp.com?part=4