* [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications
2026-07-20 7:02 [PATCH RFC 0/4] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
@ 2026-07-20 7:02 ` Abdurrahman Hussain
2026-07-20 7:17 ` sashiko-bot
2026-07-20 7:02 ` [PATCH RFC 2/4] of/overlay: look up absolute target-paths absolutely Abdurrahman Hussain
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Abdurrahman Hussain @ 2026-07-20 7:02 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan
Cc: devicetree, linux-kernel, Abdurrahman Hussain
/aliases entries added by a device-tree overlay are stored in the live
tree but never enter the global aliases_lookup list that of_alias_scan()
builds at boot. As a result, of_alias_get_id() returns -ENODEV for
aliases declared inside overlays, and any driver that relies on
alias-based numbering (i2c-xiic, spi, tty, mmc, ...) silently loses its
pinned id and falls back to auto-assignment.
Fix by registering an internal OF reconfig notifier from
of_core_init() that mirrors /aliases property changes into
aliases_lookup:
OF_RECONFIG_ADD_PROPERTY -> of_alias_create(pp, kzalloc, owned=true)
OF_RECONFIG_REMOVE_PROPERTY -> of_alias_destroy(name)
OF_RECONFIG_UPDATE_PROPERTY -> destroy + create
The reconfig notifier chain fires from both direct changesets and
overlay apply/revert, so the same code path covers runtime dt
modifications and overlay-declared aliases without any overlay-
specific hook in drivers/of/overlay.c. Grant Likely suggested this
shape on Geert Uytterhoeven's 2015 RFC [1]; Geert's original hook was
in dynamic.c directly.
Match the /aliases target node structurally (name == "aliases" and
parent == root) rather than by pointer against the of_aliases global.
A system with no boot-time /aliases has of_aliases == NULL, so an
overlay that creates /aliases from scratch would otherwise be missed
from the first ATTACH_NODE onward. The ATTACH/DETACH_NODE cases also
update of_aliases lazily so subsequent consumers see it.
Factor the per-property loop body of of_alias_scan() into
of_alias_create() so the boot-time scan and the runtime notifier share
one code path. Add a one-bit @owned flag to struct alias_prop tracking
whether the entry was kmalloc'd (overlay-time) or came from the boot-
time memblock allocator via of_alias_scan(). of_alias_destroy() skips
non-owned entries, so an overlay-driven UPDATE_PROPERTY against a
boot-time alias can't kfree() memblock storage — addressing the
allocator-mismatch worry Grant flagged on the 2015 series [2].
An @owned entry also holds an of_node_get() reference to its target,
released by of_alias_destroy(); this fixes a smaller leak Geert's
original of_alias_create() would have introduced for overlay targets.
Naming builds on Geert's original series:
- "of: Extract of_alias_create()" [3]
- "of: Add of_alias_destroy()" [4]
- "of/dynamic: Update list of aliases on aliases changes" [5]
Link: https://lore.kernel.org/lkml/1435675876-2159-1-git-send-email-geert+renesas@glider.be/ [1]
Link: https://lore.kernel.org/lkml/20150630172131.D4E6CC4041A@trevor.secretlab.ca/ [2]
Link: https://lore.kernel.org/lkml/1435675876-2159-2-git-send-email-geert+renesas@glider.be/ [3]
Link: https://lore.kernel.org/lkml/1435675876-2159-3-git-send-email-geert+renesas@glider.be/ [4]
Link: https://lore.kernel.org/lkml/1435675876-2159-4-git-send-email-geert+renesas@glider.be/ [5]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/of/base.c | 184 ++++++++++++++++++++++++++++++++++++++----------
drivers/of/of_private.h | 7 ++
2 files changed, 155 insertions(+), 36 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 6e7a42dedad3..d516523a71d5 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1915,6 +1915,152 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
ap->alias, ap->stem, ap->id, np);
}
+/*
+ * Build an alias_prop for @pp using @dt_alloc as the storage allocator
+ * and add it to aliases_lookup. @owned is stored on the entry so the
+ * matching destroy path knows whether the alias_prop is a kmalloc'd
+ * struct that must be kfree()d (with a paired of_node_put on the
+ * target) or a memblock/dt_alloc'd struct that must be left alone.
+ *
+ * Pseudo-properties (name, phandle, ...) and alias names not ending in
+ * a numeric id are silently skipped.
+ */
+static void of_alias_create(const struct property *pp,
+ void *(*dt_alloc)(u64 size, u64 align),
+ bool owned)
+{
+ const char *start = pp->name;
+ const char *end = start + strlen(start);
+ struct device_node *np;
+ struct alias_prop *ap;
+ int id, len;
+
+ if (is_pseudo_property(pp->name))
+ return;
+
+ np = of_find_node_by_path(pp->value);
+ if (!np)
+ return;
+
+ while (isdigit(*(end - 1)) && end > start)
+ end--;
+ len = end - start;
+
+ if (kstrtoint(end, 10, &id) < 0)
+ goto out_put;
+
+ ap = dt_alloc(sizeof(*ap) + len + 1, __alignof__(*ap));
+ if (!ap)
+ goto out_put;
+ memset(ap, 0, sizeof(*ap) + len + 1);
+ ap->alias = start;
+ ap->owned = owned;
+ of_alias_add(ap, np, id, start, len);
+ return;
+
+out_put:
+ /*
+ * Boot-time entries reach here on parse failure; leaking the
+ * of_node_get() from of_find_node_by_path() is fine because the
+ * boot tree is never freed. Overlay-time entries need the put so
+ * the target node's refcount tracks the failed-parse case
+ * symmetrically with success.
+ */
+ if (owned)
+ of_node_put(np);
+}
+
+/*
+ * Reverse of of_alias_create(): find the owned alias_prop whose name
+ * matches @name, unlink it, and release everything it owns. Non-owned
+ * (boot-time) entries are skipped so an overlay-driven UPDATE against
+ * a boot-time alias can't kfree() memblock storage. That does mean an
+ * overlay updating a boot-time alias leaves two entries in
+ * aliases_lookup — deferred as a separate cleanup.
+ */
+static void of_alias_destroy(const char *name)
+{
+ struct alias_prop *ap, *tmp;
+
+ list_for_each_entry_safe(ap, tmp, &aliases_lookup, link) {
+ if (!ap->owned || strcmp(ap->alias, name) != 0)
+ continue;
+ list_del(&ap->link);
+ of_node_put(ap->np);
+ kfree(ap);
+ return;
+ }
+}
+
+static void *alias_alloc(u64 size, u64 align)
+{
+ return kzalloc(size, GFP_KERNEL);
+}
+
+/*
+ * OF reconfig notifier that mirrors /aliases property changes into
+ * aliases_lookup. Fires on both direct changesets and overlay
+ * apply/revert, so of_alias_get_id() returns the right id for aliases
+ * declared inside an overlay.
+ */
+static int of_aliases_reconfig_notifier(struct notifier_block *nb,
+ unsigned long action, void *arg)
+{
+ struct of_reconfig_data *rd = arg;
+
+ /*
+ * Match /aliases structurally (name + root-parent) rather than by
+ * pointer against the of_aliases global — a system with no
+ * boot-time /aliases (of_aliases == NULL) can still acquire one
+ * from an overlay, and we must track its properties from the
+ * first ATTACH_NODE onward.
+ */
+ if (!rd->dn || !rd->dn->parent ||
+ !of_node_is_root(rd->dn->parent) ||
+ !of_node_name_eq(rd->dn, "aliases"))
+ return NOTIFY_DONE;
+
+ switch (action) {
+ case OF_RECONFIG_ATTACH_NODE:
+ if (!of_aliases)
+ of_aliases = rd->dn;
+ break;
+ case OF_RECONFIG_DETACH_NODE:
+ if (of_aliases == rd->dn)
+ of_aliases = NULL;
+ break;
+ case OF_RECONFIG_ADD_PROPERTY:
+ of_alias_create(rd->prop, alias_alloc, true);
+ break;
+ case OF_RECONFIG_REMOVE_PROPERTY:
+ of_alias_destroy(rd->prop->name);
+ break;
+ case OF_RECONFIG_UPDATE_PROPERTY:
+ of_alias_destroy(rd->old_prop->name);
+ of_alias_create(rd->prop, alias_alloc, true);
+ break;
+ default:
+ break;
+ }
+ return NOTIFY_OK;
+}
+
+static struct notifier_block of_aliases_nb = {
+ .notifier_call = of_aliases_reconfig_notifier,
+};
+
+static int __init of_aliases_reconfig_init(void)
+{
+ return of_reconfig_notifier_register(&of_aliases_nb);
+}
+
+/*
+ * of_alias_scan() runs from of_core_init() (core_initcall), so hook the
+ * reconfig notifier one initcall level later to guarantee the initial
+ * static scan is complete before any dynamic tracking begins.
+ */
+core_initcall_sync(of_aliases_reconfig_init);
+
/**
* of_alias_scan - Scan all properties of the 'aliases' node
* @dt_alloc: An allocator that provides a virtual address to memory
@@ -1950,42 +2096,8 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
if (!of_aliases)
return;
- for_each_property_of_node(of_aliases, pp) {
- const char *start = pp->name;
- const char *end = start + strlen(start);
- struct device_node *np;
- struct alias_prop *ap;
- int id, len;
-
- /* Skip those we do not want to proceed */
- if (is_pseudo_property(pp->name))
- continue;
-
- np = of_find_node_by_path(pp->value);
- if (!np)
- continue;
-
- /* walk the alias backwards to extract the id and work out
- * the 'stem' string */
- while (isdigit(*(end-1)) && end > start)
- end--;
- len = end - start;
-
- if (kstrtoint(end, 10, &id) < 0) {
- of_node_put(np);
- continue;
- }
-
- /* Allocate an alias_prop with enough space for the stem */
- ap = dt_alloc(sizeof(*ap) + len + 1, __alignof__(*ap));
- if (!ap) {
- of_node_put(np);
- continue;
- }
- memset(ap, 0, sizeof(*ap) + len + 1);
- ap->alias = start;
- of_alias_add(ap, np, id, start, len);
- }
+ for_each_property_of_node(of_aliases, pp)
+ of_alias_create(pp, dt_alloc, false);
}
/**
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 0ae16da066e2..9d16765ae2c3 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -17,6 +17,12 @@
* @alias: Alias property name
* @np: Pointer to device_node that the alias stands for
* @id: Index value from end of alias name
+ * @owned: True if @alias was kstrdup'd and @np was of_node_get'd on
+ * insertion (overlay-time entries). False for entries built
+ * by of_alias_scan() at boot, where @alias points into the
+ * FDT and @np is an unreferenced pointer. The removal path
+ * uses this flag to decide whether it must kfree(@alias),
+ * of_node_put(@np), and kfree(the struct itself).
* @stem: Alias string without the index
*
* The structure represents one alias property of 'aliases' node as
@@ -27,6 +33,7 @@ struct alias_prop {
const char *alias;
struct device_node *np;
int id;
+ bool owned;
char stem[];
};
--
2.54.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications
2026-07-20 7:02 ` [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications Abdurrahman Hussain
@ 2026-07-20 7:17 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-07-20 7:17 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
New issues:
- [Critical] Unconditional dereference of `rd->old_prop` on OF_RECONFIG_UPDATE_PROPERTY causes a NULL pointer panic.
- [Critical] OF_RECONFIG_DETACH_NODE fails to remove dynamically allocated aliases, leading to a Memory Leak and Use-After-Free.
- [High] Lockless modification of the global `aliases_lookup` list causes data races and memory corruption.
- [High] Updating a boot-time alias creates duplicate entries mapping to the same ID, causing driver resource conflicts.
- [Medium] OF_RECONFIG_ATTACH_NODE fails to scan the node's properties, silently ignoring aliases added in a newly attached /aliases node.
- [Medium] The global `of_aliases` pointer mismanages node reference counting, causing leaks and weak references.
- [Low] The struct alias_prop::owned documentation contains factually incorrect and misleading claims regarding memory ownership.
Pre-existing issues:
- [Critical] Creating an alias from an empty property causes a NULL pointer dereference in of_find_node_by_path().
- [High] The alias parsing loop contains a 1-byte out-of-bounds read if the property name is an empty string.
--
--- Patch [1]: [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications ---
commit 172d1d81baebcd7bb4e961520fc11e6b4eadbc38
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
of: incrementally update /aliases lookup on reconfig notifications
This commit registers an OF reconfig notifier to mirror changes in the
/aliases node into the global aliases_lookup list. It extracts the alias
parsing logic into a common function and tracks whether entries are statically
or dynamically allocated. This ensures aliases defined in overlays are
correctly resolved by drivers.
Link: https://lore.kernel.org/lkml/1435675876-2159-1-git-send-email-geert+renesas@glider.be/
Link: https://lore.kernel.org/lkml/20150630172131.D4E6CC4041A@trevor.secretlab.ca/
Link: https://lore.kernel.org/lkml/1435675876-2159-2-git-send-email-geert+renesas@glider.be/
Link: https://lore.kernel.org/lkml/1435675876-2159-3-git-send-email-geert+renesas@glider.be/
Link: https://lore.kernel.org/lkml/1435675876-2159-4-git-send-email-geert+renesas@glider.be/
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 6e7a42dedad3f..d516523a71d5e 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
[ ... ]
> @@ -1915,6 +1915,152 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
> +/*
> + * Build an alias_prop for @pp using @dt_alloc as the storage allocator
> + * and add it to aliases_lookup. @owned is stored on the entry so the
> + * matching destroy path knows whether the alias_prop is a kmalloc'd
> + * struct that must be kfree()d (with a paired of_node_put on the
> + * target) or a memblock/dt_alloc'd struct that must be left alone.
> + *
> + * Pseudo-properties (name, phandle, ...) and alias names not ending in
> + * a numeric id are silently skipped.
> + */
> +static void of_alias_create(const struct property *pp,
> + void *(*dt_alloc)(u64 size, u64 align),
> + bool owned)
> +{
> + const char *start = pp->name;
> + const char *end = start + strlen(start);
> + struct device_node *np;
> + struct alias_prop *ap;
> + int id, len;
> +
> + if (is_pseudo_property(pp->name))
> + return;
> +
> + np = of_find_node_by_path(pp->value);
[Severity: Critical]
This is a pre-existing issue, but does this cause a NULL pointer
dereference if the property is empty?
If an overlay or boot DT defines a property in /aliases with no value,
pp->value is NULL. Since of_alias_create() passes pp->value directly to
of_find_node_by_path(), which passes it to strchr(), will this crash?
> + if (!np)
> + return;
> +
> + while (isdigit(*(end - 1)) && end > start)
> + end--;
[Severity: High]
This isn't a bug introduced by this patch, but does the loop condition
cause an out-of-bounds read if the property name is an empty string?
If start is empty, end equals start. The loop evaluates isdigit(*(end - 1))
before verifying end > start, causing an immediate read of the memory byte
preceding the buffer.
> + len = end - start;
[ ... ]
> +static void of_alias_destroy(const char *name)
> +{
> + struct alias_prop *ap, *tmp;
> +
> + list_for_each_entry_safe(ap, tmp, &aliases_lookup, link) {
> + if (!ap->owned || strcmp(ap->alias, name) != 0)
> + continue;
[Severity: High]
If an overlay modifies the target of an existing boot-time alias, will
skipping the destruction of boot-time entries leave duplicate entries in
the list?
Since this skips entries where owned is false, the old boot-time alias
remains in aliases_lookup alongside the newly created overlay alias.
When of_alias_get_id() searches the list, could it return the same ID for
both devices?
> + list_del(&ap->link);
> + of_node_put(ap->np);
> + kfree(ap);
> + return;
> + }
> +}
[ ... ]
> +static int of_aliases_reconfig_notifier(struct notifier_block *nb,
> + unsigned long action, void *arg)
> +{
> + struct of_reconfig_data *rd = arg;
> +
> + /*
> + * Match /aliases structurally (name + root-parent) rather than by
> + * pointer against the of_aliases global — a system with no
> + * boot-time /aliases (of_aliases == NULL) can still acquire one
> + * from an overlay, and we must track its properties from the
> + * first ATTACH_NODE onward.
> + */
> + if (!rd->dn || !rd->dn->parent ||
> + !of_node_is_root(rd->dn->parent) ||
> + !of_node_name_eq(rd->dn, "aliases"))
> + return NOTIFY_DONE;
> +
> + switch (action) {
> + case OF_RECONFIG_ATTACH_NODE:
> + if (!of_aliases)
> + of_aliases = rd->dn;
[Severity: Medium]
Does assigning of_aliases here create a weak reference?
During boot-time scanning, of_alias_scan() acquires of_aliases with an
incremented reference count. Here, it is assigned without calling
of_node_get().
[Severity: Medium]
Also, if the system started without a boot-time /aliases node, does
attaching one fail to parse its properties?
Since this handler updates the of_aliases pointer but misses the
for_each_property_of_node() loop to actually create the aliases, it seems
the aliases in a newly attached node are silently ignored.
> + break;
> + case OF_RECONFIG_DETACH_NODE:
> + if (of_aliases == rd->dn)
> + of_aliases = NULL;
[Severity: Medium]
Does clearing the pointer here cause a reference leak since of_node_put()
is omitted?
[Severity: Critical]
Additionally, does skipping the removal of the dynamic aliases cause a
memory leak and use-after-free?
The DETACH_NODE handler clears the of_aliases pointer but skips removing
the corresponding alias_prop entries from aliases_lookup. When the detached
node and its properties are later freed, dangling entries will remain in
the global list. A subsequent alias update could iterate the list and call
strcmp() on freed pointer strings.
> + break;
> + case OF_RECONFIG_ADD_PROPERTY:
> + of_alias_create(rd->prop, alias_alloc, true);
[Severity: High]
Is it safe to modify aliases_lookup locklessly here?
The notifier processes dynamic changes and calls into of_alias_create()
which uses list_add_tail() on the global aliases_lookup list. OF reconfig
notifiers are intentionally executed after of_mutex has been unlocked, such
as in __of_changeset_apply_notify().
Could this race against other writers, or against driver probes calling
of_alias_get_id() which iterates the list expecting it to be protected by
of_mutex?
> + break;
> + case OF_RECONFIG_REMOVE_PROPERTY:
> + of_alias_destroy(rd->prop->name);
> + break;
> + case OF_RECONFIG_UPDATE_PROPERTY:
> + of_alias_destroy(rd->old_prop->name);
[Severity: Critical]
Can rd->old_prop be NULL here?
The of_update_property() API allows adding a property if it does not
already exist, and emits an OF_RECONFIG_UPDATE_PROPERTY event with
old_prop set to NULL. Would unconditionally dereferencing
rd->old_prop->name cause a panic in this scenario?
> + of_alias_create(rd->prop, alias_alloc, true);
> + break;
> + default:
> + break;
> + }
> + return NOTIFY_OK;
> +}
[ ... ]
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 0ae16da066e2c..9d16765ae2c3f 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -17,6 +17,12 @@
> * @alias: Alias property name
> * @np: Pointer to device_node that the alias stands for
> * @id: Index value from end of alias name
> + * @owned: True if @alias was kstrdup'd and @np was of_node_get'd on
> + * insertion (overlay-time entries). False for entries built
> + * by of_alias_scan() at boot, where @alias points into the
> + * FDT and @np is an unreferenced pointer. The removal path
> + * uses this flag to decide whether it must kfree(@alias),
> + * of_node_put(@np), and kfree(the struct itself).
[Severity: Low]
Does this documentation accurately reflect the implementation?
The comment claims @alias is kstrdup'd on insertion and kfree'd on
removal. However, the code merely stores the pp->name pointer directly
and never calls kfree() on the alias string. It also mentions np is
unreferenced at boot time, but of_find_node_by_path() does take a
reference.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-nh-of-alias-overlay-v1-0-f1e5d9889b30@nexthop.ai?part=1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH RFC 2/4] of/overlay: look up absolute target-paths absolutely
2026-07-20 7:02 [PATCH RFC 0/4] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
2026-07-20 7:02 ` [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications Abdurrahman Hussain
@ 2026-07-20 7:02 ` Abdurrahman Hussain
2026-07-20 7:15 ` sashiko-bot
2026-07-20 7:02 ` [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths Abdurrahman Hussain
2026-07-20 7:02 ` [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert Abdurrahman Hussain
3 siblings, 1 reply; 9+ messages in thread
From: Abdurrahman Hussain @ 2026-07-20 7:02 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan
Cc: devicetree, linux-kernel, Abdurrahman Hussain
When of_overlay_fdt_apply() is called with a non-NULL target base,
find_target() currently concatenates the base's full path with every
fragment's target-path via "%pOF%s" — so target-path="" resolves to
the base itself (the intended common case), but target-path="/foo"
resolves to "<base>/foo" (never the DT root) and target-path="/" to
"<base>/" (never a valid node at all).
That makes it impossible for a two-fragment overlay to modify one
subtree under the base and one node at the DT root — a shape that
arises naturally when a PCI-attached device wants to declare its
peripherals under dev_of_node(&pdev->dev) AND add /aliases entries
so alias-aware drivers (i2c-xiic, spi, tty, ...) can pin bus numbers.
Treat target-path as absolute whenever it is non-empty. An empty
target-path continues to mean "the target base itself", preserving
the existing shape used by drivers/misc/lan966x_pci.c and its dtso
(the only in-tree of_overlay_fdt_apply() caller today that passes a
non-NULL base).
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/of/overlay.c | 34 ++++++++++++++++------------------
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 08d5351746be..654a70d5cb07 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -693,7 +693,6 @@ static struct device_node *find_target(const struct device_node *info_node,
const struct device_node *target_base)
{
struct device_node *node;
- char *target_path;
const char *path;
u32 val;
int ret;
@@ -709,23 +708,22 @@ static struct device_node *find_target(const struct device_node *info_node,
ret = of_property_read_string(info_node, "target-path", &path);
if (!ret) {
- if (target_base) {
- target_path = kasprintf(GFP_KERNEL, "%pOF%s", target_base, path);
- if (!target_path)
- return NULL;
- node = of_find_node_by_path(target_path);
- if (!node) {
- pr_err("find target, node: %pOF, path '%s' not found\n",
- info_node, target_path);
- }
- kfree(target_path);
- } else {
- node = of_find_node_by_path(path);
- if (!node) {
- pr_err("find target, node: %pOF, path '%s' not found\n",
- info_node, path);
- }
- }
+ /*
+ * With a non-NULL @target_base, an empty target-path means
+ * "the target base itself" — this is the common
+ * of_overlay_fdt_apply(..., base) form used by e.g. the
+ * LAN966x PCI overlay. Any other target-path is looked up
+ * absolutely, so overlays that also need to reach the DT
+ * root (e.g. to add /aliases entries alongside a base-
+ * relative fragment) can do so with target-path="/aliases".
+ */
+ if (target_base && path[0] == '\0')
+ return of_node_get((struct device_node *)target_base);
+
+ node = of_find_node_by_path(path);
+ if (!node)
+ pr_err("find target, node: %pOF, path '%s' not found\n",
+ info_node, path);
return node;
}
--
2.54.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH RFC 2/4] of/overlay: look up absolute target-paths absolutely
2026-07-20 7:02 ` [PATCH RFC 2/4] of/overlay: look up absolute target-paths absolutely Abdurrahman Hussain
@ 2026-07-20 7:15 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-07-20 7:15 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: conor+dt, devicetree, robh
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths
2026-07-20 7:02 [PATCH RFC 0/4] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
2026-07-20 7:02 ` [PATCH RFC 1/4] of: incrementally update /aliases lookup on reconfig notifications Abdurrahman Hussain
2026-07-20 7:02 ` [PATCH RFC 2/4] of/overlay: look up absolute target-paths absolutely Abdurrahman Hussain
@ 2026-07-20 7:02 ` Abdurrahman Hussain
2026-07-20 7:20 ` sashiko-bot
2026-07-20 7:02 ` [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert Abdurrahman Hussain
3 siblings, 1 reply; 9+ messages in thread
From: Abdurrahman Hussain @ 2026-07-20 7:02 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan
Cc: devicetree, linux-kernel, Abdurrahman Hussain
/aliases entries added by 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths
2026-07-20 7:02 ` [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths Abdurrahman Hussain
@ 2026-07-20 7:20 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-07-20 7:20 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: robh, devicetree, conor+dt
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert
2026-07-20 7:02 [PATCH RFC 0/4] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
` (2 preceding siblings ...)
2026-07-20 7:02 ` [PATCH RFC 3/4] of/overlay: rewrite /aliases path values to live-tree paths Abdurrahman Hussain
@ 2026-07-20 7:02 ` Abdurrahman Hussain
2026-07-20 7:10 ` sashiko-bot
3 siblings, 1 reply; 9+ messages in thread
From: Abdurrahman Hussain @ 2026-07-20 7:02 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan
Cc: devicetree, linux-kernel, Abdurrahman Hussain
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert
2026-07-20 7:02 ` [PATCH RFC 4/4] of: unittest: cover /aliases updates from overlay apply/revert Abdurrahman Hussain
@ 2026-07-20 7:10 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-07-20 7:10 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: conor+dt, robh, devicetree
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The test 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
^ permalink raw reply [flat|nested] 9+ messages in thread