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

[Rob Herring]

On Wed, Aug 05, 2026 at 01:31:01PM -0700, Abdurrahman Hussain wrote:
> of_find_node_opts_by_path() walks the property list of of_aliases
> without taking a reference on the node and passes pp->value straight
> to of_find_node_by_path().
> 
> Take a reference across the walk. The walk itself stays lock-free
> like every other property iteration: it can race property surgery and
> see a stale view (a removed property's ->next is repointed at the
> deadprops list), but nothing it can reach is freed while the node
> reference is held. devtree_lock covers only the pointer load,
> pairing it with a later patch in this series that clears of_aliases
> and drops its reference when the node is detached at runtime.
> 
> Validate the value before resolving it. of_alias_value_ok() requires
> a non-empty, NUL-terminated, absolute path:
> 
>   - an empty property has a NULL value and crashes in strchr()
>   - a value without a NUL inside the property is read past its end
>   - a relative value naming another alias (loop = "loop") recurses
>     through of_find_node_by_path() until the stack is exhausted
> 
> All three are reachable with a malformed boot FDT today.
> 
> The name comparison loses its redundant strlen() pass while here.
> 
> Assisted-by: Claude:claude-fable-5 [Claude Code]
> Signed-off-by: Abdurrahman Hussain <abdurrahman@xxxxxxxxxx>
> ---
>  drivers/of/base.c       | 20 +++++++++++++++-----
>  drivers/of/of_private.h |  8 ++++++++
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 477017ed6f49..eca1f55eee87 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -995,6 +995,8 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>  
>  	/* The path could begin with an alias */
>  	if (*path != '/') {
> +		struct device_node *aliases;
> +		const char *value = NULL;
>  		int len;
>  		const char *p = strchrnul(path, '/');
>  
> @@ -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);

There's no need to take the spinlock for just a get. Furthermore, as 
long as the of_aliases pointer is exposed to the rest of the kernel, a 
reference should always be held. Not that you should rely on that 
here...

Rob