[PATCH RFC 0/4] of: teach overlay code to keep /aliases in sync

[PATCH RFC 0/4] of: teach overlay code to keep /aliases in sync

[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.

The gap has been public since 2015 [1] and reproduces trivially: apply
an overlay that declares e.g. `i2c99 = &foo;`, ask
of_alias_get_id(foo, "i2c") -> -ENODEV. Bootlin's ELCE 2025 talk on
PCI DT overlays [2] enumerates "i2c muxes" as one of the subsystems
broken by dynamic overlays; alias pinning is the underlying cause.

The core fix (patch 1) is a reconfig notifier that mirrors /aliases
property changes into aliases_lookup. Two smaller overlay-code fixes
(patches 2 and 3) fall out of the same use case: without them, the
notifier alone can't actually resolve overlay-declared aliases.

Prior art
---------

Geert Uytterhoeven posted a 3-patch RFC in June 2015 [1] with the same
alias-tracking design shape. Grant Likely reviewed positively; merge
was gated on missing unittests and an object-lifetime concern the
author self-flagged, and the series was never reposted as non-RFC. Ten
years later, drivers/of/overlay.c still contains zero references to
aliases, of_alias_scan, or aliases_lookup.

Series contents
---------------

Patch 1 adds a reconfig notifier that mirrors /aliases property
changes into aliases_lookup. It also refactors of_alias_scan()'s
per-property body into a helper of_alias_create() shared by the
boot-time scan and the runtime notifier. A one-bit `owned` flag on
struct alias_prop distinguishes kmalloc'd (overlay-time) entries from
memblock-backed (boot-time) ones so the remove path can't kfree the
wrong storage — addressing the lifetime concern that stopped Geert in
2015. The notifier keys off structural properties of the target node
(name + root-parent) rather than the of_aliases global, so overlays
that create /aliases from scratch on a system without a boot-time
aliases node are covered too.

Patch 2 fixes find_target() so an overlay applied with a non-NULL
target base can still reach the DT root via target-path="/foo". The
current code unconditionally concatenates base + target-path via
"%pOF%s", so target-path="/aliases" resolves to "<base>/aliases" and
target-path="/" produces "<base>/" (never a valid node). After this
patch, an empty target-path continues to mean "the target base
itself" — preserving the shape used by drivers/misc/lan966x_pci.c,
the only in-tree caller of of_overlay_fdt_apply() that passes a
non-NULL base — while any non-empty target-path is looked up
absolutely.

Patch 3 rewrites /aliases property values from the overlay's internal
fragment path ("/fragment@N/__overlay__/...") to the live-tree path
that the node will occupy after apply. The overlay code already does
this for /__symbols__ via dup_and_fixup_symbol_prop(); /aliases uses
the same textual convention and can reuse the same helper. Without
this, patch 1's notifier receives paths that never resolve in the
live tree, so of_alias_get_id() still returns -ENODEV.

Patch 4 adds a unittest — overlay_alias.dtso plus
of_unittest_overlay_alias() — that applies an overlay declaring
`testcase-alias99` under /aliases and asserts of_alias_get_id() flips
from -ENODEV to 99 across the apply, and back to -ENODEV across the
revert.

Open items — feedback wanted before v1
--------------------------------------

Locking. of_alias_get_id() traverses aliases_lookup without a lock,
which was safe when the list was populated once at boot. Runtime
add/remove requires synchronization; this series does NOT yet add
any. Options:

  (a) protect aliases_lookup with a spinlock (simple, sheds the
      lockless-lookup contract);
  (b) convert to list_head_rcu + synchronize_rcu on remove (keeps
      lookup lockless, more churn);
  (c) piggyback on of_mutex — but of_alias_get_id() is called from
      driver probe context, so grabbing of_mutex there risks lock
      inversion.

Currently leaning (b); happy to be talked out of it.

Verification
------------

Series was applied to v7.2-rc3 and boot-tested on an x86 platform
with two PCI-attached Xilinx FPGAs whose i2c-xiic controllers come
from driver-embedded DT overlays. Before this series, i2c bus
numbers auto-assigned regardless of what /aliases said and shifted
across boots depending on which FPGA won the probe race. After: 89
adapters, `/dev/i2c-N` numbering matches the overlay aliases exactly
and is stable across reboots, `sensors` reads live telemetry through
mux channels, and dmesg is free of WARN/BUG/Oops. Patch 4's unittest
passes.

[1] https://lore.kernel.org/lkml/1435675876-2159-1-git-send-email-geert+renesas@glider.be/
    Geert Uytterhoeven, "[PATCH/RFC 0/3] of/overlay: Update aliases
    when added or removed", 2015-06-30.
[2] Hervé Codina, "Using Device Tree Overlays to Support Complex PCI
    Devices in Linux", ELCE 2025.
    https://bootlin.com/pub/conferences/2025/elce/codina-pcie-dt-overlay.pdf

Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
Abdurrahman Hussain (4):
      of: incrementally update /aliases lookup on reconfig notifications
      of/overlay: look up absolute target-paths absolutely
      of/overlay: rewrite /aliases path values to live-tree paths
      of: unittest: cover /aliases updates from overlay apply/revert

 drivers/of/base.c                           | 184 ++++++++++++++++++++++------
 drivers/of/of_private.h                     |   7 ++
 drivers/of/overlay.c                        |  47 ++++---
 drivers/of/unittest-data/Makefile           |   2 +
 drivers/of/unittest-data/overlay_alias.dtso |   9 ++
 drivers/of/unittest.c                       |  51 ++++++++
 6 files changed, 246 insertions(+), 54 deletions(-)
---
base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
change-id: 20260719-nh-of-alias-overlay-6e5e0d56212b

Best regards,
--  
Abdurrahman Hussain <abdurrahman@nexthop.ai>

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

[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

[PATCH RFC 2/4] of/overlay: look up absolute target-paths absolutely

[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

[PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths

[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() as well. Fall back to a raw dup when the
value is not a rewritable fragment path (legacy string aliases like
"ttyS0" that some out-of-tree code writes verbatim).

Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/of/overlay.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 654a70d5cb07..338786cef17a 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -350,6 +350,19 @@ 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")) {
+		/*
+		 * /aliases property values are string paths in the same
+		 * "/fragment@N/__overlay__/..." form as /__symbols__.
+		 * Rewrite them to live-tree paths so of_alias_get_id() can
+		 * resolve them; fall back to a raw dup for non-fragment
+		 * values (e.g. legacy string aliases like "ttyS0").
+		 */
+		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
+		if (!new_prop)
+			new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
 	} else {
 		new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
 	}

-- 
2.54.0

[PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert

[Abdurrahman Hussain]

Add overlay_alias.dtso, which declares `testcase-alias99 = ...` under
/aliases via the &{/aliases} shorthand, and an of_unittest_overlay_alias()
runner that:

  1. asserts of_alias_get_id(target, "testcase-alias") is -ENODEV
     before the overlay is applied,
  2. applies the overlay and asserts the same call now returns 99,
  3. removes the overlay and asserts we're back to -ENODEV.

Exercises all three functional patches earlier in this series
together: patch 1's reconfig notifier is what mutates aliases_lookup
on apply/revert, patch 2 is what lets target-path="/aliases" resolve
to the DT root when target_base is non-NULL, and patch 3 is what
rewrites the fragment-internal path in the alias value so
of_find_node_by_path() finds the live-tree target. 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 |  9 +++++
 drivers/of/unittest.c                       | 51 +++++++++++++++++++++++++++++
 3 files changed, 62 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..32532c80505a
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_alias.dtso
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+/* overlay_alias - declare an alias inside an overlay */
+
+&{/aliases} {
+	testcase-alias99 = "/testcase-data/overlay-node/test-bus/test-unittest100";
+};
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e255f54f4d76..a9f4a0bb3087 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -3475,6 +3475,53 @@ static struct notifier_block of_nb = {
 	.notifier_call = of_notify,
 };
 
+static void __init of_unittest_overlay_alias(void)
+{
+	const char *path = "/testcase-data/overlay-node/test-bus/test-unittest100";
+	struct device_node *np;
+	int ovcs_id = 0;
+	int id, ret;
+
+	np = of_find_node_by_path(path);
+	if (!np) {
+		unittest(0, "could not find %s for alias test\n", path);
+		return;
+	}
+
+	id = of_alias_get_id(np, "testcase-alias");
+	if (id != -ENODEV) {
+		unittest(0,
+			 "of_alias_get_id() = %d before overlay, expected -ENODEV\n",
+			 id);
+		goto out;
+	}
+
+	ret = overlay_data_apply("overlay_alias", &ovcs_id);
+	if (!ret) {
+		unittest(0, "overlay_alias apply failed\n");
+		goto out;
+	}
+
+	id = of_alias_get_id(np, "testcase-alias");
+	unittest(id == 99,
+		 "of_alias_get_id() = %d after overlay apply, expected 99\n", id);
+
+	ret = of_overlay_remove(&ovcs_id);
+	if (ret) {
+		unittest(0, "overlay_alias remove failed, ret = %d\n", ret);
+		goto out;
+	}
+
+	id = of_alias_get_id(np, "testcase-alias");
+	unittest(id == -ENODEV,
+		 "of_alias_get_id() = %d after overlay remove, expected -ENODEV\n",
+		 id);
+
+	unittest(1, "overlay alias test passed\n");
+out:
+	of_node_put(np);
+}
+
 static void __init of_unittest_overlay_notify(void)
 {
 	int ovcs_id;
@@ -3649,6 +3696,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 +3897,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 +3935,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

Re: [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The test claims to exercise patch 2's logic but bypasses it by passing NULL for target_base.
- [Medium] The test claims to exercise patch 3's logic but bypasses it by using an absolute live-tree path.
--

--- Patch [4]: [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert ---
commit 1237920f0bd1d803a084dc2f03f9e63aaf5cd6a2
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

of: unittest: cover /aliases updates from overlay apply/revert

This commit adds an overlay and a test runner to verify alias creation and
removal dynamically during device tree overlay apply and revert operations.
It asserts that the alias is correctly added and then removed.

>     exercises all three functional patches earlier in this series
>     together: patch 1's reconfig notifier is what mutates aliases_lookup
>     on apply/revert, patch 2 is what lets target-path="/aliases" resolve
>     to the DT root when target_base is non-NULL, and patch 3 is what
>     rewrites the fragment-internal path in the alias value so
>     of_find_node_by_path() finds the live-tree target. Any one of the
>     three missing turns the middle assertion (get_id -> 99) into
>     -ENODEV.

[Severity: Medium]
Is this description of the test coverage accurate?

As detailed below, the test appears to bypass the logic for both patch 2 and
patch 3. The overlay uses an absolute path instead of an intra-overlay
reference (bypassing patch 3), and overlay_data_apply() passes NULL for
target_base (bypassing patch 2).

> diff --git a/drivers/of/unittest-data/overlay_alias.dtso b/drivers/of/unittest-data/overlay_alias.dtso
> new file mode 100644
> index 0000000000000..32532c80505a7
> --- /dev/null
> +++ b/drivers/of/unittest-data/overlay_alias.dtso
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +/plugin/;
> +
> +/* overlay_alias - declare an alias inside an overlay */
> +
> +&{/aliases} {
> +	testcase-alias99 = "/testcase-data/overlay-node/test-bus/test-unittest100";

[Severity: Medium]
Does this overlay actually test the path rewrite logic from patch 3?

The testcase-alias99 property is defined using an absolute live-tree path
rather than an intra-overlay fragment reference. Because this is already a
valid absolute path, it seems of_alias_create() will successfully resolve it
via of_find_node_by_path() even if the rewrite logic from patch 3 is
missing.

> +};
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index e255f54f4d760..a9f4a0bb30872 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
[ ... ]
> @@ -3475,6 +3475,53 @@ static struct notifier_block of_nb = {
>  	.notifier_call = of_notify,
>  };
>  
> +static void __init of_unittest_overlay_alias(void)
> +{
> +	const char *path = "/testcase-data/overlay-node/test-bus/test-unittest100";
> +	struct device_node *np;
> +	int ovcs_id = 0;
> +	int id, ret;
> +
> +	np = of_find_node_by_path(path);
> +	if (!np) {
> +		unittest(0, "could not find %s for alias test\n", path);
> +		return;
> +	}
> +
> +	id = of_alias_get_id(np, "testcase-alias");
> +	if (id != -ENODEV) {
> +		unittest(0,
> +			 "of_alias_get_id() = %d before overlay, expected -ENODEV\n",
> +			 id);
> +		goto out;
> +	}
> +
> +	ret = overlay_data_apply("overlay_alias", &ovcs_id);

[Severity: Medium]
Does this test actually exercise the target_base logic from patch 2?

Looking at overlay_data_apply(), it calls of_overlay_fdt_apply() with a
hardcoded NULL for target_base:

drivers/of/unittest.c:overlay_data_apply() {
    ...
    ret = of_overlay_fdt_apply(info->dtbo_begin, size, &info->ovcs_id,
                               NULL);
    ...
}

When target_base is NULL, the pre-patch-2 code already resolves
target-path="/aliases" correctly via of_find_node_by_path(). This means the
test might pass and bypass patch 2 entirely, missing the intended test
coverage.

> +	if (!ret) {
> +		unittest(0, "overlay_alias apply failed\n");
> +		goto out;
> +	}
[ ... ]

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

Re: [PATCH RFC 2/4] of/overlay: look up absolute target-paths absolutely

[sashiko-bot]

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

Pre-existing issues:
- [High] Overlay changeset fragments' target and overlay nodes are leaked on the error paths in `init_overlay_changeset()`.
- [High] Calling `of_node_put()` under `devtree_lock` (a raw spinlock) can sleep in atomic context.
- [High] The `lan966x_pci` driver leaks overlay resources if `of_overlay_fdt_apply()` fails.
--

--- Patch [2]: [PATCH RFC 2/4] of/overlay: look up absolute target-paths absolutely ---
commit 8125ed0b64def60707430d565ce13659fa3c53dc
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Subject: of/overlay: look up absolute target-paths absolutely

This commit modifies find_target() to treat non-empty target-paths as
absolute when applying a device tree overlay. It allows overlays to modify
both a subtree under a base node and nodes at the device tree root.

>     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).

[Severity: High]
This isn't a regression introduced by this patch, but does the lan966x_pci
driver leak overlay resources if of_overlay_fdt_apply() fails?

The API contract for of_overlay_fdt_apply() states that if it fails during
of_overlay_apply(), it intentionally skips freeing the changeset and instead
populates *ret_ovcs_id, delegating the cleanup to the caller.

However, if lan966x_pci_load_overlay() encounters a failure, it immediately
returns the error to lan966x_pci_probe():

drivers/misc/lan966x_pci.c:lan966x_pci_probe() {
	...
	ret = lan966x_pci_load_overlay(data);
	if (ret)
		return ret;
	...
}

This returns without invoking of_overlay_remove() on the returned ID, which
would permanently leak the overlay changeset structure, unflattened FDT
memory, and partially applied device tree nodes. Should lan966x_pci_probe()
clean up the overlay if it fails?

> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 08d5351746be6..654a70d5cb075 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c

[ ... ]

> @@ -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;
>  	}

[Severity: High]
This is a pre-existing issue, but I noticed a memory leak when find_target()
returns NULL.

In init_overlay_changeset(), ovcs->count is only updated to the final fragment
count (cnt) at the end of the function. If find_target() returns NULL, the
function jumps to the error path err_out without updating ovcs->count, leaving
it at 0:

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;
		}
	...
}

When the caller subsequently cleans up by calling free_overlay_changeset(), it
uses ovcs->count to iterate over the fragments and release their target and
overlay nodes:

drivers/of/overlay.c:free_overlay_changeset() {
	...
	for (i = 0; i < ovcs->count; i++) {
		of_node_put(ovcs->fragments[i].target);
		of_node_put(ovcs->fragments[i].overlay);
	}
	...
}

Since ovcs->count is 0, none of the previously processed fragments have their
node references released. Can we ensure ovcs->count is updated incrementally
or handle the cleanup of already-processed fragments on the error path?

[Severity: High]
I also noticed another pre-existing issue unrelated to this patch, but can 
calling of_node_put() under devtree_lock cause a sleep in atomic context?

The devtree_lock is a raw spinlock. Several OF node iterators, such as
__of_get_next_child(), call of_node_put(prev) while holding this lock:

drivers/of/base.c:of_get_next_child() {
	...
	raw_spin_lock_irqsave(&devtree_lock, flags);
	next = __of_get_next_child(node, prev);
	raw_spin_unlock_irqrestore(&devtree_lock, flags);
	...
}

drivers/of/base.c:__of_get_next_child() {
	...
	of_node_put(prev);
	return next;
}

If the node being put is a dynamic device node (e.g., from an overlay) and
this is its final reference, of_node_put() calls kobject_release(), which
invokes of_node_release(). Inside of_node_release(), the code calls
fwnode_links_purge(), which acquires the fwnode_link_lock mutex. Taking a
mutex while holding a raw spinlock and with interrupts disabled is a fatal
scheduling-while-atomic bug.

Could this lead to deadlocks or system crashes when an overlay is detached
concurrently with device tree iteration?

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

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

[sashiko-bot]

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

Re: [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths

[sashiko-bot]

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

New issues:
- [High] Fallback to `__of_prop_dup()` for `/aliases` masks ENOMEM errors and bypasses null-termination safety checks, causing silent corruption and potential OOB reads.

Pre-existing issues:
- [Critical] Dangling pointer in target->np->deadprops leads to use-after-free and double-free if of_changeset_add_property() fails.
--

--- Patch [3]: [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths ---
commit 8741f5ffd2983da599c7be77cbf8c1af8325d4be
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

of/overlay: rewrite /aliases path values to live-tree paths

/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.

> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 654a70d5cb075..338786cef17ab 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -350,6 +350,19 @@ 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")) {
> +		/*
> +		 * /aliases property values are string paths in the same
> +		 * "/fragment@N/__overlay__/..." form as /__symbols__.
> +		 * Rewrite them to live-tree paths so of_alias_get_id() can
> +		 * resolve them; fall back to a raw dup for non-fragment
> +		 * values (e.g. legacy string aliases like "ttyS0").
> +		 */
> +		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
> +		if (!new_prop)
> +			new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);

[Severity: High]
If dup_and_fixup_symbol_prop() fails because the string is not
null-terminated, does this fallback to __of_prop_dup() end up copying the
malformed string?

Because dup_and_fixup_symbol_prop() can return NULL for both memory
allocation failures and structural errors like non-null-terminated strings,
falling back to a raw duplicate here preserves the non-null-terminated
string.

Later, the reconfig notifier can trigger of_alias_create() which attempts to
read the property:

drivers/of/base.c:of_alias_create() {
    ...
	if (owned)
		np = of_find_node_by_path(pp->value);
    ...
}

Could this lead to an out-of-bounds read when of_find_node_by_path() treats
the non-null-terminated property value as a C string?

Additionally, does this mask memory allocation failures from
dup_and_fixup_symbol_prop() by falling back to a raw duplicate, injecting
unresolved paths directly into the live tree instead of failing the overlay
application?

>  	} else {
>  		new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
>  	}

[Severity: Critical]
This is a pre-existing issue, but while reviewing add_changeset_property(), I
noticed a problem further down in the same function.

When a property is added to a newly created node, it is linked into the
node's deadprops list before the changeset operation:

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 (ret)
		__of_prop_free(new_prop);
	return ret;
}

If of_changeset_add_property() fails, perhaps due to an allocation error,
__of_prop_free() is called on new_prop, but new_prop is not unlinked from
target->np->deadprops.

Will this leave a dangling pointer in the deadprops list, causing a double
free later when the changeset is aborted and of_node_release() calls
property_list_free() on node->deadprops?

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

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

[Abdurrahman Hussain]

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

Re: [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths

[Abdurrahman Hussain]

On Mon Jul 20, 2026 at 12:20 AM PDT, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Fallback to `__of_prop_dup()` for `/aliases` masks ENOMEM errors and bypasses null-termination safety checks, causing silent corruption and potential OOB reads.
>
> Pre-existing issues:
> - [Critical] Dangling pointer in target->np->deadprops leads to use-after-free and double-free if of_changeset_add_property() fails.
> --
>
> --- Patch [3]: [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths ---
> commit 8741f5ffd2983da599c7be77cbf8c1af8325d4be
> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>
> of/overlay: rewrite /aliases path values to live-tree paths
>
> /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.
>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 654a70d5cb075..338786cef17ab 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -350,6 +350,19 @@ 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")) {
>> +		/*
>> +		 * /aliases property values are string paths in the same
>> +		 * "/fragment@N/__overlay__/..." form as /__symbols__.
>> +		 * Rewrite them to live-tree paths so of_alias_get_id() can
>> +		 * resolve them; fall back to a raw dup for non-fragment
>> +		 * values (e.g. legacy string aliases like "ttyS0").
>> +		 */
>> +		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
>> +		if (!new_prop)
>> +			new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
>
> [Severity: High]
> If dup_and_fixup_symbol_prop() fails because the string is not
> null-terminated, does this fallback to __of_prop_dup() end up copying the
> malformed string?
>
> Because dup_and_fixup_symbol_prop() can return NULL for both memory
> allocation failures and structural errors like non-null-terminated strings,
> falling back to a raw duplicate here preserves the non-null-terminated
> string.
>
> Later, the reconfig notifier can trigger of_alias_create() which attempts to
> read the property:
>
> drivers/of/base.c:of_alias_create() {
>     ...
> 	if (owned)
> 		np = of_find_node_by_path(pp->value);
>     ...
> }
>
> Could this lead to an out-of-bounds read when of_find_node_by_path() treats
> the non-null-terminated property value as a C string?
>
> Additionally, does this mask memory allocation failures from
> dup_and_fixup_symbol_prop() by falling back to a raw duplicate, injecting
> unresolved paths directly into the live tree instead of failing the overlay
> application?
>
>>  	} else {
>>  		new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
>>  	}
>
> [Severity: Critical]
> This is a pre-existing issue, but while reviewing add_changeset_property(), I
> noticed a problem further down in the same function.
>
> When a property is added to a newly created node, it is linked into the
> node's deadprops list before the changeset operation:
>
> 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 (ret)
> 		__of_prop_free(new_prop);
> 	return ret;
> }
>
> If of_changeset_add_property() fails, perhaps due to an allocation error,
> __of_prop_free() is called on new_prop, but new_prop is not unlinked from
> target->np->deadprops.
>
> Will this leave a dangling pointer in the deadprops list, causing a double
> free later when the changeset is aborted and of_node_release() calls
> property_list_free() on node->deadprops?

Will address in v2

Re: [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert

[Abdurrahman Hussain]

On Mon Jul 20, 2026 at 12:10 AM PDT, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The test claims to exercise patch 2's logic but bypasses it by passing NULL for target_base.
> - [Medium] The test claims to exercise patch 3's logic but bypasses it by using an absolute live-tree path.
> --
>
> --- Patch [4]: [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert ---
> commit 1237920f0bd1d803a084dc2f03f9e63aaf5cd6a2
> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
>
> of: unittest: cover /aliases updates from overlay apply/revert
>
> This commit adds an overlay and a test runner to verify alias creation and
> removal dynamically during device tree overlay apply and revert operations.
> It asserts that the alias is correctly added and then removed.
>
>>     exercises all three functional patches earlier in this series
>>     together: patch 1's reconfig notifier is what mutates aliases_lookup
>>     on apply/revert, patch 2 is what lets target-path="/aliases" resolve
>>     to the DT root when target_base is non-NULL, and patch 3 is what
>>     rewrites the fragment-internal path in the alias value so
>>     of_find_node_by_path() finds the live-tree target. Any one of the
>>     three missing turns the middle assertion (get_id -> 99) into
>>     -ENODEV.
>
> [Severity: Medium]
> Is this description of the test coverage accurate?
>
> As detailed below, the test appears to bypass the logic for both patch 2 and
> patch 3. The overlay uses an absolute path instead of an intra-overlay
> reference (bypassing patch 3), and overlay_data_apply() passes NULL for
> target_base (bypassing patch 2).
>
>> diff --git a/drivers/of/unittest-data/overlay_alias.dtso b/drivers/of/unittest-data/overlay_alias.dtso
>> new file mode 100644
>> index 0000000000000..32532c80505a7
>> --- /dev/null
>> +++ b/drivers/of/unittest-data/overlay_alias.dtso
>> @@ -0,0 +1,9 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/dts-v1/;
>> +/plugin/;
>> +
>> +/* overlay_alias - declare an alias inside an overlay */
>> +
>> +&{/aliases} {
>> +	testcase-alias99 = "/testcase-data/overlay-node/test-bus/test-unittest100";
>
> [Severity: Medium]
> Does this overlay actually test the path rewrite logic from patch 3?
>
> The testcase-alias99 property is defined using an absolute live-tree path
> rather than an intra-overlay fragment reference. Because this is already a
> valid absolute path, it seems of_alias_create() will successfully resolve it
> via of_find_node_by_path() even if the rewrite logic from patch 3 is
> missing.
>
>> +};
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index e255f54f4d760..a9f4a0bb30872 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
> [ ... ]
>> @@ -3475,6 +3475,53 @@ static struct notifier_block of_nb = {
>>  	.notifier_call = of_notify,
>>  };
>>  
>> +static void __init of_unittest_overlay_alias(void)
>> +{
>> +	const char *path = "/testcase-data/overlay-node/test-bus/test-unittest100";
>> +	struct device_node *np;
>> +	int ovcs_id = 0;
>> +	int id, ret;
>> +
>> +	np = of_find_node_by_path(path);
>> +	if (!np) {
>> +		unittest(0, "could not find %s for alias test\n", path);
>> +		return;
>> +	}
>> +
>> +	id = of_alias_get_id(np, "testcase-alias");
>> +	if (id != -ENODEV) {
>> +		unittest(0,
>> +			 "of_alias_get_id() = %d before overlay, expected -ENODEV\n",
>> +			 id);
>> +		goto out;
>> +	}
>> +
>> +	ret = overlay_data_apply("overlay_alias", &ovcs_id);
>
> [Severity: Medium]
> Does this test actually exercise the target_base logic from patch 2?
>
> Looking at overlay_data_apply(), it calls of_overlay_fdt_apply() with a
> hardcoded NULL for target_base:
>
> drivers/of/unittest.c:overlay_data_apply() {
>     ...
>     ret = of_overlay_fdt_apply(info->dtbo_begin, size, &info->ovcs_id,
>                                NULL);
>     ...
> }
>
> When target_base is NULL, the pre-patch-2 code already resolves
> target-path="/aliases" correctly via of_find_node_by_path(). This means the
> test might pass and bypass patch 2 entirely, missing the intended test
> coverage.
>
>> +	if (!ret) {
>> +		unittest(0, "overlay_alias apply failed\n");
>> +		goto out;
>> +	}
> [ ... ]

Will address in v2.

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

[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

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

[sashiko-bot]

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