Re: [PATCH v5 1/9] of: hold a reference on of_aliases during alias path resolution

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Calling of_node_put() while holding a raw spinlock causes a sleep-in-atomic bug if it drops the last reference to a dynamic node.
- [High] Lockless iteration of device tree properties is unsafe because updated or removed properties have their `next` pointer modified, which derails concurrent readers.
--

--- Patch [1]: [PATCH v5 1/9] of: hold a reference on of_aliases during alias path resolution ---
commit aa90279de0a9f76f84e2b40b29743f11984c3ffb
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Subject: of: hold a reference on of_aliases during alias path resolution

This patch modifies of_find_node_opts_by_path to hold a reference to
of_aliases during alias path resolution. It also validates the alias value
to prevent crashes or infinite recursion with malformed device trees.

> Take a reference across the walk. The walk itself stays lock-free
> like every other property iteration: removed properties keep their
> ->next and are only freed when the node is released, so the node
> reference is sufficient.

[Severity: High]
This isn't a bug introduced by this patch, but is the description of the
lock-free walk accurate?

Looking at __of_remove_property() and __of_update_property(), when a
property is removed, it is moved to the node's deadprops list:

drivers/of/base.c:__of_remove_property() {
    ...
    /* Found the property, add it to deadprops list */
    prop->next = np->deadprops;
    np->deadprops = prop;
    ...
}

If a concurrent writer (e.g., overlay application) removes a property while
for_each_property_of_node() is iterating over it, won't the reader's next
access of pp->next follow the pointer into the deadprops list? 

This would cause the reader to traverse the dead properties and skip the
remaining valid properties in np->properties, potentially causing spurious
lookup failures.

> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 6e7a42dedad3f..6f79f593fc779 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 is a pre-existing issue, but does calling __of_find_node_by_full_path()
while holding devtree_lock further down in this function risk a
sleep-in-atomic bug?

Right after this block in of_find_node_opts_by_path(), devtree_lock (which is
a raw spinlock) is acquired around __of_find_node_by_full_path():

drivers/of/base.c:of_find_node_opts_by_path() {
    ...
    /* Step down the tree matching path components */
    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(), it iteratively drops node references:

drivers/of/base.c:__of_find_node_by_full_path() {
    ...
    node = __of_find_node_by_path(node, path);
    of_node_put(tmp);
    ...
}

If a parallel thread (such as an overlay removal) detaches a dynamic node,
its primary tree reference is dropped.

If of_node_put() drops the last reference to that node while held under the
raw spinlock, won't it synchronously invoke of_node_release()?

For dynamic nodes, of_node_release() calls fwnode_links_purge(), which
acquires fwnode_link_lock (a sleeping mutex), resulting in a sleep-in-atomic
scenario.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-nh-of-alias-overlay-v5-0-2abe2bb9cdbc@nexthop.ai?part=1