* [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications
2026-07-21 2:52 [PATCH RFC 0/4] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
@ 2026-07-21 2:52 ` Abdurrahman Hussain
2026-07-21 3:04 ` sashiko-bot
2026-07-21 2:52 ` [PATCH RFC 2/4] of/overlay: look up absolute target-paths absolutely Abdurrahman Hussain
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Abdurrahman Hussain @ 2026-07-21 2:52 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan
Cc: devicetree, linux-kernel, Abdurrahman Hussain
/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.
Fix by registering an internal OF reconfig notifier from
of_core_init() that mirrors /aliases changes into aliases_lookup:
OF_RECONFIG_ADD_PROPERTY -> of_alias_create()
OF_RECONFIG_REMOVE_PROPERTY -> of_alias_destroy()
OF_RECONFIG_UPDATE_PROPERTY -> destroy + create
OF_RECONFIG_ATTACH_NODE -> scan all properties (defensive)
OF_RECONFIG_DETACH_NODE -> forget all properties (defensive)
The reconfig notifier chain fires from both direct changesets and
overlay apply/revert, so the same code path covers runtime dt
modifications and overlay-declared aliases without any overlay-
specific hook in drivers/of/overlay.c. Grant Likely suggested this
shape on Geert Uytterhoeven's 2015 RFC [1]; Geert's original hook was
in dynamic.c directly.
Match the /aliases target node structurally (name == "aliases" and
parent == root) rather than by pointer against the of_aliases global.
A system with no boot-time /aliases has of_aliases == NULL, so an
overlay that creates /aliases from scratch would otherwise be missed
from the first ATTACH_NODE onward. ATTACH/DETACH also update
of_aliases lazily (with of_node_get/put) so subsequent consumers see
it.
Overlays build the node up empty and add properties via separate
ADD_PROPERTY events; other callers of of_attach_node() are allowed to
attach a fully-populated node in one shot. Handle both shapes by
scanning the node's properties on ATTACH_NODE and forgetting them on
DETACH_NODE. Overlay-driven flows still receive per-property events
and remain correct.
Factor the per-property loop body of of_alias_scan() into
of_alias_create() so the boot-time scan and the runtime notifier
share one code path. Owned (runtime) entries kstrdup the alias name
and hold an of_node_get() reference on the target so the alias_prop
survives the property that spawned it — required for the overlay
revert path where the source property is freed before we get a chance
to see it drop. A one-bit @owned flag on struct alias_prop distinguishes
kmalloc'd entries from memblock-backed ones so the destroy path
kfree()s the right ones.
The destroy path unlinks matching entries regardless of ownership
(freeing storage only for owned ones) so an overlay UPDATE against a
boot-time alias leaves at most one entry per stem+id. This addresses
the allocator-mismatch worry Grant flagged on the 2015 series [2] and
also the duplicate-mapping side effect that would otherwise leak
through.
Serialize aliases_lookup on a dedicated aliases_mutex: readers
(of_alias_get_id, of_alias_get_highest_id) and the reconfig notifier
both hold it around every access. Boot-time of_alias_scan() runs
single-threaded during init and stays lockless. This is preferable to
piggy-backing on of_mutex because the reconfig notifier is called
both under of_mutex (overlay apply path) and outside of it (direct
of_add_property() path from dynamic.c), so a nested acquisition would
deadlock on some callers.
Validate the property value before feeding it to of_find_node_by_path():
pp->value must be non-empty and null-terminated within pp->length. An
overlay that hasn't been through /aliases fixup can otherwise present
a fragment-internal string that isn't a valid live-tree path or a
malformed non-terminated value, and of_find_node_by_path() derefs it
as a C string — an OOB read on the malformed case.
Naming builds on Geert's original series:
- "of: Extract of_alias_create()" [3]
- "of: Add of_alias_destroy()" [4]
- "of/dynamic: Update list of aliases on aliases changes" [5]
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]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/of/base.c | 271 +++++++++++++++++++++++++++++++++++++++++-------
drivers/of/of_private.h | 7 ++
2 files changed, 238 insertions(+), 40 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 6e7a42dedad3..2695c5f8bb93 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);
+
+/*
+ * Build an alias_prop for @pp using @dt_alloc as the storage allocator
+ * and add it to aliases_lookup. @owned is stored on the entry so the
+ * matching destroy path knows whether the alias_prop is a kmalloc'd
+ * struct that must be kfree()d (with a paired of_node_put on the
+ * target and kfree on the kstrdup'd alias name) or a memblock/
+ * dt_alloc'd struct that must be left alone.
+ *
+ * Callers other than of_alias_scan() must hold @aliases_mutex.
+ *
+ * Skips pseudo-properties (name, phandle, ...), malformed property
+ * values (empty or not null-terminated within pp->length), and alias
+ * names not ending in a numeric id.
+ */
+static void of_alias_create(const struct property *pp,
+ void *(*dt_alloc)(u64 size, u64 align),
+ bool owned)
+{
+ const char *start = pp->name;
+ const char *end;
+ struct device_node *np;
+ struct alias_prop *ap;
+ const char *dup;
+ int id, len;
+
+ if (is_pseudo_property(pp->name))
+ return;
+
+ /*
+ * pp->value must be a non-empty, null-terminated string within
+ * pp->length. of_find_node_by_path() derefs it as a C string, so
+ * a missing terminator would cause an OOB read.
+ */
+ if (!pp->value || pp->length < 2 ||
+ strnlen(pp->value, pp->length) >= pp->length)
+ return;
+
+ np = of_find_node_by_path(pp->value);
+ if (!np)
+ return;
+
+ end = start + strlen(start);
+ while (end > start && isdigit(*(end - 1)))
+ end--;
+ len = end - start;
+ if (len == 0)
+ goto out_put;
+
+ 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);
+}
+
+/*
+ * 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;
+ }
+}
+
+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);
+}
+
+/*
+ * OF reconfig notifier that mirrors /aliases property changes into
+ * aliases_lookup. Fires on both direct changesets and overlay
+ * apply/revert, so of_alias_get_id() returns the right id for aliases
+ * declared inside an overlay.
+ */
+static int of_aliases_reconfig_notifier(struct notifier_block *nb,
+ unsigned long action, void *arg)
+{
+ struct of_reconfig_data *rd = arg;
+
+ /*
+ * Match /aliases structurally (name + root-parent) rather than by
+ * pointer against the of_aliases global — a system with no
+ * boot-time /aliases (of_aliases == NULL) can still acquire one
+ * from an overlay, and we must track its properties from the
+ * first ATTACH_NODE onward.
+ */
+ if (!rd->dn || !rd->dn->parent ||
+ !of_node_is_root(rd->dn->parent) ||
+ !of_node_name_eq(rd->dn, "aliases"))
+ return NOTIFY_DONE;
+
+ mutex_lock(&aliases_mutex);
+ switch (action) {
+ case OF_RECONFIG_ATTACH_NODE:
+ /*
+ * Overlays build the node up empty and add properties via
+ * separate ADD_PROPERTY events, but the reconfig API also
+ * permits attaching a fully-populated node in one shot —
+ * scan defensively so pre-populated aliases aren't lost.
+ */
+ of_alias_node_scan(rd->dn);
+ if (!of_aliases)
+ of_aliases = of_node_get(rd->dn);
+ break;
+ case OF_RECONFIG_DETACH_NODE:
+ /*
+ * Symmetric with ATTACH_NODE: some callers detach without
+ * emitting per-property REMOVE events first, so drop every
+ * alias_prop backed by this node's properties before it
+ * disappears.
+ */
+ of_alias_node_forget(rd->dn);
+ if (of_aliases == rd->dn) {
+ of_node_put(of_aliases);
+ of_aliases = NULL;
+ }
+ break;
+ case OF_RECONFIG_ADD_PROPERTY:
+ of_alias_create(rd->prop, alias_alloc, true);
+ break;
+ case OF_RECONFIG_REMOVE_PROPERTY:
+ of_alias_destroy(rd->prop->name);
+ break;
+ case OF_RECONFIG_UPDATE_PROPERTY:
+ if (rd->old_prop)
+ of_alias_destroy(rd->old_prop->name);
+ of_alias_create(rd->prop, alias_alloc, true);
+ break;
+ default:
+ break;
+ }
+ mutex_unlock(&aliases_mutex);
+ return NOTIFY_OK;
+}
+
+static struct notifier_block of_aliases_nb = {
+ .notifier_call = of_aliases_reconfig_notifier,
+};
+
+static int __init of_aliases_reconfig_init(void)
+{
+ return of_reconfig_notifier_register(&of_aliases_nb);
+}
+
+/*
+ * of_alias_scan() runs from of_core_init() (core_initcall), so hook the
+ * reconfig notifier one initcall level later to guarantee the initial
+ * static scan is complete before any dynamic tracking begins.
+ */
+core_initcall_sync(of_aliases_reconfig_init);
+
/**
* of_alias_scan - Scan all properties of the 'aliases' node
* @dt_alloc: An allocator that provides a virtual address to memory
@@ -1950,42 +2175,8 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
if (!of_aliases)
return;
- for_each_property_of_node(of_aliases, pp) {
- const char *start = pp->name;
- const char *end = start + strlen(start);
- struct device_node *np;
- struct alias_prop *ap;
- int id, len;
-
- /* Skip those we do not want to proceed */
- if (is_pseudo_property(pp->name))
- continue;
-
- np = of_find_node_by_path(pp->value);
- if (!np)
- continue;
-
- /* walk the alias backwards to extract the id and work out
- * the 'stem' string */
- while (isdigit(*(end-1)) && end > start)
- end--;
- len = end - start;
-
- if (kstrtoint(end, 10, &id) < 0) {
- of_node_put(np);
- continue;
- }
-
- /* Allocate an alias_prop with enough space for the stem */
- ap = dt_alloc(sizeof(*ap) + len + 1, __alignof__(*ap));
- if (!ap) {
- of_node_put(np);
- continue;
- }
- memset(ap, 0, sizeof(*ap) + len + 1);
- ap->alias = start;
- of_alias_add(ap, np, id, start, len);
- }
+ for_each_property_of_node(of_aliases, pp)
+ of_alias_create(pp, dt_alloc, false);
}
/**
@@ -2003,7 +2194,7 @@ int of_alias_get_id(const struct device_node *np, const char *stem)
struct alias_prop *app;
int id = -ENODEV;
- mutex_lock(&of_mutex);
+ mutex_lock(&aliases_mutex);
list_for_each_entry(app, &aliases_lookup, link) {
if (strcmp(app->stem, stem) != 0)
continue;
@@ -2013,7 +2204,7 @@ int of_alias_get_id(const struct device_node *np, const char *stem)
break;
}
}
- mutex_unlock(&of_mutex);
+ mutex_unlock(&aliases_mutex);
return id;
}
@@ -2031,7 +2222,7 @@ int of_alias_get_highest_id(const char *stem)
struct alias_prop *app;
int id = -ENODEV;
- mutex_lock(&of_mutex);
+ mutex_lock(&aliases_mutex);
list_for_each_entry(app, &aliases_lookup, link) {
if (strcmp(app->stem, stem) != 0)
continue;
@@ -2039,7 +2230,7 @@ int of_alias_get_highest_id(const char *stem)
if (app->id > id)
id = app->id;
}
- mutex_unlock(&of_mutex);
+ mutex_unlock(&aliases_mutex);
return id;
}
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 0ae16da066e2..9d16765ae2c3 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -17,6 +17,12 @@
* @alias: Alias property name
* @np: Pointer to device_node that the alias stands for
* @id: Index value from end of alias name
+ * @owned: True if @alias was kstrdup'd and @np was of_node_get'd on
+ * insertion (overlay-time entries). False for entries built
+ * by of_alias_scan() at boot, where @alias points into the
+ * FDT and @np is an unreferenced pointer. The removal path
+ * uses this flag to decide whether it must kfree(@alias),
+ * of_node_put(@np), and kfree(the struct itself).
* @stem: Alias string without the index
*
* The structure represents one alias property of 'aliases' node as
@@ -27,6 +33,7 @@ struct alias_prop {
const char *alias;
struct device_node *np;
int id;
+ bool owned;
char stem[];
};
--
2.54.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications
2026-07-21 2:52 ` [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications Abdurrahman Hussain
@ 2026-07-21 3:04 ` sashiko-bot
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-07-21 3:04 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: devicetree, conor+dt, robh
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH RFC 2/4] of/overlay: look up absolute target-paths absolutely
2026-07-21 2:52 [PATCH RFC 0/4] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
2026-07-21 2:52 ` [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications Abdurrahman Hussain
@ 2026-07-21 2:52 ` Abdurrahman Hussain
2026-07-21 2:52 ` [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths Abdurrahman Hussain
2026-07-21 2:52 ` [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert Abdurrahman Hussain
3 siblings, 0 replies; 11+ messages in thread
From: Abdurrahman Hussain @ 2026-07-21 2:52 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan
Cc: devicetree, linux-kernel, Abdurrahman Hussain
When of_overlay_fdt_apply() is called with a non-NULL target base,
find_target() currently concatenates the base's full path with every
fragment's target-path via "%pOF%s" — so target-path="" resolves to
the base itself (the intended common case), but target-path="/foo"
resolves to "<base>/foo" (never the DT root) and target-path="/" to
"<base>/" (never a valid node at all).
That makes it impossible for a two-fragment overlay to modify one
subtree under the base and one node at the DT root — a shape that
arises naturally when a PCI-attached device wants to declare its
peripherals under dev_of_node(&pdev->dev) AND add /aliases entries
so alias-aware drivers (i2c-xiic, spi, tty, ...) can pin bus numbers.
Treat target-path as absolute whenever it is non-empty. An empty
target-path continues to mean "the target base itself", preserving
the existing shape used by drivers/misc/lan966x_pci.c and its dtso
(the only in-tree of_overlay_fdt_apply() caller today that passes a
non-NULL base).
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/of/overlay.c | 34 ++++++++++++++++------------------
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 08d5351746be..654a70d5cb07 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -693,7 +693,6 @@ static struct device_node *find_target(const struct device_node *info_node,
const struct device_node *target_base)
{
struct device_node *node;
- char *target_path;
const char *path;
u32 val;
int ret;
@@ -709,23 +708,22 @@ static struct device_node *find_target(const struct device_node *info_node,
ret = of_property_read_string(info_node, "target-path", &path);
if (!ret) {
- if (target_base) {
- target_path = kasprintf(GFP_KERNEL, "%pOF%s", target_base, path);
- if (!target_path)
- return NULL;
- node = of_find_node_by_path(target_path);
- if (!node) {
- pr_err("find target, node: %pOF, path '%s' not found\n",
- info_node, target_path);
- }
- kfree(target_path);
- } else {
- node = of_find_node_by_path(path);
- if (!node) {
- pr_err("find target, node: %pOF, path '%s' not found\n",
- info_node, path);
- }
- }
+ /*
+ * With a non-NULL @target_base, an empty target-path means
+ * "the target base itself" — this is the common
+ * of_overlay_fdt_apply(..., base) form used by e.g. the
+ * LAN966x PCI overlay. Any other target-path is looked up
+ * absolutely, so overlays that also need to reach the DT
+ * root (e.g. to add /aliases entries alongside a base-
+ * relative fragment) can do so with target-path="/aliases".
+ */
+ if (target_base && path[0] == '\0')
+ return of_node_get((struct device_node *)target_base);
+
+ node = of_find_node_by_path(path);
+ if (!node)
+ pr_err("find target, node: %pOF, path '%s' not found\n",
+ info_node, path);
return node;
}
--
2.54.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths
2026-07-21 2:52 [PATCH RFC 0/4] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
2026-07-21 2:52 ` [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications Abdurrahman Hussain
2026-07-21 2:52 ` [PATCH RFC 2/4] of/overlay: look up absolute target-paths absolutely Abdurrahman Hussain
@ 2026-07-21 2:52 ` Abdurrahman Hussain
2026-07-21 3:06 ` sashiko-bot
2026-07-21 2:52 ` [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert Abdurrahman Hussain
3 siblings, 1 reply; 11+ messages in thread
From: Abdurrahman Hussain @ 2026-07-21 2:52 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan
Cc: devicetree, linux-kernel, Abdurrahman Hussain
/aliases entries added by an overlay reference labeled nodes inside
the overlay via '&label' in the .dtso. dtc renders those references
as string paths at compile time, but the paths encode the overlay's
internal fragment layout (e.g. "/fragment@1/__overlay__/fpga@0/i2c@40000")
rather than the location where the node will live after apply.
Currently only /__symbols__ has its property values rewritten from
overlay-internal paths to live-tree paths by dup_and_fixup_symbol_prop().
/aliases values fall through the plain __of_prop_dup() path and are
copied byte-for-byte, so of_find_node_by_path() on such a value returns
NULL, of_alias_get_id() reports -ENODEV — and the reconfig notifier
added earlier in this series sees uninterpretable paths and can't
populate aliases_lookup for overlay-declared aliases.
The values in /aliases follow the same textual convention as
/__symbols__, so we can reuse the existing rewriter. Detect the
/aliases target node and route its properties through
dup_and_fixup_symbol_prop() only when the value actually looks like a
fragment-internal path (non-empty, null-terminated within pp->length,
"/fragment@" prefix); other alias values — legacy string aliases like
"ttyS0" that some out-of-tree code writes verbatim — pass through the
plain __of_prop_dup() path unchanged.
Discriminating up-front rather than falling back on a NULL return
matters because dup_and_fixup_symbol_prop() also returns NULL on
allocation failure and on malformed values. A blanket fallback would
mask -ENOMEM as "not our shape" and, worse, would copy a
non-null-terminated value byte-for-byte into the live tree — where
the alias reconfig notifier's downstream of_find_node_by_path() would
treat it as a C string and read past the end. Structural validation
here means a NULL from dup_and_fixup_symbol_prop() below can only be
-ENOMEM and is propagated as such.
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/of/overlay.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 654a70d5cb07..69358811a7f9 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -350,6 +350,33 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
if (prop)
return -EINVAL;
new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
+ } else if (target->np->parent &&
+ of_node_is_root(target->np->parent) &&
+ of_node_name_eq(target->np, "aliases") &&
+ overlay_prop->length >= sizeof("/fragment@") &&
+ overlay_prop->value &&
+ strnlen(overlay_prop->value, overlay_prop->length) <
+ overlay_prop->length &&
+ !strncmp(overlay_prop->value, "/fragment@",
+ strlen("/fragment@"))) {
+ /*
+ * /aliases property values that reference a labeled node
+ * inside this overlay are rendered by dtc as string paths
+ * "/fragment@N/__overlay__/..." — the overlay's internal
+ * layout rather than where the node lives after apply.
+ * Reuse dup_and_fixup_symbol_prop() (which already handles
+ * this rewrite for /__symbols__) so of_alias_get_id() can
+ * resolve the value in the live tree.
+ *
+ * The property-value shape (non-empty, null-terminated
+ * within pp->length, prefix "/fragment@") is validated
+ * above so a NULL return from dup_and_fixup_symbol_prop()
+ * here means -ENOMEM, not "not our shape"; let the shared
+ * -ENOMEM check below handle it rather than falling back
+ * to a raw dup that would inject an unresolvable path
+ * into the live tree.
+ */
+ new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
} else {
new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
}
--
2.54.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths
2026-07-21 2:52 ` [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths Abdurrahman Hussain
@ 2026-07-21 3:06 ` sashiko-bot
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-07-21 3:06 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: robh, devicetree, conor+dt
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [Medium] A structural validation failure in `dup_and_fixup_symbol_prop()` is incorrectly masked as a memory allocation failure.
Pre-existing issues:
- [High] The `__of_prop_dup` fallback permits non-null-terminated alias strings to enter the live tree, causing a Slab Out-Of-Bounds Read.
- [High] Double Free / Use-After-Free of `new_prop` on memory allocation failure.
- [High] Memory leak of successfully processed device nodes on early exit in `init_overlay_changeset()`.
--
--- Patch [3]: [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths ---
commit ac88ca3a222d2b48e52178c676efb92de0cef66b
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Subject: of/overlay: rewrite /aliases path values to live-tree paths
The commit rewrites /aliases entries added by an overlay to use live-tree
paths instead of overlay-internal fragment paths. It checks if the alias
value starts with /fragment@ and uses dup_and_fixup_symbol_prop() to fix
it up. Other alias values fall through to __of_prop_dup().
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 654a70d5cb075..69358811a7f94 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -350,6 +350,33 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> if (prop)
> return -EINVAL;
> new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
> + } else if (target->np->parent &&
> + of_node_is_root(target->np->parent) &&
> + of_node_name_eq(target->np, "aliases") &&
> + overlay_prop->length >= sizeof("/fragment@") &&
> + overlay_prop->value &&
> + strnlen(overlay_prop->value, overlay_prop->length) <
> + overlay_prop->length &&
> + !strncmp(overlay_prop->value, "/fragment@",
> + strlen("/fragment@"))) {
[Severity: High]
This isn't a bug introduced by this patch, but does this logic still allow
non-null-terminated strings to enter the live tree?
If an overlay provides an /aliases property with a non-null-terminated
string that does not start with /fragment@ (e.g., ttyS0 missing a null byte),
the else if condition evaluates to false.
Execution falls through to the else block:
drivers/of/overlay.c:add_changeset_property() {
...
} else {
new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
}
...
}
Here __of_prop_dup() blindly copies the non-null-terminated bytes directly
into the live device tree. When a caller such as of_find_node_opts_by_path()
attempts to resolve the alias, it passes pp->value to of_find_node_by_path(),
which uses standard C string functions (strncmp, strchr) on the buffer,
reading past the end.
> + /*
> + * /aliases property values that reference a labeled node
> + * inside this overlay are rendered by dtc as string paths
> + * "/fragment@N/__overlay__/..." — the overlay's internal
> + * layout rather than where the node lives after apply.
> + * Reuse dup_and_fixup_symbol_prop() (which already handles
> + * this rewrite for /__symbols__) so of_alias_get_id() can
> + * resolve the value in the live tree.
> + *
> + * The property-value shape (non-empty, null-terminated
> + * within pp->length, prefix "/fragment@") is validated
> + * above so a NULL return from dup_and_fixup_symbol_prop()
> + * here means -ENOMEM, not "not our shape"; let the shared
> + * -ENOMEM check below handle it rather than falling back
> + * to a raw dup that would inject an unresolvable path
> + * into the live tree.
> + */
> + new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
[Severity: Medium]
Does this incorrectly mask a structural validation failure as a memory
allocation failure?
When an overlay provides a structurally valid /aliases property that starts
with /fragment@ and is null-terminated, but references a non-existent
overlay fragment, dup_and_fixup_symbol_prop() will legitimately return NULL:
drivers/of/overlay.c:dup_and_fixup_symbol_prop() {
...
if (k >= ovcs->count)
return NULL;
...
}
The shared error handler below then incorrectly converts this structural
failure into a memory exhaustion error (-ENOMEM).
> } else {
> new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
> }
[Severity: High]
This isn't a bug introduced by this patch, but can this cause a double free
of new_prop on memory allocation failure?
If !target->in_livetree, new_prop is linked into target->np->deadprops:
drivers/of/overlay.c:add_changeset_property() {
...
if (!prop) {
if (!target->in_livetree) {
new_prop->next = target->np->deadprops;
target->np->deadprops = new_prop;
}
ret = of_changeset_add_property(&ovcs->cset, target->np,
new_prop);
}
...
}
If of_changeset_add_property() returns an error, the function explicitly
calls __of_prop_free(new_prop):
drivers/of/overlay.c:add_changeset_property() {
...
if (ret)
__of_prop_free(new_prop);
return ret;
}
Since new_prop was not unlinked from the deadprops list, when the overlay
application aborts and of_node_release() iterates over deadprops, will it
call __of_prop_free() again on the already-freed pointer?
[ ... ]
[Severity: High]
This isn't a bug introduced by this patch, but is there a memory leak of
successfully processed device nodes on early exit in
init_overlay_changeset()?
When applying a multi-fragment device tree overlay, if an intermediate
fragment fails find_target() (e.g., due to an invalid target-path), the
code jumps to err_out:
drivers/of/overlay.c:init_overlay_changeset() {
...
fragment->target = find_target(node, target_base);
if (!fragment->target) {
of_node_put(fragment->overlay);
ret = -EINVAL;
of_node_put(node);
goto err_out;
}
...
}
As the function iterates over the fragments, it increments the reference
counts for both the overlay and target device nodes, storing them in the
fragments array. When find_target() fails for a subsequent fragment, the
function exits with an error without updating ovcs->count (which remains 0).
The cleanup function free_overlay_changeset() relies on ovcs->count to loop
through and release the node references. Because the count is 0, the cleanup
loop is skipped entirely, causing a permanent leak of the successfully
processed node references from the prior fragments.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-nh-of-alias-overlay-v1-0-27da6848dd84@nexthop.ai?part=3
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert
2026-07-21 2:52 [PATCH RFC 0/4] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
` (2 preceding siblings ...)
2026-07-21 2:52 ` [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths Abdurrahman Hussain
@ 2026-07-21 2:52 ` Abdurrahman Hussain
2026-07-21 2:58 ` sashiko-bot
3 siblings, 1 reply; 11+ messages in thread
From: Abdurrahman Hussain @ 2026-07-21 2:52 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan
Cc: devicetree, linux-kernel, Abdurrahman Hussain
Add overlay_alias.dtso plus of_unittest_overlay_alias() to cover the
"aliases inside an overlay" flow end-to-end.
The overlay has two fragments:
fragment@0: target-path="" grafts a labeled node under target_base.
fragment@1: target-path="/aliases" adds `testcase-alias99 =
&<label>` at DT-root /aliases.
The runner applies the overlay with a non-NULL target_base via
of_overlay_fdt_apply() directly (rather than the overlay_data_apply()
wrapper, which always passes NULL), then asserts of_alias_get_id() on
the grafted node returns 99 across apply and -ENODEV across revert.
Exercises all three functional patches earlier in this series
together:
* the reconfig notifier is what mutates aliases_lookup on
apply/revert;
* find_target()'s absolute-target-path handling is what lets
fragment@1's target-path="/aliases" reach the DT root when
target_base is non-NULL;
* dup_and_fixup_symbol_prop()'s /aliases branch is what rewrites
the fragment-internal alias value ("&<label>" -> fragment path)
into the live-tree path that of_find_node_by_path() can resolve.
Any one of the three missing turns the middle assertion (get_id -> 99)
into -ENODEV.
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/of/unittest-data/Makefile | 2 +
drivers/of/unittest-data/overlay_alias.dtso | 36 ++++++++++++++
drivers/of/unittest.c | 74 +++++++++++++++++++++++++++++
3 files changed, 112 insertions(+)
diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 01a966e39f23..0a8bd9a74283 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.dtbo.o \
overlay_18.dtbo.o \
overlay_19.dtbo.o \
overlay_20.dtbo.o \
+ overlay_alias.dtbo.o \
overlay_bad_add_dup_node.dtbo.o \
overlay_bad_add_dup_prop.dtbo.o \
overlay_bad_phandle.dtbo.o \
@@ -87,6 +88,7 @@ apply_static_overlay_1 := overlay_0.dtbo \
overlay_18.dtbo \
overlay_19.dtbo \
overlay_20.dtbo \
+ overlay_alias.dtbo \
overlay_gpio_01.dtbo \
overlay_gpio_02a.dtbo \
overlay_gpio_02b.dtbo \
diff --git a/drivers/of/unittest-data/overlay_alias.dtso b/drivers/of/unittest-data/overlay_alias.dtso
new file mode 100644
index 000000000000..7763fffa8699
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_alias.dtso
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+/*
+ * overlay_alias - exercise the full "aliases inside an overlay" path.
+ *
+ * fragment@0 grafts a labeled node under target_base (empty target-path
+ * means "the target base itself"). fragment@1 adds a numbered alias at
+ * the DT-root /aliases and points it at that label. target-path="/aliases"
+ * is an absolute path — reaching it while target_base is non-NULL
+ * exercises the absolute-target-paths handling in find_target(). The
+ * alias value is emitted by dtc as the fragment-internal path
+ * "/fragment@0/__overlay__/alias-target"; the overlay code's
+ * /aliases path rewrite is what makes it resolve in the live tree.
+ * The reconfig notifier is what mirrors the change into aliases_lookup.
+ */
+
+/ {
+ fragment@0 {
+ target-path = "";
+ __overlay__ {
+ testcase-target: alias-target {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+ };
+ };
+
+ fragment@1 {
+ target-path = "/aliases";
+ __overlay__ {
+ testcase-alias99 = &testcase-target;
+ };
+ };
+};
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e255f54f4d76..35d75f2b3ef2 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -3475,6 +3475,76 @@ static struct notifier_block of_nb = {
.notifier_call = of_notify,
};
+static void __init of_unittest_overlay_alias(void)
+{
+ const char *base_path =
+ "/testcase-data/overlay-node/test-bus/test-unittest100";
+ const char *target_path =
+ "/testcase-data/overlay-node/test-bus/test-unittest100/alias-target";
+ struct device_node *base, *target;
+ struct overlay_info *info;
+ int ovcs_id = 0;
+ int id, ret;
+ u32 size;
+
+ /*
+ * Apply the overlay with a non-NULL target_base so that
+ * fragment@1's target-path="/aliases" is looked up absolutely
+ * (exercising find_target()'s absolute-path handling) rather
+ * than under the base. The alias value inside the overlay is a
+ * fragment-internal path that only resolves after the overlay
+ * code rewrites /aliases values, so this run covers the whole
+ * chain end-to-end.
+ */
+ base = of_find_node_by_path(base_path);
+ if (!base) {
+ unittest(0, "could not find alias target base %s\n", base_path);
+ return;
+ }
+
+ for (info = overlays; info && info->name; info++)
+ if (!strcmp("overlay_alias", info->name))
+ break;
+ if (!info || !info->name) {
+ unittest(0, "no overlay data for overlay_alias\n");
+ goto out_put_base;
+ }
+
+ size = info->dtbo_end - info->dtbo_begin;
+ ret = of_overlay_fdt_apply(info->dtbo_begin, size, &ovcs_id, base);
+ if (ret < 0) {
+ unittest(0, "overlay_alias apply failed, ret = %d\n", ret);
+ goto out_put_base;
+ }
+
+ target = of_find_node_by_path(target_path);
+ if (!target) {
+ unittest(0, "could not find grafted target %s\n", target_path);
+ goto out_remove;
+ }
+
+ id = of_alias_get_id(target, "testcase-alias");
+ unittest(id == 99,
+ "of_alias_get_id() = %d after overlay apply, expected 99\n",
+ id);
+ of_node_put(target);
+
+out_remove:
+ ret = of_overlay_remove(&ovcs_id);
+ if (ret) {
+ unittest(0, "overlay_alias remove failed, ret = %d\n", ret);
+ goto out_put_base;
+ }
+
+ id = of_alias_get_id(base, "testcase-alias");
+ unittest(id == -ENODEV,
+ "of_alias_get_id() = %d after overlay remove, expected -ENODEV\n",
+ id);
+
+out_put_base:
+ of_node_put(base);
+}
+
static void __init of_unittest_overlay_notify(void)
{
int ovcs_id;
@@ -3649,6 +3719,8 @@ static void __init of_unittest_overlay(void)
of_unittest_overlay_gpio();
+ of_unittest_overlay_alias();
+
of_unittest_remove_tracked_overlays();
of_unittest_overlay_notify();
@@ -3848,6 +3920,7 @@ OVERLAY_INFO_EXTERN(overlay_17);
OVERLAY_INFO_EXTERN(overlay_18);
OVERLAY_INFO_EXTERN(overlay_19);
OVERLAY_INFO_EXTERN(overlay_20);
+OVERLAY_INFO_EXTERN(overlay_alias);
OVERLAY_INFO_EXTERN(overlay_gpio_01);
OVERLAY_INFO_EXTERN(overlay_gpio_02a);
OVERLAY_INFO_EXTERN(overlay_gpio_02b);
@@ -3885,6 +3958,7 @@ static struct overlay_info overlays[] = {
OVERLAY_INFO(overlay_18, 0, 0),
OVERLAY_INFO(overlay_19, 0, 0),
OVERLAY_INFO(overlay_20, 0, 0),
+ OVERLAY_INFO(overlay_alias, 0, 0),
OVERLAY_INFO(overlay_gpio_01, 0, 0),
OVERLAY_INFO(overlay_gpio_02a, 0, 0),
OVERLAY_INFO(overlay_gpio_02b, 0, 0),
--
2.54.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert
2026-07-21 2:52 ` [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert Abdurrahman Hussain
@ 2026-07-21 2:58 ` sashiko-bot
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-07-21 2:58 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: conor+dt, robh, devicetree
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The test assertion for verifying alias removal trivially passes because it checks the wrong node, rendering the removal test ineffective.
- [Medium] The error path for `of_overlay_fdt_apply()` skips cleanup, leading to a memory and device tree state leak.
--
--- Patch [4]: [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert ---
commit d95c5208da27c0c2461530192533ee49d2b62c1c
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
of: unittest: cover /aliases updates from overlay apply/revert
This commit adds overlay_alias.dtso and of_unittest_overlay_alias() to
cover the end-to-end flow of aliases inside an overlay. It verifies alias
updates when applying and reverting overlays with a non-NULL target base.
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index e255f54f4d760..35d75f2b3ef25 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
[ ... ]
> + size = info->dtbo_end - info->dtbo_begin;
> + ret = of_overlay_fdt_apply(info->dtbo_begin, size, &ovcs_id, base);
> + if (ret < 0) {
> + unittest(0, "overlay_alias apply failed, ret = %d\n", ret);
> + goto out_put_base;
[Severity: Medium]
Does this error path skip cleaning up the changeset?
Looking at of_overlay_fdt_apply(), if it fails during the apply phase, it
may still populate ovcs_id. Does this code need to call of_overlay_remove()
to clean up the partial changeset before jumping to out_put_base, to avoid
leaking memory and device tree state?
> + }
> +
> + target = of_find_node_by_path(target_path);
> + if (!target) {
> + unittest(0, "could not find grafted target %s\n", target_path);
> + goto out_remove;
> + }
> +
> + id = of_alias_get_id(target, "testcase-alias");
> + unittest(id == 99,
> + "of_alias_get_id() = %d after overlay apply, expected 99\n",
> + id);
> + of_node_put(target);
> +
> +out_remove:
> + ret = of_overlay_remove(&ovcs_id);
> + if (ret) {
> + unittest(0, "overlay_alias remove failed, ret = %d\n", ret);
> + goto out_put_base;
> + }
> +
> + id = of_alias_get_id(base, "testcase-alias");
[Severity: Medium]
Does this code test the correct node during the removal assertion?
The assertion checks 'base' for the alias, but the earlier success path
checked 'target'. Doesn't of_alias_get_id(base, "testcase-alias") always
return -ENODEV here, meaning the test will pass even if the overlay removal
failed to actually clean up the alias?
> + unittest(id == -ENODEV,
> + "of_alias_get_id() = %d after overlay remove, expected -ENODEV\n",
> + id);
> +
> +out_put_base:
> + of_node_put(base);
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-nh-of-alias-overlay-v1-0-27da6848dd84@nexthop.ai?part=4
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications
2026-07-20 7:02 [PATCH RFC 0/4] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
@ 2026-07-20 7:02 ` Abdurrahman Hussain
2026-07-20 7:17 ` sashiko-bot
0 siblings, 1 reply; 11+ messages in thread
From: Abdurrahman Hussain @ 2026-07-20 7:02 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan
Cc: devicetree, linux-kernel, Abdurrahman Hussain
/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.
Fix by registering an internal OF reconfig notifier from
of_core_init() that mirrors /aliases property changes into
aliases_lookup:
OF_RECONFIG_ADD_PROPERTY -> of_alias_create(pp, kzalloc, owned=true)
OF_RECONFIG_REMOVE_PROPERTY -> of_alias_destroy(name)
OF_RECONFIG_UPDATE_PROPERTY -> destroy + create
The reconfig notifier chain fires from both direct changesets and
overlay apply/revert, so the same code path covers runtime dt
modifications and overlay-declared aliases without any overlay-
specific hook in drivers/of/overlay.c. Grant Likely suggested this
shape on Geert Uytterhoeven's 2015 RFC [1]; Geert's original hook was
in dynamic.c directly.
Match the /aliases target node structurally (name == "aliases" and
parent == root) rather than by pointer against the of_aliases global.
A system with no boot-time /aliases has of_aliases == NULL, so an
overlay that creates /aliases from scratch would otherwise be missed
from the first ATTACH_NODE onward. The ATTACH/DETACH_NODE cases also
update of_aliases lazily so subsequent consumers see it.
Factor the per-property loop body of of_alias_scan() into
of_alias_create() so the boot-time scan and the runtime notifier share
one code path. Add a one-bit @owned flag to struct alias_prop tracking
whether the entry was kmalloc'd (overlay-time) or came from the boot-
time memblock allocator via of_alias_scan(). of_alias_destroy() skips
non-owned entries, so an overlay-driven UPDATE_PROPERTY against a
boot-time alias can't kfree() memblock storage — addressing the
allocator-mismatch worry Grant flagged on the 2015 series [2].
An @owned entry also holds an of_node_get() reference to its target,
released by of_alias_destroy(); this fixes a smaller leak Geert's
original of_alias_create() would have introduced for overlay targets.
Naming builds on Geert's original series:
- "of: Extract of_alias_create()" [3]
- "of: Add of_alias_destroy()" [4]
- "of/dynamic: Update list of aliases on aliases changes" [5]
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]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/of/base.c | 184 ++++++++++++++++++++++++++++++++++++++----------
drivers/of/of_private.h | 7 ++
2 files changed, 155 insertions(+), 36 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 6e7a42dedad3..d516523a71d5 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1915,6 +1915,152 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
ap->alias, ap->stem, ap->id, np);
}
+/*
+ * Build an alias_prop for @pp using @dt_alloc as the storage allocator
+ * and add it to aliases_lookup. @owned is stored on the entry so the
+ * matching destroy path knows whether the alias_prop is a kmalloc'd
+ * struct that must be kfree()d (with a paired of_node_put on the
+ * target) or a memblock/dt_alloc'd struct that must be left alone.
+ *
+ * Pseudo-properties (name, phandle, ...) and alias names not ending in
+ * a numeric id are silently skipped.
+ */
+static void of_alias_create(const struct property *pp,
+ void *(*dt_alloc)(u64 size, u64 align),
+ bool owned)
+{
+ const char *start = pp->name;
+ const char *end = start + strlen(start);
+ struct device_node *np;
+ struct alias_prop *ap;
+ int id, len;
+
+ if (is_pseudo_property(pp->name))
+ return;
+
+ np = of_find_node_by_path(pp->value);
+ if (!np)
+ return;
+
+ while (isdigit(*(end - 1)) && end > start)
+ end--;
+ len = end - start;
+
+ 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);
+ ap->alias = start;
+ ap->owned = owned;
+ of_alias_add(ap, np, id, start, len);
+ return;
+
+out_put:
+ /*
+ * Boot-time entries reach here on parse failure; leaking the
+ * of_node_get() from of_find_node_by_path() is fine because the
+ * boot tree is never freed. Overlay-time entries need the put so
+ * the target node's refcount tracks the failed-parse case
+ * symmetrically with success.
+ */
+ if (owned)
+ of_node_put(np);
+}
+
+/*
+ * Reverse of of_alias_create(): find the owned alias_prop whose name
+ * matches @name, unlink it, and release everything it owns. Non-owned
+ * (boot-time) entries are skipped so an overlay-driven UPDATE against
+ * a boot-time alias can't kfree() memblock storage. That does mean an
+ * overlay updating a boot-time alias leaves two entries in
+ * aliases_lookup — deferred as a separate cleanup.
+ */
+static void of_alias_destroy(const char *name)
+{
+ struct alias_prop *ap, *tmp;
+
+ list_for_each_entry_safe(ap, tmp, &aliases_lookup, link) {
+ if (!ap->owned || strcmp(ap->alias, name) != 0)
+ continue;
+ list_del(&ap->link);
+ of_node_put(ap->np);
+ kfree(ap);
+ return;
+ }
+}
+
+static void *alias_alloc(u64 size, u64 align)
+{
+ return kzalloc(size, GFP_KERNEL);
+}
+
+/*
+ * OF reconfig notifier that mirrors /aliases property changes into
+ * aliases_lookup. Fires on both direct changesets and overlay
+ * apply/revert, so of_alias_get_id() returns the right id for aliases
+ * declared inside an overlay.
+ */
+static int of_aliases_reconfig_notifier(struct notifier_block *nb,
+ unsigned long action, void *arg)
+{
+ struct of_reconfig_data *rd = arg;
+
+ /*
+ * Match /aliases structurally (name + root-parent) rather than by
+ * pointer against the of_aliases global — a system with no
+ * boot-time /aliases (of_aliases == NULL) can still acquire one
+ * from an overlay, and we must track its properties from the
+ * first ATTACH_NODE onward.
+ */
+ if (!rd->dn || !rd->dn->parent ||
+ !of_node_is_root(rd->dn->parent) ||
+ !of_node_name_eq(rd->dn, "aliases"))
+ return NOTIFY_DONE;
+
+ switch (action) {
+ case OF_RECONFIG_ATTACH_NODE:
+ if (!of_aliases)
+ of_aliases = rd->dn;
+ break;
+ case OF_RECONFIG_DETACH_NODE:
+ if (of_aliases == rd->dn)
+ of_aliases = NULL;
+ break;
+ case OF_RECONFIG_ADD_PROPERTY:
+ of_alias_create(rd->prop, alias_alloc, true);
+ break;
+ case OF_RECONFIG_REMOVE_PROPERTY:
+ of_alias_destroy(rd->prop->name);
+ break;
+ case OF_RECONFIG_UPDATE_PROPERTY:
+ of_alias_destroy(rd->old_prop->name);
+ of_alias_create(rd->prop, alias_alloc, true);
+ break;
+ default:
+ break;
+ }
+ return NOTIFY_OK;
+}
+
+static struct notifier_block of_aliases_nb = {
+ .notifier_call = of_aliases_reconfig_notifier,
+};
+
+static int __init of_aliases_reconfig_init(void)
+{
+ return of_reconfig_notifier_register(&of_aliases_nb);
+}
+
+/*
+ * of_alias_scan() runs from of_core_init() (core_initcall), so hook the
+ * reconfig notifier one initcall level later to guarantee the initial
+ * static scan is complete before any dynamic tracking begins.
+ */
+core_initcall_sync(of_aliases_reconfig_init);
+
/**
* of_alias_scan - Scan all properties of the 'aliases' node
* @dt_alloc: An allocator that provides a virtual address to memory
@@ -1950,42 +2096,8 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
if (!of_aliases)
return;
- for_each_property_of_node(of_aliases, pp) {
- const char *start = pp->name;
- const char *end = start + strlen(start);
- struct device_node *np;
- struct alias_prop *ap;
- int id, len;
-
- /* Skip those we do not want to proceed */
- if (is_pseudo_property(pp->name))
- continue;
-
- np = of_find_node_by_path(pp->value);
- if (!np)
- continue;
-
- /* walk the alias backwards to extract the id and work out
- * the 'stem' string */
- while (isdigit(*(end-1)) && end > start)
- end--;
- len = end - start;
-
- if (kstrtoint(end, 10, &id) < 0) {
- of_node_put(np);
- continue;
- }
-
- /* Allocate an alias_prop with enough space for the stem */
- ap = dt_alloc(sizeof(*ap) + len + 1, __alignof__(*ap));
- if (!ap) {
- of_node_put(np);
- continue;
- }
- memset(ap, 0, sizeof(*ap) + len + 1);
- ap->alias = start;
- of_alias_add(ap, np, id, start, len);
- }
+ for_each_property_of_node(of_aliases, pp)
+ of_alias_create(pp, dt_alloc, false);
}
/**
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 0ae16da066e2..9d16765ae2c3 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -17,6 +17,12 @@
* @alias: Alias property name
* @np: Pointer to device_node that the alias stands for
* @id: Index value from end of alias name
+ * @owned: True if @alias was kstrdup'd and @np was of_node_get'd on
+ * insertion (overlay-time entries). False for entries built
+ * by of_alias_scan() at boot, where @alias points into the
+ * FDT and @np is an unreferenced pointer. The removal path
+ * uses this flag to decide whether it must kfree(@alias),
+ * of_node_put(@np), and kfree(the struct itself).
* @stem: Alias string without the index
*
* The structure represents one alias property of 'aliases' node as
@@ -27,6 +33,7 @@ struct alias_prop {
const char *alias;
struct device_node *np;
int id;
+ bool owned;
char stem[];
};
--
2.54.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications
2026-07-20 7:02 ` [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications Abdurrahman Hussain
@ 2026-07-20 7:17 ` sashiko-bot
2026-07-21 2:49 ` Abdurrahman Hussain
0 siblings, 1 reply; 11+ messages in thread
From: sashiko-bot @ 2026-07-20 7:17 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
New issues:
- [Critical] Unconditional dereference of `rd->old_prop` on OF_RECONFIG_UPDATE_PROPERTY causes a NULL pointer panic.
- [Critical] OF_RECONFIG_DETACH_NODE fails to remove dynamically allocated aliases, leading to a Memory Leak and Use-After-Free.
- [High] Lockless modification of the global `aliases_lookup` list causes data races and memory corruption.
- [High] Updating a boot-time alias creates duplicate entries mapping to the same ID, causing driver resource conflicts.
- [Medium] OF_RECONFIG_ATTACH_NODE fails to scan the node's properties, silently ignoring aliases added in a newly attached /aliases node.
- [Medium] The global `of_aliases` pointer mismanages node reference counting, causing leaks and weak references.
- [Low] The struct alias_prop::owned documentation contains factually incorrect and misleading claims regarding memory ownership.
Pre-existing issues:
- [Critical] Creating an alias from an empty property causes a NULL pointer dereference in of_find_node_by_path().
- [High] The alias parsing loop contains a 1-byte out-of-bounds read if the property name is an empty string.
--
--- Patch [1]: [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications ---
commit 172d1d81baebcd7bb4e961520fc11e6b4eadbc38
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
of: incrementally update /aliases lookup on reconfig notifications
This commit registers an OF reconfig notifier to mirror changes in the
/aliases node into the global aliases_lookup list. It extracts the alias
parsing logic into a common function and tracks whether entries are statically
or dynamically allocated. This ensures aliases defined in overlays are
correctly resolved by drivers.
Link: https://lore.kernel.org/lkml/1435675876-2159-1-git-send-email-geert+renesas@glider.be/
Link: https://lore.kernel.org/lkml/20150630172131.D4E6CC4041A@trevor.secretlab.ca/
Link: https://lore.kernel.org/lkml/1435675876-2159-2-git-send-email-geert+renesas@glider.be/
Link: https://lore.kernel.org/lkml/1435675876-2159-3-git-send-email-geert+renesas@glider.be/
Link: https://lore.kernel.org/lkml/1435675876-2159-4-git-send-email-geert+renesas@glider.be/
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 6e7a42dedad3f..d516523a71d5e 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
[ ... ]
> @@ -1915,6 +1915,152 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
> +/*
> + * Build an alias_prop for @pp using @dt_alloc as the storage allocator
> + * and add it to aliases_lookup. @owned is stored on the entry so the
> + * matching destroy path knows whether the alias_prop is a kmalloc'd
> + * struct that must be kfree()d (with a paired of_node_put on the
> + * target) or a memblock/dt_alloc'd struct that must be left alone.
> + *
> + * Pseudo-properties (name, phandle, ...) and alias names not ending in
> + * a numeric id are silently skipped.
> + */
> +static void of_alias_create(const struct property *pp,
> + void *(*dt_alloc)(u64 size, u64 align),
> + bool owned)
> +{
> + const char *start = pp->name;
> + const char *end = start + strlen(start);
> + struct device_node *np;
> + struct alias_prop *ap;
> + int id, len;
> +
> + if (is_pseudo_property(pp->name))
> + return;
> +
> + np = of_find_node_by_path(pp->value);
[Severity: Critical]
This is a pre-existing issue, but does this cause a NULL pointer
dereference if the property is empty?
If an overlay or boot DT defines a property in /aliases with no value,
pp->value is NULL. Since of_alias_create() passes pp->value directly to
of_find_node_by_path(), which passes it to strchr(), will this crash?
> + if (!np)
> + return;
> +
> + while (isdigit(*(end - 1)) && end > start)
> + end--;
[Severity: High]
This isn't a bug introduced by this patch, but does the loop condition
cause an out-of-bounds read if the property name is an empty string?
If start is empty, end equals start. The loop evaluates isdigit(*(end - 1))
before verifying end > start, causing an immediate read of the memory byte
preceding the buffer.
> + len = end - start;
[ ... ]
> +static void of_alias_destroy(const char *name)
> +{
> + struct alias_prop *ap, *tmp;
> +
> + list_for_each_entry_safe(ap, tmp, &aliases_lookup, link) {
> + if (!ap->owned || strcmp(ap->alias, name) != 0)
> + continue;
[Severity: High]
If an overlay modifies the target of an existing boot-time alias, will
skipping the destruction of boot-time entries leave duplicate entries in
the list?
Since this skips entries where owned is false, the old boot-time alias
remains in aliases_lookup alongside the newly created overlay alias.
When of_alias_get_id() searches the list, could it return the same ID for
both devices?
> + list_del(&ap->link);
> + of_node_put(ap->np);
> + kfree(ap);
> + return;
> + }
> +}
[ ... ]
> +static int of_aliases_reconfig_notifier(struct notifier_block *nb,
> + unsigned long action, void *arg)
> +{
> + struct of_reconfig_data *rd = arg;
> +
> + /*
> + * Match /aliases structurally (name + root-parent) rather than by
> + * pointer against the of_aliases global — a system with no
> + * boot-time /aliases (of_aliases == NULL) can still acquire one
> + * from an overlay, and we must track its properties from the
> + * first ATTACH_NODE onward.
> + */
> + if (!rd->dn || !rd->dn->parent ||
> + !of_node_is_root(rd->dn->parent) ||
> + !of_node_name_eq(rd->dn, "aliases"))
> + return NOTIFY_DONE;
> +
> + switch (action) {
> + case OF_RECONFIG_ATTACH_NODE:
> + if (!of_aliases)
> + of_aliases = rd->dn;
[Severity: Medium]
Does assigning of_aliases here create a weak reference?
During boot-time scanning, of_alias_scan() acquires of_aliases with an
incremented reference count. Here, it is assigned without calling
of_node_get().
[Severity: Medium]
Also, if the system started without a boot-time /aliases node, does
attaching one fail to parse its properties?
Since this handler updates the of_aliases pointer but misses the
for_each_property_of_node() loop to actually create the aliases, it seems
the aliases in a newly attached node are silently ignored.
> + break;
> + case OF_RECONFIG_DETACH_NODE:
> + if (of_aliases == rd->dn)
> + of_aliases = NULL;
[Severity: Medium]
Does clearing the pointer here cause a reference leak since of_node_put()
is omitted?
[Severity: Critical]
Additionally, does skipping the removal of the dynamic aliases cause a
memory leak and use-after-free?
The DETACH_NODE handler clears the of_aliases pointer but skips removing
the corresponding alias_prop entries from aliases_lookup. When the detached
node and its properties are later freed, dangling entries will remain in
the global list. A subsequent alias update could iterate the list and call
strcmp() on freed pointer strings.
> + break;
> + case OF_RECONFIG_ADD_PROPERTY:
> + of_alias_create(rd->prop, alias_alloc, true);
[Severity: High]
Is it safe to modify aliases_lookup locklessly here?
The notifier processes dynamic changes and calls into of_alias_create()
which uses list_add_tail() on the global aliases_lookup list. OF reconfig
notifiers are intentionally executed after of_mutex has been unlocked, such
as in __of_changeset_apply_notify().
Could this race against other writers, or against driver probes calling
of_alias_get_id() which iterates the list expecting it to be protected by
of_mutex?
> + break;
> + case OF_RECONFIG_REMOVE_PROPERTY:
> + of_alias_destroy(rd->prop->name);
> + break;
> + case OF_RECONFIG_UPDATE_PROPERTY:
> + of_alias_destroy(rd->old_prop->name);
[Severity: Critical]
Can rd->old_prop be NULL here?
The of_update_property() API allows adding a property if it does not
already exist, and emits an OF_RECONFIG_UPDATE_PROPERTY event with
old_prop set to NULL. Would unconditionally dereferencing
rd->old_prop->name cause a panic in this scenario?
> + of_alias_create(rd->prop, alias_alloc, true);
> + break;
> + default:
> + break;
> + }
> + return NOTIFY_OK;
> +}
[ ... ]
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 0ae16da066e2c..9d16765ae2c3f 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -17,6 +17,12 @@
> * @alias: Alias property name
> * @np: Pointer to device_node that the alias stands for
> * @id: Index value from end of alias name
> + * @owned: True if @alias was kstrdup'd and @np was of_node_get'd on
> + * insertion (overlay-time entries). False for entries built
> + * by of_alias_scan() at boot, where @alias points into the
> + * FDT and @np is an unreferenced pointer. The removal path
> + * uses this flag to decide whether it must kfree(@alias),
> + * of_node_put(@np), and kfree(the struct itself).
[Severity: Low]
Does this documentation accurately reflect the implementation?
The comment claims @alias is kstrdup'd on insertion and kfree'd on
removal. However, the code merely stores the pp->name pointer directly
and never calls kfree() on the alias string. It also mentions np is
unreferenced at boot time, but of_find_node_by_path() does take a
reference.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-nh-of-alias-overlay-v1-0-f1e5d9889b30@nexthop.ai?part=1
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications
2026-07-20 7:17 ` sashiko-bot
@ 2026-07-21 2:49 ` Abdurrahman Hussain
0 siblings, 0 replies; 11+ messages in thread
From: Abdurrahman Hussain @ 2026-07-21 2:49 UTC (permalink / raw)
To: sashiko-reviews, Abdurrahman Hussain; +Cc: robh, conor+dt, devicetree
On Mon Jul 20, 2026 at 12:17 AM PDT, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
>
> New issues:
> - [Critical] Unconditional dereference of `rd->old_prop` on OF_RECONFIG_UPDATE_PROPERTY causes a NULL pointer panic.
> - [Critical] OF_RECONFIG_DETACH_NODE fails to remove dynamically allocated aliases, leading to a Memory Leak and Use-After-Free.
> - [High] Lockless modification of the global `aliases_lookup` list causes data races and memory corruption.
> - [High] Updating a boot-time alias creates duplicate entries mapping to the same ID, causing driver resource conflicts.
> - [Medium] OF_RECONFIG_ATTACH_NODE fails to scan the node's properties, silently ignoring aliases added in a newly attached /aliases node.
> - [Medium] The global `of_aliases` pointer mismanages node reference counting, causing leaks and weak references.
> - [Low] The struct alias_prop::owned documentation contains factually incorrect and misleading claims regarding memory ownership.
>
> Pre-existing issues:
> - [Critical] Creating an alias from an empty property causes a NULL pointer dereference in of_find_node_by_path().
> - [High] The alias parsing loop contains a 1-byte out-of-bounds read if the property name is an empty string.
> --
>
> --- Patch [1]: [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications ---
> commit 172d1d81baebcd7bb4e961520fc11e6b4eadbc38
> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>
> of: incrementally update /aliases lookup on reconfig notifications
>
> This commit registers an OF reconfig notifier to mirror changes in the
> /aliases node into the global aliases_lookup list. It extracts the alias
> parsing logic into a common function and tracks whether entries are statically
> or dynamically allocated. This ensures aliases defined in overlays are
> correctly resolved by drivers.
>
> Link: https://lore.kernel.org/lkml/1435675876-2159-1-git-send-email-geert+renesas@glider.be/
> Link: https://lore.kernel.org/lkml/20150630172131.D4E6CC4041A@trevor.secretlab.ca/
> Link: https://lore.kernel.org/lkml/1435675876-2159-2-git-send-email-geert+renesas@glider.be/
> Link: https://lore.kernel.org/lkml/1435675876-2159-3-git-send-email-geert+renesas@glider.be/
> Link: https://lore.kernel.org/lkml/1435675876-2159-4-git-send-email-geert+renesas@glider.be/
>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 6e7a42dedad3f..d516523a71d5e 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>
> [ ... ]
>
>> @@ -1915,6 +1915,152 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
>> +/*
>> + * Build an alias_prop for @pp using @dt_alloc as the storage allocator
>> + * and add it to aliases_lookup. @owned is stored on the entry so the
>> + * matching destroy path knows whether the alias_prop is a kmalloc'd
>> + * struct that must be kfree()d (with a paired of_node_put on the
>> + * target) or a memblock/dt_alloc'd struct that must be left alone.
>> + *
>> + * Pseudo-properties (name, phandle, ...) and alias names not ending in
>> + * a numeric id are silently skipped.
>> + */
>> +static void of_alias_create(const struct property *pp,
>> + void *(*dt_alloc)(u64 size, u64 align),
>> + bool owned)
>> +{
>> + const char *start = pp->name;
>> + const char *end = start + strlen(start);
>> + struct device_node *np;
>> + struct alias_prop *ap;
>> + int id, len;
>> +
>> + if (is_pseudo_property(pp->name))
>> + return;
>> +
>> + np = of_find_node_by_path(pp->value);
>
> [Severity: Critical]
> This is a pre-existing issue, but does this cause a NULL pointer
> dereference if the property is empty?
>
> If an overlay or boot DT defines a property in /aliases with no value,
> pp->value is NULL. Since of_alias_create() passes pp->value directly to
> of_find_node_by_path(), which passes it to strchr(), will this crash?
>
>> + if (!np)
>> + return;
>> +
>> + while (isdigit(*(end - 1)) && end > start)
>> + end--;
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does the loop condition
> cause an out-of-bounds read if the property name is an empty string?
>
> If start is empty, end equals start. The loop evaluates isdigit(*(end - 1))
> before verifying end > start, causing an immediate read of the memory byte
> preceding the buffer.
>
>> + len = end - start;
>
> [ ... ]
>
>> +static void of_alias_destroy(const char *name)
>> +{
>> + struct alias_prop *ap, *tmp;
>> +
>> + list_for_each_entry_safe(ap, tmp, &aliases_lookup, link) {
>> + if (!ap->owned || strcmp(ap->alias, name) != 0)
>> + continue;
>
> [Severity: High]
> If an overlay modifies the target of an existing boot-time alias, will
> skipping the destruction of boot-time entries leave duplicate entries in
> the list?
>
> Since this skips entries where owned is false, the old boot-time alias
> remains in aliases_lookup alongside the newly created overlay alias.
> When of_alias_get_id() searches the list, could it return the same ID for
> both devices?
>
>> + list_del(&ap->link);
>> + of_node_put(ap->np);
>> + kfree(ap);
>> + return;
>> + }
>> +}
>
> [ ... ]
>
>> +static int of_aliases_reconfig_notifier(struct notifier_block *nb,
>> + unsigned long action, void *arg)
>> +{
>> + struct of_reconfig_data *rd = arg;
>> +
>> + /*
>> + * Match /aliases structurally (name + root-parent) rather than by
>> + * pointer against the of_aliases global — a system with no
>> + * boot-time /aliases (of_aliases == NULL) can still acquire one
>> + * from an overlay, and we must track its properties from the
>> + * first ATTACH_NODE onward.
>> + */
>> + if (!rd->dn || !rd->dn->parent ||
>> + !of_node_is_root(rd->dn->parent) ||
>> + !of_node_name_eq(rd->dn, "aliases"))
>> + return NOTIFY_DONE;
>> +
>> + switch (action) {
>> + case OF_RECONFIG_ATTACH_NODE:
>> + if (!of_aliases)
>> + of_aliases = rd->dn;
>
> [Severity: Medium]
> Does assigning of_aliases here create a weak reference?
>
> During boot-time scanning, of_alias_scan() acquires of_aliases with an
> incremented reference count. Here, it is assigned without calling
> of_node_get().
>
> [Severity: Medium]
> Also, if the system started without a boot-time /aliases node, does
> attaching one fail to parse its properties?
>
> Since this handler updates the of_aliases pointer but misses the
> for_each_property_of_node() loop to actually create the aliases, it seems
> the aliases in a newly attached node are silently ignored.
>
>> + break;
>> + case OF_RECONFIG_DETACH_NODE:
>> + if (of_aliases == rd->dn)
>> + of_aliases = NULL;
>
> [Severity: Medium]
> Does clearing the pointer here cause a reference leak since of_node_put()
> is omitted?
>
> [Severity: Critical]
> Additionally, does skipping the removal of the dynamic aliases cause a
> memory leak and use-after-free?
>
> The DETACH_NODE handler clears the of_aliases pointer but skips removing
> the corresponding alias_prop entries from aliases_lookup. When the detached
> node and its properties are later freed, dangling entries will remain in
> the global list. A subsequent alias update could iterate the list and call
> strcmp() on freed pointer strings.
>
>> + break;
>> + case OF_RECONFIG_ADD_PROPERTY:
>> + of_alias_create(rd->prop, alias_alloc, true);
>
> [Severity: High]
> Is it safe to modify aliases_lookup locklessly here?
>
> The notifier processes dynamic changes and calls into of_alias_create()
> which uses list_add_tail() on the global aliases_lookup list. OF reconfig
> notifiers are intentionally executed after of_mutex has been unlocked, such
> as in __of_changeset_apply_notify().
>
> Could this race against other writers, or against driver probes calling
> of_alias_get_id() which iterates the list expecting it to be protected by
> of_mutex?
>
>> + break;
>> + case OF_RECONFIG_REMOVE_PROPERTY:
>> + of_alias_destroy(rd->prop->name);
>> + break;
>> + case OF_RECONFIG_UPDATE_PROPERTY:
>> + of_alias_destroy(rd->old_prop->name);
>
> [Severity: Critical]
> Can rd->old_prop be NULL here?
>
> The of_update_property() API allows adding a property if it does not
> already exist, and emits an OF_RECONFIG_UPDATE_PROPERTY event with
> old_prop set to NULL. Would unconditionally dereferencing
> rd->old_prop->name cause a panic in this scenario?
>
>> + of_alias_create(rd->prop, alias_alloc, true);
>> + break;
>> + default:
>> + break;
>> + }
>> + return NOTIFY_OK;
>> +}
>
> [ ... ]
>
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index 0ae16da066e2c..9d16765ae2c3f 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -17,6 +17,12 @@
>> * @alias: Alias property name
>> * @np: Pointer to device_node that the alias stands for
>> * @id: Index value from end of alias name
>> + * @owned: True if @alias was kstrdup'd and @np was of_node_get'd on
>> + * insertion (overlay-time entries). False for entries built
>> + * by of_alias_scan() at boot, where @alias points into the
>> + * FDT and @np is an unreferenced pointer. The removal path
>> + * uses this flag to decide whether it must kfree(@alias),
>> + * of_node_put(@np), and kfree(the struct itself).
>
> [Severity: Low]
> Does this documentation accurately reflect the implementation?
>
> The comment claims @alias is kstrdup'd on insertion and kfree'd on
> removal. However, the code merely stores the pp->name pointer directly
> and never calls kfree() on the alias string. It also mentions np is
> unreferenced at boot time, but of_find_node_by_path() does take a
> reference.
Good feedback! Will address in v2.
Thanks,
Abdurrahman
^ permalink raw reply [flat|nested] 11+ messages in thread