Devicetree
 help / color / mirror / Atom feed
* [PATCH v4 0/6] of: teach overlay code to keep /aliases in sync
@ 2026-07-22  1:01 Abdurrahman Hussain
  2026-07-22  1:01 ` [PATCH v4 1/6] of: resolve alias-prefixed paths under devtree_lock Abdurrahman Hussain
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Abdurrahman Hussain @ 2026-07-22  1:01 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.

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 2) is a reconfig notifier that mirrors /aliases
property changes into aliases_lookup. Patch 1 closes a pre-existing
race in the alias path lookup that becomes load-bearing once /aliases
is dynamic. Three smaller overlay-code changes (patches 3-5) 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 closes a pre-existing race in of_find_node_opts_by_path():
the alias-resolution walk reads of_aliases and iterates its property
list with no lock held and no reference taken — racing property
surgery done under devtree_lock, and the node's own teardown once
/aliases can be created and destroyed at runtime. The walk now runs
under devtree_lock with a reference held across the live-tree lookup.

Patch 2 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
(kstrdup'd alias name, of_node_get'd target) from memblock-backed
(boot-time) ones so the remove path can't kfree the wrong storage.
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. All aliases_lookup readers and writers
serialize on a dedicated aliases_mutex.

Patch 3 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 4 converts dup_and_fixup_symbol_prop() to return ERR_PTR so
callers can tell malformed values, not-overlay-internal paths, and
allocation failures apart — /__symbols__ errors stop being reported
as -ENOMEM regardless of cause.

Patch 5 rewrites /aliases property values from the overlay's internal
fragment path ("/<fragment>/__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 2's notifier receives paths that never resolve in the
live tree, so of_alias_get_id() still returns -ENODEV. Values that
don't resolve inside the overlay (legacy string aliases, malformed
junk) are copied verbatim and stay inert — consumers validate before
dereferencing — so no overlay that applied before this series stops
applying.

Patch 6 adds a unittest — overlay_alias.dtso plus
of_unittest_overlay_alias() — that applies an overlay with a
non-NULL target_base, declaring a labeled node under the base and an
`testcase-alias99 = &that-label` entry in DT-root /aliases, then
asserts of_alias_get_id() on the grafted node returns 99 after the
apply and of_alias_get_highest_id() reports the stem gone after the
revert. Missing any one of the three functional patches (notifier /
absolute-target-paths / alias path rewrite) causes the assertion to
fail.

Changes in v4
-------------

Addresses the automated review of v3, plus findings from a local
adversarial review pass over the v4 candidate.

New patch 1:
  - of_find_node_opts_by_path() walked of_aliases' property list with
    no lock and no node reference. Racy against devtree_lock-protected
    property surgery since the walk was introduced, but only reachable
    in practice once overlays can create and destroy /aliases.
    Prerequisite for the DETACH change below.
  - The reader also validates the value's shape (of_alias_value_ok())
    before dereferencing it as a C string — raw of_add_property()
    producers are not validated anywhere — and no longer passes the
    NULL value of an empty alias property to of_find_node_by_path().

Patch 2 (was patch 1):
  - DETACH_NODE now drops the reference taken at ATTACH_NODE after
    clearing the of_aliases pointer under devtree_lock. v3 kept the
    reference to protect lockless readers, but a kept reference
    violates the changeset-destroy contract —
    __of_changeset_entry_destroy() expects refcount 1 — so every
    apply/revert cycle of an /aliases-creating overlay logged
    "ERROR: memory leak" and permanently leaked the node. With the
    reader from patch 1 taking its own reference under devtree_lock,
    the put is safe.
  - The aliases_lookup sweep now runs only when the detached node is
    the tracked of_aliases, so a stray duplicate root node named
    "aliases" can no longer wipe entries backed by the real node; the
    node match is an exact-name compare (of_node_is_aliases()) since
    of_node_name_eq() would also match "aliases@1".
  - The notifier machinery is compiled only for CONFIG_OF_DYNAMIC;
    the of_reconfig_notifier_register() stub returns -EINVAL, so the
    unconditional core_initcall_sync failed on every non-dynamic DT
    kernel.
  - Per-entry teardown factored into __of_alias_del(); shared
    of_node_is_aliases()/of_alias_value_ok() helpers in of_private.h
    replace open-coded copies in base.c and overlay.c.

New patch 4:
  - dup_and_fixup_symbol_prop() returns ERR_PTR, distinguishing
    malformed values (-EINVAL), not-overlay-internal paths (-ENODEV),
    and allocation failure (-ENOMEM) — the discrimination v3's
    "/fragment@" prefix heuristic approximated, and the reason a
    structural failure was previously misreported as -ENOMEM.

Patch 5 (was patch 3):
  - Drop the "/fragment@" prefix heuristic: init_overlay_changeset()
    accepts fragments with any node name, so a hand-named fragment's
    alias would silently stay unrewritten (the bug this series fixes),
    and a live-tree path starting with "/fragment@" would fail apply.
    Discrimination now comes from where the value actually resolves
    (patch 4).
  - Malformed alias values no longer fail the overlay apply with
    -EINVAL; they are admitted verbatim with a warning and stay inert
    (consumers validate before dereferencing). An overlay that applied
    cleanly before this series keeps applying.
  - Exempt pseudo-properties from the /aliases handling: the
    is_pseudo_property() skip at the top of add_changeset_property()
    is gated on target->in_livetree, so a phandle (a raw cell, not a
    C string) of a newly created /aliases node could fail the string
    checks and abort a valid overlay.

Patch 6 (was patch 4):
  - Drop the grafted-node reference before of_overlay_remove();
    holding it across the revert trips the same changeset-destroy
    refcount check (kernel ERROR + leaked node).
  - Assert alias removal via of_alias_get_highest_id() — keyed by
    stem, needs no node reference — with a matching precondition
    check before the apply.
  - Wrap the apply in EXPECT_BEGIN/END for the standard "memory leak
    will occur" warning printed for properties added to a node the
    overlay didn't create.

Patch 3:
  - Document the empty-vs-absolute target-path contract in the
    kernel-doc for @base/@target_base and find_target(); the old text
    predated the semantic change.

Link to v3: https://patch.msgid.link/20260721-nh-of-alias-overlay-v3-0-7001028fe2f5@nexthop.ai

Changes in v3
-------------

Addresses the automated review of v2.

Patch 1:
  - of_device_uevent() in drivers/of/device.c now walks aliases_lookup
    under aliases_mutex; the previous of_mutex path could race with
    the reconfig notifier that mutates under aliases_mutex.
  - Drop the defensive ATTACH_NODE property scan. Overlay attach
    queues each property as a separate ADD_PROPERTY changeset entry,
    so per-property notifications cover the case that matters;
    walking rd->dn->properties without devtree_lock could race with
    concurrent DT mutations. A direct of_attach_node() caller that
    pre-populates /aliases loses tracking for those entries, matching
    pre-series behavior.
  - Replace DETACH_NODE's per-property forget with a single scan of
    aliases_lookup itself (same rationale). Boot-time entries are
    unlinked; memblock-backed struct + alias name are not freed.
  - of_alias_destroy() now of_node_puts() the target for every
    matching entry, closing a leak of the reference
    of_find_node_by_path() took at of_alias_scan() time for boot-time
    entries.

Patch 3:
  - Enforce non-empty + null-terminated-within-pp->length up-front
    for every /aliases property value. Values that don't match
    "/fragment@" now flow through __of_prop_dup() only after that
    validation, closing a pre-existing hole where a malformed alias
    value could reach the live tree.

Patch 4:
  - Hold a reference on the grafted target node across the revert
    so the post-remove assertion queries the right np, not the
    base. of_alias_get_id(base, ...) would trivially return -ENODEV
    regardless of whether removal actually cleaned up the alias.
  - On of_overlay_fdt_apply() failure, reclaim the (possibly
    partially-populated) ovcs_id via of_overlay_remove() to avoid
    leaking the unflattened FDT and partially-applied nodes.

Additional fixes from local review of the v3 candidate:

All patches:
  - Trim inline comments to the constraint being enforced; rationale
    and use-case narrative moved to the commit messages.

Patch 1:
  - of_alias_create() releases the target-node reference on every
    error path, not only for owned entries — restoring the original
    of_alias_scan() error behavior for boot-time entries.
  - Correct the struct alias_prop @owned kernel-doc: every entry holds
    a reference on @np (removal drops it regardless of @owned); the
    flag only selects whether the struct and alias name are kfree()d.
  - Correct the notifier-registration comment and commit message:
    of_alias_scan() runs pre-initcall from unflatten_device_tree() /
    of_pdt_build_devicetree(), not from of_core_init().
  - DETACH_NODE clears the of_aliases pointer but keeps the reference
    taken at ATTACH_NODE: of_find_node_opts_by_path() reads of_aliases
    and walks its properties locklessly, so the node must outlive any
    concurrent reader.
  - Call out (in the commit message) the pre-existing one-byte OOB
    read in the stem parser that the refactor fixes: isdigit() was
    tested before the end > start bound.

Patch 3:
  - Correct the commit-message claim that a NULL return from
    dup_and_fixup_symbol_prop() can only mean -ENOMEM; unresolvable
    fragment paths also return NULL and are reported as -ENOMEM,
    matching the existing /__symbols__ handling.

Patch 4:
  - Fix the .dtso label: dtc labels cannot contain hyphens, so
    testcase-target failed to compile. Now testcase_target.
  - Do not add overlay_alias.dtbo to the fdtoverlay static build
    test: fdtoverlay cannot resolve the empty (base-relative)
    target-path, so static_test_1.dtb would fail to build.
  - Drop the unnecessary #address-cells/#size-cells from the grafted
    node (dtc W=1 avoid_unnecessary_addr_size).

Changes in v2
-------------

Addressed the automated review of the RFC posting.

Patch 1:
  - Guard rd->old_prop before dereferencing in the UPDATE_PROPERTY
    handler; some notifier producers pass NULL.
  - Handle OF_RECONFIG_ATTACH_NODE and OF_RECONFIG_DETACH_NODE for
    /aliases nodes attached or detached with pre-populated properties.
    (v3 pared this back — see above.)
  - Serialize aliases_lookup with a dedicated aliases_mutex; readers
    (of_alias_get_id, of_alias_get_highest_id) and the reconfig
    notifier both hold it. Boot-time of_alias_scan() stays lockless
    (single-threaded during init).
  - Destroy path unlinks matching entries irrespective of ownership
    (freeing storage only for owned ones), so an overlay UPDATE
    against a boot-time alias no longer leaves duplicate stem+id
    entries in aliases_lookup.
  - Owned entries kstrdup the alias name and hold an of_node_get()
    reference on the target — released in the destroy path — so a
    property freed by overlay revert can't dangle into aliases_lookup.
  - of_aliases is refcounted lazily on ATTACH/DETACH.
  - Validate pp->value is non-empty and null-terminated within
    pp->length before feeding it to of_find_node_by_path() — prevents
    a stray malformed alias entry from causing an OOB read.

Patch 3:
  - Only route /aliases properties through dup_and_fixup_symbol_prop()
    when the value looks like a fragment-internal path. Other alias
    values pass through the raw duplicator unchanged. This removes
    the previous unconditional fallback that would silently propagate
    a raw dup on ENOMEM.

Patch 4:
  - Rewrite the .dtso to actually exercise all three functional
    patches: a labeled node under a fragment with target-path="" plus
    an /aliases fragment with target-path="/aliases" (absolute) and
    a value referencing the label via `&`. The RFC test bypassed
    patches 2 and 3.
  - Rewrite the runner to call of_overlay_fdt_apply() directly with a
    non-NULL target_base and assert against the grafted node's np.

Link to v1: https://patch.msgid.link/20260720-nh-of-alias-overlay-v1-0-27da6848dd84@nexthop.ai

Open items
----------

None outstanding. Pre-existing issues surfaced by the automated review
are unrelated to this series and are being tracked separately:
  - fragment target/overlay refcount leak on init_overlay_changeset()
    error paths (ovcs->count still 0 when free_overlay_changeset()
    runs),
  - deadprops double-free in add_changeset_property() when
    of_changeset_add_property() fails after the property was linked
    into target->np->deadprops,
  - node duplicated by __of_node_dup() leaked in add_changeset_node()
    when of_changeset_attach_node() fails,
  - of_node_put() under devtree_lock (raw spinlock) reaching
    fwnode_links_purge(),
  - lan966x_pci_probe() not reclaiming a populated ovcs_id when
    of_overlay_fdt_apply() fails.

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

The OF unittest was run under QEMU (x86_64, OF_UNITTEST=y, v7.2-rc3
plus this series): of_unittest_overlay_alias() passes and the full
run reports 415 passed, 0 failed, with no unexpected "OF: ERROR:"
lines or changeset-destroy refcount complaints in the log. base.c
also builds with CONFIG_OF=y and CONFIG_OF_DYNAMIC=n (the notifier
machinery compiles out).

The series was also backported onto a 7.1-based image and boot-tested
on an x86 Nexthop switch with two PCI-attached Xilinx FPGAs whose
i2c-xiic controllers come from driver-embedded DT overlays applied
with a non-NULL target base. 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: 95 i2c
adapters with /dev/i2c-N numbering matching the overlay aliases
exactly and stable across reboots, sensors reading live telemetry
through mux channels, and dmesg free of WARN/BUG/Oops and unexpected
OF errors. Runtime overlay revert via driver unbind was exercised on
the same hardware: the changeset revert removed all 153 of the
overlay's /aliases entries through the reconfig notifier with no
refcount or of_node_put errors, leaving the other overlay's 12
entries intact. (The revert then hangs in device teardown, in a
pre-existing i2c-xiic i2c_del_adapter() issue unrelated to this
series; the complete apply/revert cycle is covered by the unittest
under QEMU.)

[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

Assisted-by: Claude:claude-fable-5 [Claude Code]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
Abdurrahman Hussain (6):
      of: resolve alias-prefixed paths under devtree_lock
      of: incrementally update /aliases lookup on reconfig notifications
      of/overlay: look up absolute target-paths absolutely
      of/overlay: return ERR_PTR from dup_and_fixup_symbol_prop()
      of/overlay: rewrite /aliases path values to live-tree paths
      of: unittest: cover /aliases updates from overlay apply/revert

 drivers/of/base.c                           | 236 ++++++++++++++++++++++------
 drivers/of/device.c                         |   4 +-
 drivers/of/of_private.h                     |  21 +++
 drivers/of/overlay.c                        |  71 +++++----
 drivers/of/unittest-data/Makefile           |   5 +
 drivers/of/unittest-data/overlay_alias.dtso |  25 +++
 drivers/of/unittest.c                       |  73 +++++++++
 7 files changed, 358 insertions(+), 77 deletions(-)
---
base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
change-id: 20260719-nh-of-alias-overlay-6e5e0d56212b

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v4 1/6] of: resolve alias-prefixed paths under devtree_lock
  2026-07-22  1:01 [PATCH v4 0/6] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
@ 2026-07-22  1:01 ` Abdurrahman Hussain
  2026-07-22  1:19   ` sashiko-bot
  2026-07-22  7:30   ` Krzysztof Kozlowski
  2026-07-22  1:01 ` [PATCH v4 2/6] of: incrementally update /aliases lookup on reconfig notifications Abdurrahman Hussain
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 11+ messages in thread
From: Abdurrahman Hussain @ 2026-07-22  1:01 UTC (permalink / raw)
  To: Rob Herring, Saravana Kannan
  Cc: devicetree, linux-kernel, Abdurrahman Hussain

of_find_node_opts_by_path() resolves an alias-prefixed path by
walking the property list of the of_aliases node with no lock held
and no reference taken on the node. Property surgery on /aliases —
of_add_property(), of_remove_property(), changeset apply/revert —
mutates that list under devtree_lock, so the lockless walk can step
into a property that is being unlinked. Nothing keeps the node itself
alive across the walk either: detaching /aliases and dropping the
last reference frees the node and its property list mid-iteration.

The race has existed since the walk was introduced, but is hard to
hit with a boot-FDT /aliases node that nothing ever detaches. The
rest of this series makes /aliases dynamic — overlays can add and
remove properties, and create and destroy the node itself — so close
it first: find the matching property under devtree_lock with a
reference held on of_aliases, and keep that reference across the
of_find_node_by_path() call that resolves the value. The reference is
what keeps the value string valid outside the lock: a concurrently
removed property moves to the node's deadprops list and is only freed
when the node itself is released.

While here, validate the value's shape before handing it to
of_find_node_by_path(): /aliases contents can now come from overlays
and from raw changeset/of_add_property() callers, and only the
overlay path validates at the producer. of_alias_value_ok() (shared
with the alias tracking added later in this series) rejects values
that are not non-empty C strings NUL-terminated within the property
length, so the consumer no longer trusts any producer. This also
covers the empty property (NULL value) that previously crashed in
strchr().

The name match is folded into one bounded strncmp plus a check of the
terminating NUL instead of strlen + strncmp, halving the string
traversal now done with interrupts disabled.

Assisted-by: Claude:claude-fable-5 [Claude Code]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/of/base.c       | 26 ++++++++++++++++++--------
 drivers/of/of_private.h |  7 +++++++
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 6e7a42dedad3..9c2770823889 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -995,6 +995,8 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
 
 	/* The path could begin with an alias */
 	if (*path != '/') {
+		struct device_node *aliases;
+		const char *value = NULL;
 		int len;
 		const char *p = strchrnul(path, '/');
 
@@ -1002,16 +1004,24 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
 			p = separator;
 		len = p - path;
 
-		/* of_aliases must not be NULL */
-		if (!of_aliases)
-			return NULL;
-
-		for_each_property_of_node(of_aliases, pp) {
-			if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
-				np = of_find_node_by_path(pp->value);
-				break;
+		raw_spin_lock_irqsave(&devtree_lock, flags);
+		aliases = of_node_get(of_aliases);
+		if (aliases) {
+			for_each_property_of_node(aliases, pp) {
+				if (!strncmp(pp->name, path, len) &&
+				    !pp->name[len]) {
+					if (of_alias_value_ok(pp))
+						value = pp->value;
+					break;
+				}
 			}
 		}
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+		/* the reference on @aliases keeps @value alive */
+		if (value)
+			np = of_find_node_by_path(value);
+		of_node_put(aliases);
 		if (!np)
 			return NULL;
 		path = p;
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 0ae16da066e2..792756389691 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -215,6 +215,13 @@ static inline bool is_pseudo_property(const char *prop_name)
 		!of_prop_cmp(prop_name, "linux,phandle");
 }
 
+/* alias values are deref'd as C strings; they must terminate within length */
+static inline bool of_alias_value_ok(const struct property *pp)
+{
+	return pp->value && pp->length >= 2 &&
+	       strnlen(pp->value, pp->length) < pp->length;
+}
+
 #if IS_ENABLED(CONFIG_KUNIT)
 int __of_address_resource_bounds(struct resource *r, u64 start, u64 size);
 #endif

-- 
2.54.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 2/6] of: incrementally update /aliases lookup on reconfig notifications
  2026-07-22  1:01 [PATCH v4 0/6] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
  2026-07-22  1:01 ` [PATCH v4 1/6] of: resolve alias-prefixed paths under devtree_lock Abdurrahman Hussain
@ 2026-07-22  1:01 ` Abdurrahman Hussain
  2026-07-22  1:01 ` [PATCH v4 3/6] of/overlay: look up absolute target-paths absolutely Abdurrahman Hussain
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Abdurrahman Hussain @ 2026-07-22  1:01 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 that mirrors
/aliases changes into aliases_lookup. Registration happens at
core_initcall_sync time, safely after the boot-time of_alias_scan(),
which runs pre-initcall from unflatten_device_tree() (or
of_pdt_build_devicetree() on OF-real platforms):

  OF_RECONFIG_ADD_PROPERTY    -> of_alias_create()
  OF_RECONFIG_REMOVE_PROPERTY -> of_alias_destroy()
  OF_RECONFIG_UPDATE_PROPERTY -> destroy + create
  OF_RECONFIG_ATTACH_NODE     -> adopt the node as of_aliases
  OF_RECONFIG_DETACH_NODE     -> drop every aliases_lookup entry

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 (exact name "aliases",
parent == root, via the shared of_node_is_aliases()) 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 name compare is exact rather than of_node_name_eq():
the latter ignores unit addresses and would also match a root node
named "aliases@1", which neither path lookup nor the DT spec treats
as the aliases node. ATTACH publishes the adopted node
in of_aliases with a reference held; DETACH clears the pointer and
drops that reference again. Both pointer updates happen under
devtree_lock, pairing with the locked reader in
of_find_node_opts_by_path() from the previous patch — a reader either
observes NULL or takes its own reference before the notifier's put
can be the last one. Dropping the reference at DETACH also keeps the
node at refcount 1 by the time an overlay changeset that created
/aliases is destroyed, which __of_changeset_entry_destroy() insists
on before it lets the node be freed.

Only per-property notifications populate aliases_lookup —
the notifier does not walk the attached node's property list, which
would race with devtree_lock-protected property mutations. A direct
of_attach_node() caller that pre-populates /aliases is not tracked,
matching pre-series behavior.

DETACH_NODE conversely drops every aliases_lookup entry — but only
when the detached node is the tracked of_aliases. __of_attach_node()
has no duplicate-name check, so a stray second root node named
"aliases" can exist; detaching it must not wipe entries backed by the
real node. Walking aliases_lookup itself (under aliases_mutex) avoids
the same property-list race. Overlay revert additionally emits per-
property REMOVE events beforehand; the sweep catches direct
of_detach_node() callers that don't. The per-entry teardown is
factored into __of_alias_del(), shared by the single-name destroy and
the detach-time sweep.

One ordering caveat is inherent to the notification architecture:
within a single changeset, a device created by an earlier ATTACH
entry can be probed by of_platform_notify() before a later /aliases
ADD_PROPERTY entry reaches this notifier. Overlays that declare an
alias for a node they also create should order the /aliases fragment
first if the target bus is populated with a bound driver at apply
time.

The notifier machinery is built only for CONFIG_OF_DYNAMIC kernels:
without it no reconfig notifications exist and
of_reconfig_notifier_register() is a stub returning -EINVAL, so an
unconditional registration would fail the initcall on every
non-dynamic DT kernel.

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
so the alias_prop survives the property that spawned it — required
for the overlay revert path where the source property is freed. 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. Every entry, boot-time or runtime, holds the target-node
reference that of_find_node_by_path() returned at create time;
destroy drops it symmetrically.

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
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, of_device_uevent) and the
reconfig notifier 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.

The refactor also fixes a pre-existing one-byte out-of-bounds read in
the stem parser: the old loop tested isdigit(*(end - 1)) before
checking end > start, reading one byte before the property name when
the name is empty or all digits. of_alias_create() checks the bound
first and rejects a zero-length stem.

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]
Assisted-by: Claude:claude-fable-5 [Claude Code]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/of/base.c       | 210 +++++++++++++++++++++++++++++++++++++++---------
 drivers/of/device.c     |   4 +-
 drivers/of/of_private.h |  14 ++++
 3 files changed, 186 insertions(+), 42 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 9c2770823889..669eed7d03cf 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1925,6 +1925,170 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
 		 ap->alias, ap->stem, ap->id, np);
 }
 
+/*
+ * Protects aliases_lookup and of_aliases. of_alias_scan() runs single-
+ * threaded at init and skips it; every other reader/writer must hold it.
+ */
+DEFINE_MUTEX(aliases_mutex);
+
+/* Callers other than of_alias_scan() must hold @aliases_mutex. */
+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;
+
+	if (!of_alias_value_ok(pp))
+		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);
+
+	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:
+	of_node_put(np);
+}
+
+#ifdef CONFIG_OF_DYNAMIC
+/* Unlink @ap; free its storage if it was runtime-allocated. */
+static void __of_alias_del(struct alias_prop *ap)
+{
+	list_del(&ap->link);
+	of_node_put(ap->np);
+	if (ap->owned) {
+		kfree(ap->alias);
+		kfree(ap);
+	}
+}
+
+/* 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;
+		__of_alias_del(ap);
+		return;
+	}
+}
+
+static void *alias_alloc(u64 size, u64 align)
+{
+	return kzalloc(size, GFP_KERNEL);
+}
+
+/* Callers must hold @aliases_mutex. */
+static void of_aliases_forget_all(void)
+{
+	struct alias_prop *ap, *tmp;
+
+	list_for_each_entry_safe(ap, tmp, &aliases_lookup, link)
+		__of_alias_del(ap);
+}
+
+static int of_aliases_reconfig_notifier(struct notifier_block *nb,
+					unsigned long action, void *arg)
+{
+	struct of_reconfig_data *rd = arg;
+	struct device_node *put = NULL;
+	unsigned long flags;
+
+	/* of_aliases may still be NULL when an overlay creates the node */
+	if (!rd->dn || !of_node_is_aliases(rd->dn))
+		return NOTIFY_DONE;
+
+	mutex_lock(&aliases_mutex);
+	switch (action) {
+	case OF_RECONFIG_ATTACH_NODE:
+		/* of_aliases is read under devtree_lock by alias path lookup */
+		raw_spin_lock_irqsave(&devtree_lock, flags);
+		if (!of_aliases)
+			of_aliases = of_node_get(rd->dn);
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
+		break;
+	case OF_RECONFIG_DETACH_NODE:
+		raw_spin_lock_irqsave(&devtree_lock, flags);
+		if (of_aliases == rd->dn) {
+			of_aliases = NULL;
+			put = rd->dn;
+		}
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
+		if (put) {
+			of_aliases_forget_all();
+			/* may free the node, so must sit outside devtree_lock */
+			of_node_put(put);
+		}
+		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 pre-initcall, so the boot-time scan is complete */
+core_initcall_sync(of_aliases_reconfig_init);
+#endif /* CONFIG_OF_DYNAMIC */
+
 /**
  * of_alias_scan - Scan all properties of the 'aliases' node
  * @dt_alloc:	An allocator that provides a virtual address to memory
@@ -1960,42 +2124,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);
 }
 
 /**
@@ -2013,7 +2143,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;
@@ -2023,7 +2153,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;
 }
@@ -2041,7 +2171,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;
@@ -2049,7 +2179,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/device.c b/drivers/of/device.c
index b3dc78f2fa3a..fa0cc8129ab0 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -237,7 +237,7 @@ void of_device_uevent(const struct device *dev, struct kobj_uevent_env *env)
 	add_uevent_var(env, "OF_COMPATIBLE_N=%d", seen);
 
 	seen = 0;
-	mutex_lock(&of_mutex);
+	mutex_lock(&aliases_mutex);
 	list_for_each_entry(app, &aliases_lookup, link) {
 		if (dev->of_node == app->np) {
 			add_uevent_var(env, "OF_ALIAS_%d=%s", seen,
@@ -245,7 +245,7 @@ void of_device_uevent(const struct device *dev, struct kobj_uevent_env *env)
 			seen++;
 		}
 	}
-	mutex_unlock(&of_mutex);
+	mutex_unlock(&aliases_mutex);
 }
 EXPORT_SYMBOL_GPL(of_device_uevent);
 
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 792756389691..604a0b0dfda8 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -17,6 +17,11 @@
  * @alias:	Alias property name
  * @np:		Pointer to device_node that the alias stands for
  * @id:		Index value from end of alias name
+ * @owned:	True for runtime entries, where the struct and @alias are
+ *		kmalloc'd/kstrdup'd and freed on removal. False for boot-time
+ *		entries, which live in memblock (@alias points into the FDT)
+ *		and are only unlinked. Every entry holds a reference on @np;
+ *		removal drops it regardless of @owned.
  * @stem:	Alias string without the index
  *
  * The structure represents one alias property of 'aliases' node as
@@ -27,6 +32,7 @@ struct alias_prop {
 	const char *alias;
 	struct device_node *np;
 	int id;
+	bool owned;
 	char stem[];
 };
 
@@ -40,6 +46,7 @@ struct alias_prop {
 
 extern struct mutex of_mutex;
 extern raw_spinlock_t devtree_lock;
+extern struct mutex aliases_mutex;
 extern struct list_head aliases_lookup;
 extern struct kset *of_kset;
 
@@ -222,6 +229,13 @@ static inline bool of_alias_value_ok(const struct property *pp)
 	       strnlen(pp->value, pp->length) < pp->length;
 }
 
+/* the /aliases node: root child with the exact name "aliases" */
+static inline bool of_node_is_aliases(const struct device_node *np)
+{
+	return of_node_is_root(np->parent) &&
+	       !strcmp(kbasename(np->full_name), "aliases");
+}
+
 #if IS_ENABLED(CONFIG_KUNIT)
 int __of_address_resource_bounds(struct resource *r, u64 start, u64 size);
 #endif

-- 
2.54.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 3/6] of/overlay: look up absolute target-paths absolutely
  2026-07-22  1:01 [PATCH v4 0/6] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
  2026-07-22  1:01 ` [PATCH v4 1/6] of: resolve alias-prefixed paths under devtree_lock Abdurrahman Hussain
  2026-07-22  1:01 ` [PATCH v4 2/6] of: incrementally update /aliases lookup on reconfig notifications Abdurrahman Hussain
@ 2026-07-22  1:01 ` Abdurrahman Hussain
  2026-07-22  1:01 ` [PATCH v4 4/6] of/overlay: return ERR_PTR from dup_and_fixup_symbol_prop() Abdurrahman Hussain
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Abdurrahman Hussain @ 2026-07-22  1:01 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).

Spell the new contract out in the kernel-doc for @base and
@target_base and in find_target()'s strategy comment, so out-of-tree
callers that modeled a base-relative target-path on the old
concatenation behavior have a documented signal that the semantics
changed.

Assisted-by: Claude:claude-fable-5 [Claude Code]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/of/overlay.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 08d5351746be..74aea704835a 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -688,12 +688,15 @@ static int build_changeset(struct overlay_changeset *ovcs)
  *
  * 1) "target" property containing the phandle of the target
  * 2) "target-path" property containing the path of the target
+ *
+ * With a non-NULL @target_base, an empty "target-path" means
+ * @target_base itself; any non-empty "target-path" is resolved
+ * absolutely from the live-tree root.
  */
 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 +712,14 @@ 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);
-			}
-		}
+		/* an empty target-path means the target base itself */
+		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;
 	}
 
@@ -737,7 +731,9 @@ static struct device_node *find_target(const struct device_node *info_node,
 /**
  * init_overlay_changeset() - initialize overlay changeset from overlay tree
  * @ovcs:		Overlay changeset to build
- * @target_base:	Point to the target node to apply overlay
+ * @target_base:	Target for fragments with an empty "target-path";
+ *			fragments with a non-empty "target-path" resolve
+ *			absolutely and ignore @target_base
  *
  * Initialize @ovcs.  Populate @ovcs->fragments with node information from
  * the top level of @overlay_root.  The relevant top level nodes are the
@@ -982,7 +978,9 @@ static int of_overlay_apply(struct overlay_changeset *ovcs,
  * @overlay_fdt:	pointer to overlay FDT
  * @overlay_fdt_size:	number of bytes in @overlay_fdt
  * @ret_ovcs_id:	pointer for returning created changeset id
- * @base:		pointer for the target node to apply overlay
+ * @base:		target for fragments with an empty "target-path";
+ *			fragments with a non-empty "target-path" resolve
+ *			absolutely and ignore @base
  *
  * Creates and applies an overlay changeset.
  *

-- 
2.54.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 4/6] of/overlay: return ERR_PTR from dup_and_fixup_symbol_prop()
  2026-07-22  1:01 [PATCH v4 0/6] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
                   ` (2 preceding siblings ...)
  2026-07-22  1:01 ` [PATCH v4 3/6] of/overlay: look up absolute target-paths absolutely Abdurrahman Hussain
@ 2026-07-22  1:01 ` Abdurrahman Hussain
  2026-07-22  1:13   ` sashiko-bot
  2026-07-22  1:01 ` [PATCH v4 5/6] of/overlay: rewrite /aliases path values to live-tree paths Abdurrahman Hussain
  2026-07-22  1:01 ` [PATCH v4 6/6] of: unittest: cover /aliases updates from overlay apply/revert Abdurrahman Hussain
  5 siblings, 1 reply; 11+ messages in thread
From: Abdurrahman Hussain @ 2026-07-22  1:01 UTC (permalink / raw)
  To: Rob Herring, Saravana Kannan
  Cc: devicetree, linux-kernel, Abdurrahman Hussain

dup_and_fixup_symbol_prop() returns NULL for three very different
reasons: the property value is not a valid non-empty C string
(malformed input), the value does not resolve to a node inside one of
the overlay's fragments (not an overlay-internal path), and memory
allocation failure. Its only caller today reports every NULL as
-ENOMEM, so structural problems in an overlay's /__symbols__ node are
diagnosed as memory exhaustion.

The next patch reuses the helper for /aliases values, where the
distinction is load-bearing: a value that doesn't resolve inside the
overlay is a legacy alias that must be copied verbatim, a malformed
value must be admitted inertly with a warning, and only a real
allocation failure may fail the overlay apply.

Return ERR_PTR instead: -EINVAL for malformed values, -ENODEV when
the value is not a path into one of the overlay's fragments, -ENOMEM
for allocation failures. The /__symbols__ caller now propagates the
distinct errno instead of collapsing everything to -ENOMEM.

Assisted-by: Claude:claude-fable-5 [Claude Code]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/of/overlay.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 74aea704835a..ad4e50482515 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -206,6 +206,10 @@ static void overlay_fw_devlink_refresh(struct overlay_changeset *ovcs)
  * The duplicated property value will be modified by replacing the
  * "/fragment_name/__overlay/" portion of the value  with the target
  * path from the fragment node.
+ *
+ * Return: the fixed-up property, or ERR_PTR: -EINVAL if @prop's value
+ * is not a valid non-empty C string, -ENODEV if it is not a path into
+ * one of @ovcs's fragments, -ENOMEM on allocation failure.
  */
 static struct property *dup_and_fixup_symbol_prop(
 		struct overlay_changeset *ovcs, const struct property *prop)
@@ -224,14 +228,14 @@ static struct property *dup_and_fixup_symbol_prop(
 	int target_path_len;
 
 	if (!prop->value)
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	if (strnlen(prop->value, prop->length) >= prop->length)
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	path = prop->value;
 	path_len = strlen(path);
 
 	if (path_len < 1)
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	fragment_node = __of_find_node_by_path(ovcs->overlay_root, path + 1);
 	overlay_node = __of_find_node_by_path(fragment_node, "__overlay__/");
 	of_node_put(fragment_node);
@@ -243,18 +247,18 @@ static struct property *dup_and_fixup_symbol_prop(
 			break;
 	}
 	if (k >= ovcs->count)
-		return NULL;
+		return ERR_PTR(-ENODEV);
 
 	overlay_name_len = snprintf(NULL, 0, "%pOF", fragment->overlay);
 
 	if (overlay_name_len > path_len)
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	path_tail = path + overlay_name_len;
 	path_tail_len = strlen(path_tail);
 
 	target_path = kasprintf(GFP_KERNEL, "%pOF", fragment->target);
 	if (!target_path)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 	target_path_len = strlen(target_path);
 
 	new_prop = kzalloc_obj(*new_prop);
@@ -281,7 +285,7 @@ static struct property *dup_and_fixup_symbol_prop(
 err_free_target_path:
 	kfree(target_path);
 
-	return NULL;
+	return ERR_PTR(-ENOMEM);
 }
 
 /**
@@ -350,6 +354,8 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
 		if (prop)
 			return -EINVAL;
 		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
+		if (IS_ERR(new_prop))
+			return PTR_ERR(new_prop);
 	} else {
 		new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
 	}

-- 
2.54.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 5/6] of/overlay: rewrite /aliases path values to live-tree paths
  2026-07-22  1:01 [PATCH v4 0/6] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
                   ` (3 preceding siblings ...)
  2026-07-22  1:01 ` [PATCH v4 4/6] of/overlay: return ERR_PTR from dup_and_fixup_symbol_prop() Abdurrahman Hussain
@ 2026-07-22  1:01 ` Abdurrahman Hussain
  2026-07-22  1:16   ` sashiko-bot
  2026-07-22  1:01 ` [PATCH v4 6/6] of: unittest: cover /aliases updates from overlay apply/revert Abdurrahman Hussain
  5 siblings, 1 reply; 11+ messages in thread
From: Abdurrahman Hussain @ 2026-07-22  1:01 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 (of_node_is_aliases(), the same predicate the
reconfig notifier uses) and offer every non-pseudo property to
dup_and_fixup_symbol_prop(); the previous patch made its return value
carry the distinction this needs. A value that resolves inside one of
the overlay's fragments is stored rewritten. -ENODEV means the value
is not a path into this overlay — legacy string aliases like "ttyS0",
or absolute live-tree paths — and is copied verbatim, as before this
series. -EINVAL means the value is not a valid non-empty C string; it
is also copied verbatim, but with a warning: consumers
(of_alias_create() and the alias path lookup) validate before
dereferencing, so a malformed alias is inert rather than a reason to
reject an otherwise-valid overlay that applied cleanly before this
series. Only -ENOMEM fails the apply.

Matching on where the value resolves rather than on a "/fragment@"
name prefix matters: init_overlay_changeset() accepts fragments with
any node name, and dtc emits the actual fragment name into the alias
value, so a prefix test would silently leave a hand-named fragment's
alias unrewritten (and misroute a live-tree path that happens to
start with "/fragment@").

Pseudo-properties (name, phandle, linux,phandle) are exempt from the
/aliases handling: the is_pseudo_property() skip at the top of
add_changeset_property() only covers targets already in the live
tree, so a phandle of a newly created /aliases node would otherwise
reach the rewriter — and a phandle value is a raw cell, not a C
string. Such properties take the plain __of_prop_dup() path as
before this patch; of_alias_create() skips them at notifier time.

Assisted-by: Claude:claude-fable-5 [Claude Code]
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 ad4e50482515..2a37b5260ba7 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -356,6 +356,19 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
 		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
 		if (IS_ERR(new_prop))
 			return PTR_ERR(new_prop);
+	} else if (!is_pseudo_property(overlay_prop->name) &&
+		   of_node_is_aliases(target->np)) {
+		/* rewrite overlay-internal alias values to live-tree paths */
+		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
+		if (new_prop == ERR_PTR(-ENOMEM))
+			return -ENOMEM;
+		if (IS_ERR(new_prop)) {
+			if (new_prop == ERR_PTR(-EINVAL))
+				pr_warn("%pOF/%s is not a valid string; alias will be inert\n",
+					target->np, overlay_prop->name);
+			/* not overlay-internal: copy verbatim, consumers validate */
+			new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
+		}
 	} else {
 		new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
 	}

-- 
2.54.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4 6/6] of: unittest: cover /aliases updates from overlay apply/revert
  2026-07-22  1:01 [PATCH v4 0/6] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
                   ` (4 preceding siblings ...)
  2026-07-22  1:01 ` [PATCH v4 5/6] of/overlay: rewrite /aliases path values to live-tree paths Abdurrahman Hussain
@ 2026-07-22  1:01 ` Abdurrahman Hussain
  5 siblings, 0 replies; 11+ messages in thread
From: Abdurrahman Hussain @ 2026-07-22  1:01 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 after the apply and that
of_alias_get_highest_id() reports the stem gone after the revert,
with a matching precondition check before the apply.

The apply is wrapped in EXPECT_BEGIN/END for the "memory leak will
occur" warning that add_changeset_property() prints for any property
added to a node the overlay didn't create — /aliases here — matching
the other overlay tests that add properties to pre-existing nodes.

The grafted-node reference is dropped before of_overlay_remove():
overlay-created nodes must be back to refcount 1 when the changeset
is destroyed, or __of_changeset_entry_destroy() logs a leak error and
the node is never freed. The post-remove assertion therefore uses
of_alias_get_highest_id(), which is keyed by stem and needs no node
reference.

Exercises the 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.

The overlay is not added to the fdtoverlay static build test:
fdtoverlay has no target-base concept, so fragment@0's empty
target-path (meaning "the target base itself" at run time) cannot be
resolved statically.

Assisted-by: Claude:claude-fable-5 [Claude Code]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/of/unittest-data/Makefile           |  5 ++
 drivers/of/unittest-data/overlay_alias.dtso | 25 ++++++++++
 drivers/of/unittest.c                       | 73 +++++++++++++++++++++++++++++
 3 files changed, 103 insertions(+)

diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 01a966e39f23..fdfec5a7923b 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 \
@@ -66,6 +67,10 @@ DTC_FLAGS_testcases += -Wno-interrupts_property \
 #			  overlay_bad_add_dup_prop.dtbo \
 #			  overlay_bad_phandle.dtbo \
 #			  overlay_bad_symbol.dtbo \
+#
+# overlay_alias.dtbo needs a run-time target base for its empty target-path;
+# fdtoverlay has no equivalent, so it is also not included:
+#			  overlay_alias.dtbo \
 
 apply_static_overlay_1 := overlay_0.dtbo \
 			  overlay_1.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..17e02d1940f6
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_alias.dtso
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+/*
+ * overlay_alias - declare an /aliases entry that points, via label, at a
+ * node grafted under the run-time target base (empty target-path).
+ */
+
+/ {
+	fragment@0 {
+		target-path = "";
+		__overlay__ {
+			testcase_target: alias-target {
+			};
+		};
+	};
+
+	fragment@1 {
+		target-path = "/aliases";
+		__overlay__ {
+			testcase-alias99 = &testcase_target;
+		};
+	};
+};
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e255f54f4d76..2f5e4056efae 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -3475,6 +3475,77 @@ static struct notifier_block of_nb = {
 	.notifier_call = of_notify,
 };
 
+/* created by cmd_wrap_S_dtbo in scripts/Makefile.dtbs */
+extern uint8_t __dtbo_overlay_alias_begin[];
+extern uint8_t __dtbo_overlay_alias_end[];
+
+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";
+	u32 size = __dtbo_overlay_alias_end - __dtbo_overlay_alias_begin;
+	struct device_node *base, *target;
+	int ovcs_id = 0;
+	int id, ret;
+
+	base = of_find_node_by_path(base_path);
+	if (!base) {
+		unittest(0, "could not find alias target base %s\n", base_path);
+		return;
+	}
+
+	id = of_alias_get_highest_id("testcase-alias");
+	if (id != -ENODEV) {
+		unittest(0, "unexpected testcase-alias%d before overlay apply\n",
+			 id);
+		goto out_put_base;
+	}
+
+	EXPECT_BEGIN(KERN_INFO,
+		     "OF: overlay: WARNING: memory leak will occur if overlay removed, property: /aliases/testcase-alias99");
+
+	ret = of_overlay_fdt_apply(__dtbo_overlay_alias_begin, size,
+				   &ovcs_id, base);
+
+	EXPECT_END(KERN_INFO,
+		   "OF: overlay: WARNING: memory leak will occur if overlay removed, property: /aliases/testcase-alias99");
+
+	if (ret < 0) {
+		unittest(0, "overlay_alias apply failed, ret = %d\n", ret);
+		if (ovcs_id)
+			of_overlay_remove(&ovcs_id);
+		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);
+	} else {
+		id = of_alias_get_id(target, "testcase-alias");
+		unittest(id == 99,
+			 "of_alias_get_id() = %d after overlay apply, expected 99\n",
+			 id);
+		/* changeset destroy expects overlay nodes back at refcount 1 */
+		of_node_put(target);
+	}
+
+	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_highest_id("testcase-alias");
+	unittest(id == -ENODEV,
+		 "of_alias_get_highest_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 +3720,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();

-- 
2.54.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 4/6] of/overlay: return ERR_PTR from dup_and_fixup_symbol_prop()
  2026-07-22  1:01 ` [PATCH v4 4/6] of/overlay: return ERR_PTR from dup_and_fixup_symbol_prop() Abdurrahman Hussain
@ 2026-07-22  1:13   ` sashiko-bot
  0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-07-22  1:13 UTC (permalink / raw)
  To: Abdurrahman Hussain; +Cc: conor+dt, devicetree, robh

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

Pre-existing issues:
- [High] Double-free and use-after-free of `new_prop` on the `of_changeset_add_property()` error path.
--

--- Patch [4]: [PATCH v4 4/6] of/overlay: return ERR_PTR from dup_and_fixup_symbol_prop() ---
commit c4400996264f78e84238a0b8d2c87f7489c72180
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

of/overlay: return ERR_PTR from dup_and_fixup_symbol_prop()

dup_and_fixup_symbol_prop() previously returned NULL for malformed input,
unresolved paths, and allocation failures, which the caller collapsed into
-ENOMEM. This patch changes the function to return an ERR_PTR instead.
The caller is updated to propagate the specific errno.

> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 74aea704835ac..ad4e504825157 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c

[ ... ]

> @@ -350,6 +354,8 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>  		if (prop)
>  			return -EINVAL;
>  		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
> +		if (IS_ERR(new_prop))
> +			return PTR_ERR(new_prop);
>  	} else {
>  		new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
>  	}

[Severity: High]
This is a pre-existing issue, but could this code path lead to a double-free
on the error path for add_changeset_property()?

Looking further down in add_changeset_property(), if we are not in a livetree,
new_prop is linked into the deadprops list:

	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() fails (for example, due to memory exhaustion),
ret is non-zero, and the function frees new_prop at the bottom:

	if (ret)
		__of_prop_free(new_prop);

Does this leave the freed new_prop still attached to target->np->deadprops?

When the overlay application aborts and the unattached dynamically allocated
device node is cleaned up via of_node_release(), it iterates over the
deadprops list and calls __of_prop_free() on each property again. Can this
result in a use-after-free and double-free of the property?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-nh-of-alias-overlay-v4-0-8ad097e31e36@nexthop.ai?part=4

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 5/6] of/overlay: rewrite /aliases path values to live-tree paths
  2026-07-22  1:01 ` [PATCH v4 5/6] of/overlay: rewrite /aliases path values to live-tree paths Abdurrahman Hussain
@ 2026-07-22  1:16   ` sashiko-bot
  0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-07-22  1:16 UTC (permalink / raw)
  To: Abdurrahman Hussain; +Cc: robh, conor+dt, devicetree

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

New issues:
- [High] Valid absolute alias paths matching an overlay fragment name are silently corrupted in dup_and_fixup_symbol_prop().

Pre-existing issues:
- [High] Double free on add_changeset_property() error path for newly created nodes.
--

--- Patch [5]: [PATCH v4 5/6] of/overlay: rewrite /aliases path values to live-tree paths ---
commit b11eb3214c77837761c517184661f83716f7e731
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

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

The commit rewrites overlay-internal alias values in /aliases to their
live-tree paths using dup_and_fixup_symbol_prop(). This ensures that
alias paths are resolvable after the overlay is applied.

> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index ad4e504825157..2a37b5260ba7a 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -356,6 +356,19 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>  		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
>  		if (IS_ERR(new_prop))
>  			return PTR_ERR(new_prop);
> +	} else if (!is_pseudo_property(overlay_prop->name) &&
> +		   of_node_is_aliases(target->np)) {
> +		/* rewrite overlay-internal alias values to live-tree paths */
> +		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);

[Severity: High]
Can valid absolute alias paths be silently corrupted if they happen to match an
overlay fragment name?

When a user defines an absolute live-tree path in /aliases (e.g.,
serial0 = "/soc/serial_port@1234"), the path starts with "/soc".
dup_and_fixup_symbol_prop() calls __of_find_node_by_path(), which stops
matching at the first '/' delimiter.

If the overlay contains a fragment whose name matches this first component
(e.g. a hand-named fragment "soc"), __of_find_node_by_path() returns that
fragment node. Since the fragment contains an __overlay__ child, the function
assumes it's an overlay-internal path and computes an overlay_name_len
(e.g., 16 for "/soc/__overlay__"). It then blindly slices the string at that
offset, yielding "port@1234", and concatenates it with the fragment's
target_path.

Could this transform a valid absolute live-tree alias into a corrupted path?

> +		if (new_prop == ERR_PTR(-ENOMEM))
> +			return -ENOMEM;
> +		if (IS_ERR(new_prop)) {
> +			if (new_prop == ERR_PTR(-EINVAL))
> +				pr_warn("%pOF/%s is not a valid string; alias will be inert\n",
> +					target->np, overlay_prop->name);
> +			/* not overlay-internal: copy verbatim, consumers validate */
> +			new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
> +		}
>  	} else {
>  		new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
>  	}

[Severity: High]
This is a pre-existing issue, but is there a double free on this error path
for newly created nodes?

Looking at the code just below this in 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);

If the target node is newly created (!target->in_livetree), new_prop is
prepended to target->np->deadprops. If of_changeset_add_property() fails
(e.g. -ENOMEM), __of_prop_free() is called here.

However, new_prop remains linked in the node's deadprops list. When the
error propagates, the overlay application aborts and rolls back. The rollback
destroys the target node attachment, and of_node_release() calls
property_list_free() on the node's deadprops list, freeing new_prop a second
time.

Will this result in a double-free during node destruction?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-nh-of-alias-overlay-v4-0-8ad097e31e36@nexthop.ai?part=5

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 1/6] of: resolve alias-prefixed paths under devtree_lock
  2026-07-22  1:01 ` [PATCH v4 1/6] of: resolve alias-prefixed paths under devtree_lock Abdurrahman Hussain
@ 2026-07-22  1:19   ` sashiko-bot
  2026-07-22  7:30   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-07-22  1:19 UTC (permalink / raw)
  To: Abdurrahman Hussain; +Cc: robh, conor+dt, devicetree

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

Pre-existing issues:
- [Critical] Missing absolute path validation in alias resolution allows infinite recursion, leading to kernel stack exhaustion.
- [High] `of_node_put()` called under `devtree_lock` can trigger a sleep, causing a 'scheduling while atomic' panic.
--

--- Patch [1]: [PATCH v4 1/6] of: resolve alias-prefixed paths under devtree_lock ---
commit e85eedcd70255867d1859375b7ffdb99b3c4d887
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

of: resolve alias-prefixed paths under devtree_lock

This commit fixes a lockless walk issue in of_find_node_opts_by_path() by
finding the alias property under devtree_lock and holding a reference to the
of_aliases node. It also introduces of_alias_value_ok() to ensure the alias
value is a properly NUL-terminated string before use, preventing potential
crashes.

> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 6e7a42dedad3f..9c27708238897 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c

[ ... ]

> @@ -1002,16 +1004,24 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>  			p = separator;
>  		len = p - path;
>  
> -		/* of_aliases must not be NULL */
> -		if (!of_aliases)
> -			return NULL;
> -
> -		for_each_property_of_node(of_aliases, pp) {
> -			if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
> -				np = of_find_node_by_path(pp->value);
> -				break;
> +		raw_spin_lock_irqsave(&devtree_lock, flags);
> +		aliases = of_node_get(of_aliases);
> +		if (aliases) {
> +			for_each_property_of_node(aliases, pp) {
> +				if (!strncmp(pp->name, path, len) &&
> +				    !pp->name[len]) {
> +					if (of_alias_value_ok(pp))
> +						value = pp->value;
> +					break;
> +				}
>  			}
>  		}
> +		raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +
> +		/* the reference on @aliases keeps @value alive */
> +		if (value)
> +			np = of_find_node_by_path(value);
> +		of_node_put(aliases);
>  		if (!np)
>  			return NULL;
>  		path = p;

[Severity: High]
This isn't a bug introduced by this patch, but does the code further down in
of_find_node_opts_by_path() trigger a "scheduling while atomic" panic?

Immediately following this block, the function acquires the raw spinlock
again and calls __of_find_node_by_full_path():

	raw_spin_lock_irqsave(&devtree_lock, flags);
	if (!np)
		np = of_node_get(of_root);
	np = __of_find_node_by_full_path(np, path);
	raw_spin_unlock_irqrestore(&devtree_lock, flags);

Inside the path traversal loop of __of_find_node_by_full_path(),
of_node_put() drops the reference on the traversed node. If this is the last
reference (e.g. if the node was concurrently detached by an overlay removal),
of_node_release() is triggered.

of_node_release() calls fwnode_links_purge(), which acquires
fwnode_link_lock, a mutex. Attempting to lock a mutex while holding a raw
spinlock causes a panic. With dynamic /aliases overlay support, concurrent
node detachment becomes more likely.

> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 0ae16da066e2c..792756389691d 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -215,6 +215,13 @@ static inline bool is_pseudo_property(const char *prop_name)
>  		!of_prop_cmp(prop_name, "linux,phandle");
>  }
>  
> +/* alias values are deref'd as C strings; they must terminate within length */
> +static inline bool of_alias_value_ok(const struct property *pp)
> +{
> +	return pp->value && pp->length >= 2 &&
> +	       strnlen(pp->value, pp->length) < pp->length;
> +}

[Severity: Critical]
This is a pre-existing issue, but can this allow infinite recursion leading
to kernel stack exhaustion?

While this validation function ensures the alias is a properly null-terminated
string, it doesn't verify that the value is an absolute path (i.e., starts
with '/').

If an overlay provides an alias pointing to a relative path (e.g.,
loop = "loop"), of_find_node_opts_by_path() extracts this value and calls
of_find_node_by_path(value) recursively. Since the value doesn't start with
'/', the function treats it as another alias and repeats the process
infinitely.

Should this also validate that the first character is '/'?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-nh-of-alias-overlay-v4-0-8ad097e31e36@nexthop.ai?part=1

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 1/6] of: resolve alias-prefixed paths under devtree_lock
  2026-07-22  1:01 ` [PATCH v4 1/6] of: resolve alias-prefixed paths under devtree_lock Abdurrahman Hussain
  2026-07-22  1:19   ` sashiko-bot
@ 2026-07-22  7:30   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2026-07-22  7:30 UTC (permalink / raw)
  To: Abdurrahman Hussain
  Cc: Rob Herring, Saravana Kannan, devicetree, linux-kernel

On Tue, Jul 21, 2026 at 06:01:46PM -0700, Abdurrahman Hussain wrote:
> of_find_node_opts_by_path() resolves an alias-prefixed path by
> walking the property list of the of_aliases node with no lock held
> and no reference taken on the node. Property surgery on /aliases —
> of_add_property(), of_remove_property(), changeset apply/revert —
> mutates that list under devtree_lock, so the lockless walk can step
> into a property that is being unlinked. Nothing keeps the node itself
> alive across the walk either: detaching /aliases and dropping the
> last reference frees the node and its property list mid-iteration.
> 
> The race has existed since the walk was introduced, but is hard to
> hit with a boot-FDT /aliases node that nothing ever detaches. The
> rest of this series makes /aliases dynamic — overlays can add and
> remove properties, and create and destroy the node itself — so close
> it first: find the matching property under devtree_lock with a
> reference held on of_aliases, and keep that reference across the
> of_find_node_by_path() call that resolves the value. The reference is
> what keeps the value string valid outside the lock: a concurrently
> removed property moves to the node's deadprops list and is only freed
> when the node itself is released.
> 
> While here, validate the value's shape before handing it to
> of_find_node_by_path(): /aliases contents can now come from overlays

Can it?

> and from raw changeset/of_add_property() callers, and only the
> overlay path validates at the producer. of_alias_value_ok() (shared
> with the alias tracking added later in this series) rejects values
> that are not non-empty C strings NUL-terminated within the property
> length, so the consumer no longer trusts any producer. This also
> covers the empty property (NULL value) that previously crashed in
> strchr().

All this looks like AI written.

> 
> The name match is folded into one bounded strncmp plus a check of the
> terminating NUL instead of strlen + strncmp, halving the string
> traversal now done with interrupts disabled.
> 
> Assisted-by: Claude:claude-fable-5 [Claude Code]
> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
> ---
>  drivers/of/base.c       | 26 ++++++++++++++++++--------
>  drivers/of/of_private.h |  7 +++++++
>  2 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 6e7a42dedad3..9c2770823889 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -995,6 +995,8 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>  
>  	/* The path could begin with an alias */
>  	if (*path != '/') {
> +		struct device_node *aliases;
> +		const char *value = NULL;
>  		int len;
>  		const char *p = strchrnul(path, '/');
>  
> @@ -1002,16 +1004,24 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>  			p = separator;
>  		len = p - path;
>  
> -		/* of_aliases must not be NULL */
> -		if (!of_aliases)
> -			return NULL;
> -
> -		for_each_property_of_node(of_aliases, pp) {
> -			if (strlen(pp->name) == len && !strncmp(pp->name, path, len)) {
> -				np = of_find_node_by_path(pp->value);
> -				break;
> +		raw_spin_lock_irqsave(&devtree_lock, flags);

Since when do we lock looping over for_each_property_of_node()?

Do I understand correctly your proposal: you want to add
raw_spin_lock_irqsave() over every piece of code having for_each_XXX
OF-loop?

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-07-22  7:30 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-22  1:01 [PATCH v4 0/6] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
2026-07-22  1:01 ` [PATCH v4 1/6] of: resolve alias-prefixed paths under devtree_lock Abdurrahman Hussain
2026-07-22  1:19   ` sashiko-bot
2026-07-22  7:30   ` Krzysztof Kozlowski
2026-07-22  1:01 ` [PATCH v4 2/6] of: incrementally update /aliases lookup on reconfig notifications Abdurrahman Hussain
2026-07-22  1:01 ` [PATCH v4 3/6] of/overlay: look up absolute target-paths absolutely Abdurrahman Hussain
2026-07-22  1:01 ` [PATCH v4 4/6] of/overlay: return ERR_PTR from dup_and_fixup_symbol_prop() Abdurrahman Hussain
2026-07-22  1:13   ` sashiko-bot
2026-07-22  1:01 ` [PATCH v4 5/6] of/overlay: rewrite /aliases path values to live-tree paths Abdurrahman Hussain
2026-07-22  1:16   ` sashiko-bot
2026-07-22  1:01 ` [PATCH v4 6/6] of: unittest: cover /aliases updates from overlay apply/revert Abdurrahman Hussain

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox