Re: [PATCH v6 02/10] of: hold a reference on of_aliases during alias path resolution

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Calling of_node_put() while holding devtree_lock (a raw spinlock) can cause a sleep in atomic context.
--

--- Patch [2]: [PATCH v6 02/10] of: hold a reference on of_aliases during alias path resolution ---
commit 0b9ece03ae6938d97128143d8a080acc486d86ce
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

of: hold a reference on of_aliases during alias path resolution

The commit modifies of_find_node_opts_by_path() to take a reference on
of_aliases while iterating over its properties for alias resolution. It also
adds validation via of_alias_value_ok() to ensure the alias property points
to a valid absolute path, preventing crashes from malformed FDTs.

> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 477017ed6f49b..eca1f55eee872 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1002,16 +1004,24 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>  			p = separator;
>  		len = p - path;
>  
> -		/* of_aliases must not be NULL */
> -		if (!of_aliases)
> +		/* the load pairs with writers that retire the node */
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
> +		aliases = of_node_get(of_aliases);
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +		if (!aliases)
>  			return NULL;
>  
> -		for_each_property_of_node(of_aliases, pp) {
> -			if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
> -				np = of_find_node_by_path(pp->value);
> +		for_each_property_of_node(aliases, pp) {
> +			if (!strncmp(pp->name, path, len) && !pp->name[len]) {
> +				if (of_alias_value_ok(pp))
> +					value = pp->value;
>  				break;
>  			}
>  		}
> +		/* the reference on @aliases keeps @value alive */
> +		if (value)
> +			np = of_find_node_by_path(value);
> +		of_node_put(aliases);
>  		if (!np)
>  			return NULL;
>  		path = p;

[Severity: High]
This isn't a bug introduced by this patch, but does the code just below this
block safely handle reference drops while holding a raw spinlock?

In of_find_node_opts_by_path(), shortly after this diff, we do:

	raw_spin_lock_irqsave(&devtree_lock, flags);
	if (!np)
		np = of_node_get(of_root);
	np = __of_find_node_by_full_path(np, path);
	raw_spin_unlock_irqrestore(&devtree_lock, flags);

Inside __of_find_node_by_full_path(), there is a call to of_node_put():

		node = __of_find_node_by_path(node, path);
		of_node_put(tmp);

If this node was dynamically detached (e.g., via overlay removal), could this
of_node_put() call be dropping the final reference?

If the refcount drops to zero here, of_node_put() calls of_node_release().
This then calls fwnode_links_purge() which acquires fwnode_link_lock.

Since fwnode_link_lock is a sleepable mutex, can acquiring it while holding
devtree_lock (a raw spinlock with interrupts disabled) result in a sleep in
atomic context?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-nh-of-alias-overlay-v6-0-74f21d440819@nexthop.ai?part=2