Re: [PATCH v2 1/4] software node: do not resurrect swnode being released

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] software node: sysfs and parent collision in software_node_register_node()
--

commit 24164332b4b2134522c9b4a2c4cf027c9a0ece19
Author: Peng Fan <peng.fan@nxp.com>

software node: do not resurrect swnode being released

This commit updates software node lookups to use kobject_get_unless_zero() so
that software nodes whose reference count has reached zero are skipped instead
of being resurrected while awaiting kobject cleanup. It also introduces
software_node_register_node() to return the newly registered swnode reference
directly to device_add_software_node().

> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 84a77ffa3dc4b..965e960156876 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
[ ... ]
> @@ -977,21 +1014,39 @@ void software_node_unregister_node_group(const struct software_node * const *nod
>  }
>  EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
>  
> +static struct swnode *
> +software_node_register_node(const struct software_node *node)
> +{
> +	struct swnode *parent, *swnode;
> +	struct fwnode_handle *fwnode;
> +
> +	swnode = software_node_to_swnode_get(node);
> +	if (swnode) {
> +		swnode_put(swnode);
> +		return ERR_PTR(-EEXIST);
> +	}
> +
> +	parent = software_node_to_swnode_get(node->parent);
> +	if (node->parent && !parent)
> +		return ERR_PTR(-EINVAL);
> +
> +	fwnode = swnode_register(node, parent, 0);
> +	if (parent)
> +		swnode_put(parent);
> +
> +	if (IS_ERR(fwnode))
> +		return ERR_CAST(fwnode);
> +
> +	return to_swnode(fwnode);
> +}

[Severity: Medium]
As reported by Sashiko on v1 [1], what happens when a named static
software_node (where node->name is non-NULL) or a parent-child software_node
group is removed and re-registered before kobject_cleanup() runs?

When the last reference to an swnode is dropped via swnode_put() ->
kobject_put(), __kobject_del() (which removes the sysfs directory),
software_node_release() (which unlinks swnode->entry from parent->children),
and kobject_put(parent) only run inside kobject_cleanup(), which is delayed
on a workqueue when CONFIG_DEBUG_KOBJECT_RELEASE is enabled.

Because software_node_to_swnode_get() now skips the zero-ref swnode on
swnode_kset->list and returns NULL, device_add_software_node() and
software_node_register() proceed into software_node_register_node() ->
swnode_register() while the dying swnode is still present:

1. If node->name is non-NULL, swnode_register() calls:

	if (node->name)
		ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
					   parent ? &parent->kobj : NULL,
					   "%s", node->name);

while the dying swnode's sysfs directory with the same name has not been
removed by __kobject_del() yet, causing kobject_init_and_add() to warn and
fail with -EEXIST.

2. If a parent-child node group is unregistered via
software_node_unregister_node_group(), the dying child kobject still holds
its reference on parent->kobj until the child's kobject_cleanup() runs.
During that window, software_node_to_swnode_get() on the parent node still
succeeds and software_node_register_node() returns -EEXIST when attempting to
re-register the parent.

3. If an unnamed child node (node->name is NULL and node->parent is non-NULL)
is unregistered and re-registered under a live parent, swnode_register() runs:

	if (parent)
		list_add_tail(&swnode->entry, &parent->children);

while the dying child is still on parent->children waiting for
software_node_release() to run:

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

Can this race on parent->children without synchronization or fail with
-EEXIST on deferred probe retry?

[1] https://lore.kernel.org/r/20261104-swnode-race-v1-v1-1-9406a67e25be@nxp.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-swnode-race-v1-v2-0-f8bc7b14d578@nxp.com?part=1