Re: [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Data race and possible list corruption because `of_device_uevent()` iterates over `aliases_lookup` while holding `of_mutex` instead of the newly introduced `aliases_mutex`.
- [High] Lockless iteration of `aliases->properties` in `of_alias_node_scan()` and `of_alias_node_forget()` can race with concurrent property updates.
- [Medium] Missing `of_node_put()` leaks target device node references on error paths in `of_alias_create()` and during overlay updates of boot-time aliases in `of_alias_destroy()`.
--

--- Patch [1]: [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications ---
commit 0e641f78493b9291f518b7ebe305e2c1cbac0e2b
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

of: incrementally update /aliases lookup on reconfig notifications
    
/aliases entries added by a device-tree overlay are stored in the live
tree but never enter the global aliases_lookup list that of_alias_scan()
builds at boot. As a result, of_alias_get_id() returns -ENODEV for
aliases declared inside overlays, and any driver that relies on
alias-based numbering (i2c-xiic, spi, tty, mmc, ...) silently loses its
pinned id and falls back to auto-assignment.

Link: https://lore.kernel.org/lkml/1435675876-2159-1-git-send-email-geert+renesas@glider.be/ [1]
Link: https://lore.kernel.org/lkml/20150630172131.D4E6CC4041A@trevor.secretlab.ca/ [2]
Link: https://lore.kernel.org/lkml/1435675876-2159-2-git-send-email-geert+renesas@glider.be/ [3]
Link: https://lore.kernel.org/lkml/1435675876-2159-3-git-send-email-geert+renesas@glider.be/ [4]
Link: https://lore.kernel.org/lkml/1435675876-2159-4-git-send-email-geert+renesas@glider.be/ [5]

> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 6e7a42dedad3f..2695c5f8bb93a 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1915,6 +1915,231 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
>  		 ap->alias, ap->stem, ap->id, np);
>  }
>  
> +/*
> + * Serializes aliases_lookup and of_aliases across boot-time scan,
> + * runtime notifier updates, and readers. of_alias_get_id() and
> + * of_alias_get_highest_id() acquire this before walking the list; the
> + * OF reconfig notifier below acquires it around each mutation.
> + * of_alias_scan() runs single-threaded from of_core_init() and skips
> + * the lock, but any code path that reads or writes aliases_lookup
> + * outside init must hold it.
> + */
> +static DEFINE_MUTEX(aliases_mutex);

[Severity: High]
Does this new mutex also need to be acquired in of_device_uevent()?

Looking at of_device_uevent() in drivers/of/device.c, it still iterates over 
aliases_lookup using of_mutex:

drivers/of/device.c:of_device_uevent() {
    ...
	mutex_lock(&of_mutex);
	list_for_each_entry(app, &aliases_lookup, link) {
		if (dev->of_node == app->np) {
    ...
	}
	mutex_unlock(&of_mutex);
}

Since of_aliases_reconfig_notifier() modifies the list while holding only 
aliases_mutex, could this result in concurrent modification and list corruption 
if a uevent is triggered during an overlay update?

[ ... ]

> +	if (kstrtoint(end, 10, &id) < 0)
> +		goto out_put;
> +
> +	ap = dt_alloc(sizeof(*ap) + len + 1, __alignof__(*ap));
> +	if (!ap)
> +		goto out_put;
> +	memset(ap, 0, sizeof(*ap) + len + 1);
> +
> +	/*
> +	 * For runtime entries, kstrdup the alias name so the alias_prop
> +	 * doesn't depend on pp->name remaining valid — an overlay revert
> +	 * frees the source property. Boot-time entries point into the
> +	 * FDT, which is never freed.
> +	 */
> +	if (owned) {
> +		dup = kstrdup(pp->name, GFP_KERNEL);
> +		if (!dup) {
> +			kfree(ap);
> +			goto out_put;
> +		}
> +	} else {
> +		dup = start;
> +	}
> +	ap->alias = dup;
> +	ap->owned = owned;
> +	of_alias_add(ap, np, id, start, len);
> +	return;
> +
> +out_put:
> +	if (owned)
> +		of_node_put(np);
> +}

[Severity: Medium]
Does this leak the device node reference for boot-time aliases on error?

When of_find_node_by_path() is called earlier in this function, it acquires a 
reference to the returned node regardless of whether the alias is owned or not.
If an error occurs (like kstrtoint() or dt_alloc() failing) for a boot-time 
alias where owned is false, the out_put label is reached but of_node_put(np) 
is skipped, leaking the reference.

> +/*
> + * Reverse of of_alias_create() for owned entries: unlink and free the
> + * matching alias_prop and drop the reference it holds on the target
> + * node. For boot-time entries (owned=false) it unlinks only — the
> + * struct and the alias name live in memblock and are never freed —
> + * which prevents an overlay-driven UPDATE_PROPERTY against a boot-time
> + * alias from leaving stale duplicates in aliases_lookup.
> + *
> + * Callers must hold @aliases_mutex.
> + */
> +static void of_alias_destroy(const char *name)
> +{
> +	struct alias_prop *ap, *tmp;
> +
> +	list_for_each_entry_safe(ap, tmp, &aliases_lookup, link) {
> +		if (strcmp(ap->alias, name) != 0)
> +			continue;
> +		list_del(&ap->link);
> +		if (ap->owned) {
> +			of_node_put(ap->np);
> +			kfree(ap->alias);
> +			kfree(ap);
> +		}
> +		return;
> +	}
> +}

[Severity: Medium]
Does this also leak the device node reference if a boot-time alias is updated
or removed?

If an overlay updates a boot-time alias (where ap->owned is false), the alias 
is unlinked from the list, but of_node_put(ap->np) is skipped. Since the 
reference was acquired at boot time by of_find_node_by_path(), doesn't it 
need to be released when the alias is destroyed?

> +static void *alias_alloc(u64 size, u64 align)
> +{
> +	return kzalloc(size, GFP_KERNEL);
> +}
> +
> +/* Scan every property of @aliases and mirror it into aliases_lookup. */
> +static void of_alias_node_scan(struct device_node *aliases)
> +{
> +	struct property *pp;
> +
> +	for_each_property_of_node(aliases, pp)
> +		of_alias_create(pp, alias_alloc, true);
> +}
> +
> +/* Reverse: destroy every alias_prop backed by a property of @aliases. */
> +static void of_alias_node_forget(struct device_node *aliases)
> +{
> +	struct property *pp;
> +
> +	for_each_property_of_node(aliases, pp)
> +		of_alias_destroy(pp->name);
> +}

[Severity: High]
Is it safe to iterate aliases->properties without holding devtree_lock?

These functions use for_each_property_of_node() to traverse the properties list 
locklessly. Since of_aliases_reconfig_notifier() is called for ATTACH_NODE 
and DETACH_NODE events, could a concurrent thread adding or removing properties 
on the aliases node mutate the linked list under devtree_lock, causing a 
race condition or traversing stale pointers here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-nh-of-alias-overlay-v1-0-27da6848dd84@nexthop.ai?part=1