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

[Abdurrahman Hussain]

On Thu Aug 27, 2026 at 7:52 AM PDT, Rob Herring wrote:
> 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

Agreed, dropped the lock for v7 (here and around the of_aliases
updates in the notifier patch, which are already serialized by
aliases_mutex).

Since patch 1 is applied, I'll drop it and rebase the rest on dt/next
unless you prefer otherwise.

Thanks,
Abdurrahman