* [PATCH v6 01/10] of: fix out-of-bounds read in of_alias_scan() stem parser
2026-08-05 20:30 [PATCH v6 00/10] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
@ 2026-08-05 20:31 ` Abdurrahman Hussain
2026-08-06 8:10 ` Geert Uytterhoeven
2026-08-05 20:31 ` [PATCH v6 02/10] of: hold a reference on of_aliases during alias path resolution Abdurrahman Hussain
` (8 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Abdurrahman Hussain @ 2026-08-05 20:31 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Frank Rowand, David S. Miller,
Shawn Guo, Grant Likely
Cc: devicetree, linux-kernel, Abdurrahman Hussain, stable
The stem parser tests isdigit(*(end - 1)) before checking end > start
and so reads one byte before the property name when the name is empty
or all digits. Check the bound first.
Fixes: 611cad720148 ("dt: add of_alias_scan and of_alias_get_id")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-fable-5 [Claude Code]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/of/base.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 6e7a42dedad3..477017ed6f49 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1967,7 +1967,7 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
/* walk the alias backwards to extract the id and work out
* the 'stem' string */
- while (isdigit(*(end-1)) && end > start)
+ while (end > start && isdigit(*(end - 1)))
end--;
len = end - start;
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v6 01/10] of: fix out-of-bounds read in of_alias_scan() stem parser
2026-08-05 20:31 ` [PATCH v6 01/10] of: fix out-of-bounds read in of_alias_scan() stem parser Abdurrahman Hussain
@ 2026-08-06 8:10 ` Geert Uytterhoeven
0 siblings, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2026-08-06 8:10 UTC (permalink / raw)
To: Abdurrahman Hussain
Cc: Rob Herring, Saravana Kannan, Frank Rowand, David S. Miller,
Shawn Guo, Grant Likely, devicetree, linux-kernel, stable
On Wed, 5 Aug 2026 at 22:31, Abdurrahman Hussain <abdurrahman@nexthop.ai> wrote:
> The stem parser tests isdigit(*(end - 1)) before checking end > start
> and so reads one byte before the property name when the name is empty
> or all digits. Check the bound first.
>
> Fixes: 611cad720148 ("dt: add of_alias_scan and of_alias_get_id")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-fable-5 [Claude Code]
> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 02/10] of: hold a reference on of_aliases during alias path resolution
2026-08-05 20:30 [PATCH v6 00/10] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
2026-08-05 20:31 ` [PATCH v6 01/10] of: fix out-of-bounds read in of_alias_scan() stem parser Abdurrahman Hussain
@ 2026-08-05 20:31 ` Abdurrahman Hussain
2026-08-05 20:52 ` sashiko-bot
2026-08-05 20:31 ` [PATCH v6 03/10] of: update /aliases lookup on reconfig notifications Abdurrahman Hussain
` (7 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Abdurrahman Hussain @ 2026-08-05 20:31 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Frank Rowand, David S. Miller,
Shawn Guo, Grant Likely
Cc: devicetree, linux-kernel, Abdurrahman Hussain
of_find_node_opts_by_path() walks the property list of of_aliases
without taking a reference on the node and passes pp->value straight
to of_find_node_by_path().
Take a reference across the walk. The walk itself stays lock-free
like every other property iteration: it can race property surgery and
see a stale view (a removed property's ->next is repointed at the
deadprops list), but nothing it can reach is freed while the node
reference is held. devtree_lock covers only the pointer load,
pairing it with a later patch in this series that clears of_aliases
and drops its reference when the node is detached at runtime.
Validate the value before resolving it. of_alias_value_ok() requires
a non-empty, NUL-terminated, absolute path:
- an empty property has a NULL value and crashes in strchr()
- a value without a NUL inside the property is read past its end
- a relative value naming another alias (loop = "loop") recurses
through of_find_node_by_path() until the stack is exhausted
All three are reachable with a malformed boot FDT today.
The name comparison loses its redundant strlen() pass while here.
Assisted-by: Claude:claude-fable-5 [Claude Code]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/of/base.c | 20 +++++++++++++++-----
drivers/of/of_private.h | 8 ++++++++
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 477017ed6f49..eca1f55eee87 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)
+ /* the load pairs with writers that retire the node */
+ raw_spin_lock_irqsave(&devtree_lock, flags);
+ aliases = of_node_get(of_aliases);
+ raw_spin_unlock_irqrestore(&devtree_lock, flags);
+ if (!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);
+ 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;
}
}
+ /* 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..9bba999f0bf8 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -215,6 +215,14 @@ static inline bool is_pseudo_property(const char *prop_name)
!of_prop_cmp(prop_name, "linux,phandle");
}
+/* alias values must be absolute paths NUL-terminated within length */
+static inline bool of_alias_value_ok(const struct property *pp)
+{
+ return pp->value && pp->length >= 2 &&
+ *(const char *)pp->value == '/' &&
+ 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] 18+ messages in thread* Re: [PATCH v6 02/10] of: hold a reference on of_aliases during alias path resolution
2026-08-05 20:31 ` [PATCH v6 02/10] of: hold a reference on of_aliases during alias path resolution Abdurrahman Hussain
@ 2026-08-05 20:52 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-08-05 20:52 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Calling of_node_put() while holding devtree_lock (a raw spinlock) can cause a sleep in atomic context.
--
--- Patch [2]: [PATCH v6 02/10] of: hold a reference on of_aliases during alias path resolution ---
commit 0b9ece03ae6938d97128143d8a080acc486d86ce
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
of: hold a reference on of_aliases during alias path resolution
The commit modifies of_find_node_opts_by_path() to take a reference on
of_aliases while iterating over its properties for alias resolution. It also
adds validation via of_alias_value_ok() to ensure the alias property points
to a valid absolute path, preventing crashes from malformed FDTs.
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 477017ed6f49b..eca1f55eee872 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)
> + /* the load pairs with writers that retire the node */
> + raw_spin_lock_irqsave(&devtree_lock, flags);
> + aliases = of_node_get(of_aliases);
> + raw_spin_unlock_irqrestore(&devtree_lock, flags);
> + if (!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);
> + 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;
> }
> }
> + /* 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 just below this
block safely handle reference drops while holding a raw spinlock?
In of_find_node_opts_by_path(), shortly after this diff, we do:
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 __of_find_node_by_full_path(), there is a call to of_node_put():
node = __of_find_node_by_path(node, path);
of_node_put(tmp);
If this node was dynamically detached (e.g., via overlay removal), could this
of_node_put() call be dropping the final reference?
If the refcount drops to zero here, of_node_put() calls of_node_release().
This then calls fwnode_links_purge() which acquires fwnode_link_lock.
Since fwnode_link_lock is a sleepable mutex, can acquiring it while holding
devtree_lock (a raw spinlock with interrupts disabled) result in a sleep in
atomic context?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-nh-of-alias-overlay-v6-0-74f21d440819@nexthop.ai?part=2
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 03/10] of: update /aliases lookup on reconfig notifications
2026-08-05 20:30 [PATCH v6 00/10] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
2026-08-05 20:31 ` [PATCH v6 01/10] of: fix out-of-bounds read in of_alias_scan() stem parser Abdurrahman Hussain
2026-08-05 20:31 ` [PATCH v6 02/10] of: hold a reference on of_aliases during alias path resolution Abdurrahman Hussain
@ 2026-08-05 20:31 ` Abdurrahman Hussain
2026-08-05 20:31 ` [PATCH v6 04/10] of/overlay: look up absolute target-paths absolutely Abdurrahman Hussain
` (6 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Abdurrahman Hussain @ 2026-08-05 20:31 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Frank Rowand, David S. Miller,
Shawn Guo, Grant Likely
Cc: devicetree, linux-kernel, Abdurrahman Hussain
Aliases added by overlays never make it into aliases_lookup, which is
only filled by of_alias_scan() at boot. of_alias_get_id() returns
-ENODEV for them and drivers using alias based numbering (i2c, spi,
tty, mmc) fall back to dynamic ids.
Register a reconfig notifier and mirror /aliases property changes
into aliases_lookup. The notifier chain covers changesets and
overlays, so no overlay specific hook is needed. Same approach as
Geert's 2015 series [1], which was never reposted;
of_alias_create()/of_alias_destroy() keep its names.
The /aliases node is matched by name and root parent instead of the
of_aliases pointer, which is still NULL when an overlay creates the
node on a system without a boot-time /aliases. The name match is
exact, of_node_name_eq() would also match "aliases@1". ATTACH stores
the node in of_aliases with a reference held, DETACH drops it again
and flushes aliases_lookup; the pointer updates happen under
devtree_lock to pair with the reader in of_find_node_opts_by_path().
Nodes attached with properties already set are not scanned, as
before.
Lookup entries are created by of_alias_scan()'s old loop body, moved
into of_alias_create(). Entries created at runtime have kstrdup'ed
names and an of_node_get'ed target and are flagged "owned" so
of_alias_destroy() knows what to kfree(); boot entries live in
memblock and are only unlinked. Removal matches entries regardless of
ownership so updating a boot-time alias does not leave duplicates
behind, which was Grant's main concern on the old series [2].
A new aliases_mutex protects the list. of_mutex does not work here:
the notifier runs under it on the overlay path but outside of it on
the of_add_property() path.
of_alias_create() skips names that are empty or all digits instead of
creating an entry with an empty stem.
Only built for CONFIG_OF_DYNAMIC: there are no notifications without
it and the register stub returns -EINVAL.
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]
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 eca1f55eee87..612e2cd31688 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 (end > start && isdigit(*(end - 1)))
- 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 9bba999f0bf8..731b606ae3e3 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;
@@ -223,6 +230,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] 18+ messages in thread* [PATCH v6 04/10] of/overlay: look up absolute target-paths absolutely
2026-08-05 20:30 [PATCH v6 00/10] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
` (2 preceding siblings ...)
2026-08-05 20:31 ` [PATCH v6 03/10] of: update /aliases lookup on reconfig notifications Abdurrahman Hussain
@ 2026-08-05 20:31 ` Abdurrahman Hussain
2026-08-05 20:40 ` sashiko-bot
2026-08-05 20:31 ` [PATCH v6 05/10] of/overlay: put property on deadprops only after changeset add succeeds Abdurrahman Hussain
` (5 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Abdurrahman Hussain @ 2026-08-05 20:31 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Frank Rowand, David S. Miller,
Shawn Guo, Grant Likely
Cc: devicetree, linux-kernel, Abdurrahman Hussain
find_target() with a non-NULL target base concatenates the base path
and the fragment's target-path via "%pOF%s": target-path="/foo"
resolves to "<base>/foo" and can never reach the DT root. An overlay
applied with a base cannot both extend the base subtree and add
/aliases entries, which is what a PCI device declaring its
peripherals under dev_of_node() needs for alias-based bus numbering.
Treat any non-empty target-path as absolute. An empty target-path
still means the base itself, the only form used by the one in-tree
caller passing a base (drivers/misc/lan966x_pci.c).
Spell the contract out in the kernel-doc for @base/@target_base and
in find_target()'s strategy comment.
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] 18+ messages in thread* Re: [PATCH v6 04/10] of/overlay: look up absolute target-paths absolutely
2026-08-05 20:31 ` [PATCH v6 04/10] of/overlay: look up absolute target-paths absolutely Abdurrahman Hussain
@ 2026-08-05 20:40 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-08-05 20:40 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Medium] Pre-existing memory leak of target and overlay device node references occurs on the error paths of `init_overlay_changeset()`.
--
--- Patch [4]: [PATCH v6 04/10] of/overlay: look up absolute target-paths absolutely ---
commit 28d3c18ff8d649fd8af8d895999493b24c338fa1
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
of/overlay: look up absolute target-paths absolutely
This commit updates find_target() to resolve non-empty target-path properties
as absolute paths from the live-tree root rather than appending them to the
target base. Empty target-path values still resolve to the target base itself.
The documentation for init_overlay_changeset() and of_overlay_apply() is
updated to reflect this new behavior.
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 08d5351746be6..74aea704835ac 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
[ ... ]
> @@ -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
[Severity: Medium]
This is a pre-existing issue, but does init_overlay_changeset() leak
device node references when an error occurs during fragment processing?
Looking at init_overlay_changeset(), if find_target() fails (or if the
__symbols__ target is missing), it jumps to err_out without updating
ovcs->count:
fragment->target = find_target(node, target_base);
if (!fragment->target) {
of_node_put(fragment->overlay);
ret = -EINVAL;
of_node_put(node);
goto err_out;
}
Because ovcs->count is only updated to the successfully processed count
at the very end of the function, it remains 0 during the error return.
When the caller subsequently runs free_overlay_changeset() to clean up the
partially initialized changeset, its cleanup loop is skipped entirely because
ovcs->count is 0:
for (i = 0; i < ovcs->count; i++) {
of_node_put(ovcs->fragments[i].target);
of_node_put(ovcs->fragments[i].overlay);
}
Would this permanently leak the node references for any fragments that were
successfully processed before the failure occurred?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-nh-of-alias-overlay-v6-0-74f21d440819@nexthop.ai?part=4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 05/10] of/overlay: put property on deadprops only after changeset add succeeds
2026-08-05 20:30 [PATCH v6 00/10] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
` (3 preceding siblings ...)
2026-08-05 20:31 ` [PATCH v6 04/10] of/overlay: look up absolute target-paths absolutely Abdurrahman Hussain
@ 2026-08-05 20:31 ` Abdurrahman Hussain
2026-08-05 20:45 ` sashiko-bot
2026-08-05 20:31 ` [PATCH v6 06/10] of/overlay: only treat a positive changeset id as registered Abdurrahman Hussain
` (4 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Abdurrahman Hussain @ 2026-08-05 20:31 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Frank Rowand, David S. Miller,
Shawn Guo, Grant Likely
Cc: devicetree, linux-kernel, Abdurrahman Hussain
add_changeset_property() links a new property of a not-yet-live
target node into the node's deadprops list before handing it to
of_changeset_add_property(). If that fails, the error path frees the
property but leaves the freed pointer linked in deadprops, and
of_node_release() frees it a second time when the aborted overlay's
node is released.
Record the changeset entry first and link the property into deadprops
only on success. of_changeset_add_property() never looks at the
node's property lists, so the order of the two steps is otherwise
immaterial, and the error path frees a property that nothing
references.
Assisted-by: Claude:claude-fable-5 [Claude Code]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/of/overlay.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 74aea704835a..284c9bc6c9cf 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -358,12 +358,13 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
return -ENOMEM;
if (!prop) {
- if (!target->in_livetree) {
+ ret = of_changeset_add_property(&ovcs->cset, target->np,
+ new_prop);
+ /* the detached node owns the property until the apply */
+ if (!ret && !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);
} else {
ret = of_changeset_update_property(&ovcs->cset, target->np,
new_prop);
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v6 05/10] of/overlay: put property on deadprops only after changeset add succeeds
2026-08-05 20:31 ` [PATCH v6 05/10] of/overlay: put property on deadprops only after changeset add succeeds Abdurrahman Hussain
@ 2026-08-05 20:45 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-08-05 20:45 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Leaked fragment target and overlay node references when init_overlay_changeset() fails mid-iteration.
--
--- Patch [5]: [PATCH v6 05/10] of/overlay: put property on deadprops only after changeset add succeeds ---
commit 32c9bbaae9841195a053f81f6d3eb35116c97a19
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
of/overlay: put property on deadprops only after changeset add succeeds
This commit fixes a double-free issue in add_changeset_property().
It ensures a new property is only linked into the target node's deadprops
list after it has been successfully added to the changeset.
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 74aea704835ac..284c9bc6c9cfc 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
[ ... ]
While reviewing this file, I noticed a pre-existing issue nearby:
drivers/of/overlay.c:init_overlay_changeset() {
...
cnt = 0;
for_each_child_of_node(ovcs->overlay_root, node) {
overlay_node = of_get_child_by_name(node, "__overlay__");
if (!overlay_node)
continue;
fragment = &fragments[cnt];
fragment->overlay = overlay_node;
fragment->target = find_target(node, target_base);
if (!fragment->target) {
of_node_put(fragment->overlay);
ret = -EINVAL;
of_node_put(node);
goto err_out;
}
cnt++;
}
...
}
[Severity: High]
This is a pre-existing issue, but does this error path leak node references?
If find_target() fails for a later fragment, we jump to err_out without
updating ovcs->count.
When free_overlay_changeset() is later called to clean up, it relies on
ovcs->count to free the fragments:
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);
}
...
}
Because ovcs->count was never updated before the error jump in
init_overlay_changeset(), won't this skip cleaning up the successfully
processed fragments and leak their target and overlay node references?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-nh-of-alias-overlay-v6-0-74f21d440819@nexthop.ai?part=5
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 06/10] of/overlay: only treat a positive changeset id as registered
2026-08-05 20:30 [PATCH v6 00/10] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
` (4 preceding siblings ...)
2026-08-05 20:31 ` [PATCH v6 05/10] of/overlay: put property on deadprops only after changeset add succeeds Abdurrahman Hussain
@ 2026-08-05 20:31 ` Abdurrahman Hussain
2026-08-05 20:42 ` sashiko-bot
2026-08-06 7:52 ` Geert Uytterhoeven
2026-08-05 20:31 ` [PATCH v6 07/10] of/overlay: don't create "//" paths for fragments targeting the root Abdurrahman Hussain
` (3 subsequent siblings)
9 siblings, 2 replies; 18+ messages in thread
From: Abdurrahman Hussain @ 2026-08-05 20:31 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Frank Rowand, David S. Miller,
Shawn Guo, Grant Likely
Cc: devicetree, linux-kernel, Abdurrahman Hussain, stable,
Geert Uytterhoeven
of_overlay_fdt_apply() stores the idr_alloc() return value in
ovcs->id before checking it. On failure the stored id is negative,
free_overlay_changeset()'s "if (ovcs->id)" check passes, idr_remove()
is called with a negative id and list_del() runs on ovcs->ovcs_list,
which is not initialized until after the id allocation. An allocation
failure at that point dereferences NULL.
Make free_overlay_changeset() treat only a strict-positive id as
registered. The rest of the function already copes with a
partially-initialized ovcs, so the error path stays a plain
goto err_free_ovcs.
Fixes: 61b4de4e0b38 ("of: overlay: minor restructuring")
Cc: stable@vger.kernel.org
Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Assisted-by: Claude:claude-fable-5 [Claude Code]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/of/overlay.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 284c9bc6c9cf..9b9f198a1d70 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -860,7 +860,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
if (ovcs->cset.entries.next)
of_changeset_destroy(&ovcs->cset);
- if (ovcs->id) {
+ /* a failed idr_alloc() leaves its negative error in ovcs->id */
+ if (ovcs->id > 0) {
idr_remove(&ovcs_idr, ovcs->id);
list_del(&ovcs->ovcs_list);
ovcs->id = 0;
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v6 06/10] of/overlay: only treat a positive changeset id as registered
2026-08-05 20:31 ` [PATCH v6 06/10] of/overlay: only treat a positive changeset id as registered Abdurrahman Hussain
@ 2026-08-05 20:42 ` sashiko-bot
2026-08-06 7:52 ` Geert Uytterhoeven
1 sibling, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-08-05 20:42 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] This problem wasn't introduced by this patch, but lockless traversal of a shared list `ovcs_list` in `of_overlay_remove_all()` leads to a data race and potential Use-After-Free.
--
--- Patch [6]: [PATCH v6 06/10] of/overlay: only treat a positive changeset id as registered ---
commit ff7e3da3be86949a404438d74d2cffe93f4a4cba
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
of/overlay: only treat a positive changeset id as registered
of_overlay_fdt_apply() stores the idr_alloc() return value in
ovcs->id before checking it. On failure the stored id is negative,
free_overlay_changeset()'s "if (ovcs->id)" check passes, idr_remove()
is called with a negative id and list_del() runs on ovcs->ovcs_list,
which is not initialized until after the id allocation. An allocation
failure at that point dereferences NULL.
Make free_overlay_changeset() treat only a strict-positive id as
registered. The rest of the function already copes with a
partially-initialized ovcs, so the error path stays a plain
goto err_free_ovcs.
Cc: stable@vger.kernel.org
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 284c9bc6c9cfc..9b9f198a1d70a 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -860,7 +860,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> if (ovcs->cset.entries.next)
> of_changeset_destroy(&ovcs->cset);
>
> - if (ovcs->id) {
> + /* a failed idr_alloc() leaves its negative error in ovcs->id */
> + if (ovcs->id > 0) {
> idr_remove(&ovcs_idr, ovcs->id);
> list_del(&ovcs->ovcs_list);
[Severity: High]
This problem wasn't introduced by this patch, but lockless traversal of a
shared list ovcs_list in of_overlay_remove_all() leads to a data race.
Is it safe to iterate ovcs_list without holding of_mutex?
drivers/of/overlay.c:of_overlay_remove_all() {
...
list_for_each_entry_safe_reverse(ovcs, ovcs_n, &ovcs_list, ovcs_list) {
ret = of_overlay_remove(&ovcs->id);
...
}
The list_for_each_entry_safe_reverse() iterator only protects against
the deletion of the current entry being processed.
If another thread concurrently modifies the list (e.g., via
of_overlay_fdt_apply() or of_overlay_remove()), could this corrupt the
list pointers or remove the cached next entry, leading to a Use-After-Free?
> ovcs->id = 0;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-nh-of-alias-overlay-v6-0-74f21d440819@nexthop.ai?part=6
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v6 06/10] of/overlay: only treat a positive changeset id as registered
2026-08-05 20:31 ` [PATCH v6 06/10] of/overlay: only treat a positive changeset id as registered Abdurrahman Hussain
2026-08-05 20:42 ` sashiko-bot
@ 2026-08-06 7:52 ` Geert Uytterhoeven
1 sibling, 0 replies; 18+ messages in thread
From: Geert Uytterhoeven @ 2026-08-06 7:52 UTC (permalink / raw)
To: Abdurrahman Hussain
Cc: Rob Herring, Saravana Kannan, Frank Rowand, David S. Miller,
Shawn Guo, Grant Likely, devicetree, linux-kernel, stable
On Wed, 5 Aug 2026 at 22:31, Abdurrahman Hussain <abdurrahman@nexthop.ai> wrote:
> of_overlay_fdt_apply() stores the idr_alloc() return value in
> ovcs->id before checking it. On failure the stored id is negative,
> free_overlay_changeset()'s "if (ovcs->id)" check passes, idr_remove()
> is called with a negative id and list_del() runs on ovcs->ovcs_list,
> which is not initialized until after the id allocation. An allocation
> failure at that point dereferences NULL.
>
> Make free_overlay_changeset() treat only a strict-positive id as
> registered. The rest of the function already copes with a
> partially-initialized ovcs, so the error path stays a plain
> goto err_free_ovcs.
>
> Fixes: 61b4de4e0b38 ("of: overlay: minor restructuring")
> Cc: stable@vger.kernel.org
> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Assisted-by: Claude:claude-fable-5 [Claude Code]
> Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 07/10] of/overlay: don't create "//" paths for fragments targeting the root
2026-08-05 20:30 [PATCH v6 00/10] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
` (5 preceding siblings ...)
2026-08-05 20:31 ` [PATCH v6 06/10] of/overlay: only treat a positive changeset id as registered Abdurrahman Hussain
@ 2026-08-05 20:31 ` Abdurrahman Hussain
2026-08-05 20:45 ` sashiko-bot
2026-08-05 20:31 ` [PATCH v6 08/10] of/overlay: return ERR_PTR from dup_and_fixup_symbol_prop() Abdurrahman Hussain
` (2 subsequent siblings)
9 siblings, 1 reply; 18+ messages in thread
From: Abdurrahman Hussain @ 2026-08-05 20:31 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Frank Rowand, David S. Miller,
Shawn Guo, Grant Likely
Cc: devicetree, linux-kernel, Abdurrahman Hussain
dup_and_fixup_symbol_prop() rewrites a symbol value by replacing its
"/fragment/__overlay__" prefix with the fragment's target path. When
the fragment targets the root node the target path is "/" and the
result starts with "//", which __of_find_node_by_full_path() cannot
resolve: symbols pointing into such fragments silently stop
resolving.
Drop the target path when it is the root and the tail is non-empty. A
value naming the fragment root itself (empty tail) keeps the "/".
Fixes: d1651b03c2df ("of: overlay: add overlay symbols to live device tree")
Assisted-by: Claude:claude-fable-5 [Claude Code]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/of/overlay.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 9b9f198a1d70..89a9fe6d6e78 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -256,6 +256,9 @@ static struct property *dup_and_fixup_symbol_prop(
if (!target_path)
return NULL;
target_path_len = strlen(target_path);
+ /* a root target renders as "/"; drop it to avoid "//" results */
+ if (target_path_len == 1 && target_path[0] == '/' && path_tail_len)
+ target_path_len = 0;
new_prop = kzalloc_obj(*new_prop);
if (!new_prop)
--
2.54.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v6 07/10] of/overlay: don't create "//" paths for fragments targeting the root
2026-08-05 20:31 ` [PATCH v6 07/10] of/overlay: don't create "//" paths for fragments targeting the root Abdurrahman Hussain
@ 2026-08-05 20:45 ` sashiko-bot
0 siblings, 0 replies; 18+ messages in thread
From: sashiko-bot @ 2026-08-05 20:45 UTC (permalink / raw)
To: Abdurrahman Hussain; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Node reference leak for initialized fragments if find_target() fails in init_overlay_changeset().
--
--- Patch [7]: [PATCH v6 07/10] of/overlay: don't create "//" paths for fragments targeting the root ---
commit 160c0459de49f61664940cc0b23bceacbf1b3082
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
of/overlay: don't create "//" paths for fragments targeting the root
dup_and_fixup_symbol_prop() rewrites a symbol value by replacing its
"/fragment/__overlay__" prefix with the fragment's target path. When
the fragment targets the root node the target path is "/" and the
result starts with "//", which __of_find_node_by_full_path() cannot
resolve.
Drop the target path when it is the root and the tail is non-empty. A
value naming the fragment root itself (empty tail) keeps the "/".
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 9b9f198a1d70a..89a9fe6d6e786 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -256,6 +256,9 @@ static struct property *dup_and_fixup_symbol_prop(
> if (!target_path)
> return NULL;
> target_path_len = strlen(target_path);
> + /* a root target renders as "/"; drop it to avoid "//" results */
> + if (target_path_len == 1 && target_path[0] == '/' && path_tail_len)
> + target_path_len = 0;
>
> new_prop = kzalloc_obj(*new_prop);
> if (!new_prop)
[ ... ]
> @@ -804,13 +804,13 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> cnt = 0;
> for_each_child_of_node(ovcs->overlay_root, node) {
> overlay_node = of_get_child_by_name(node, "__overlay__");
> if (!overlay_node)
> continue;
>
> fragment = &fragments[cnt];
> fragment->overlay = overlay_node;
> fragment->target = find_target(node, target_base);
> if (!fragment->target) {
> of_node_put(fragment->overlay);
> ret = -EINVAL;
> of_node_put(node);
> goto err_out;
> }
[Severity: High]
This is a pre-existing issue, but does this jump to err_out leak device
node references if previous fragments were already successfully populated?
If a subsequent fragment's find_target() fails (e.g., due to an invalid target
path in the overlay), the function jumps to err_out without updating
ovcs->count.
Because ovcs->count remains 0, the cleanup function free_overlay_changeset()
will skip its loop and fail to call of_node_put() for the previously
successfully resolved fragment nodes.
Could ovcs->count be updated to cnt before jumping to err_out so that
free_overlay_changeset() can clean up the previously populated elements?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-nh-of-alias-overlay-v6-0-74f21d440819@nexthop.ai?part=7
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 08/10] of/overlay: return ERR_PTR from dup_and_fixup_symbol_prop()
2026-08-05 20:30 [PATCH v6 00/10] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
` (6 preceding siblings ...)
2026-08-05 20:31 ` [PATCH v6 07/10] of/overlay: don't create "//" paths for fragments targeting the root Abdurrahman Hussain
@ 2026-08-05 20:31 ` Abdurrahman Hussain
2026-08-05 20:31 ` [PATCH v6 09/10] of/overlay: rewrite /aliases path values to live-tree paths Abdurrahman Hussain
2026-08-05 20:31 ` [PATCH v6 10/10] of: unittest: cover /aliases updates from overlay apply/revert Abdurrahman Hussain
9 siblings, 0 replies; 18+ messages in thread
From: Abdurrahman Hussain @ 2026-08-05 20:31 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Frank Rowand, David S. Miller,
Shawn Guo, Grant Likely
Cc: devicetree, linux-kernel, Abdurrahman Hussain
dup_and_fixup_symbol_prop() returns NULL for malformed values, for
values that are not paths into one of the overlay's fragments, and
for allocation failures. The caller reports all of them as -ENOMEM,
so a structural problem in /__symbols__ is diagnosed as memory
exhaustion.
A later patch reuses the helper for /aliases values and must handle
the three cases differently: copy verbatim, warn, or fail the apply.
Return ERR_PTR(-EINVAL), ERR_PTR(-ENODEV) and ERR_PTR(-ENOMEM)
respectively and propagate the errno in the /__symbols__ caller.
Also verify that the value descends through the matched fragment's
__overlay__ node before cutting the prefix. Only the first path
component was resolved, so an absolute live-tree value sharing its
first component with a fragment name was sliced at the prefix length
and rewritten to garbage. A prefix mismatch returns -ENODEV.
Assisted-by: Claude:claude-fable-5 [Claude Code]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
drivers/of/overlay.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 89a9fe6d6e78..99cea094943d 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)
@@ -217,6 +221,8 @@ static struct property *dup_and_fixup_symbol_prop(
const char *path;
const char *path_tail;
const char *target_path;
+ char *overlay_name;
+ bool mismatch;
int k;
int overlay_name_len;
int path_len;
@@ -224,14 +230,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 +249,27 @@ static struct property *dup_and_fixup_symbol_prop(
break;
}
if (k >= ovcs->count)
- return NULL;
+ return ERR_PTR(-ENODEV);
+
+ overlay_name = kasprintf(GFP_KERNEL, "%pOF", fragment->overlay);
+ if (!overlay_name)
+ return ERR_PTR(-ENOMEM);
+ overlay_name_len = strlen(overlay_name);
- overlay_name_len = snprintf(NULL, 0, "%pOF", fragment->overlay);
+ /* @path must descend through this fragment's __overlay__ node */
+ mismatch = overlay_name_len > path_len ||
+ strncmp(path, overlay_name, overlay_name_len) != 0 ||
+ (path[overlay_name_len] != '/' && path[overlay_name_len]);
+ kfree(overlay_name);
+ if (mismatch)
+ return ERR_PTR(-ENODEV);
- if (overlay_name_len > path_len)
- return NULL;
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);
/* a root target renders as "/"; drop it to avoid "//" results */
if (target_path_len == 1 && target_path[0] == '/' && path_tail_len)
@@ -284,7 +299,7 @@ static struct property *dup_and_fixup_symbol_prop(
err_free_target_path:
kfree(target_path);
- return NULL;
+ return ERR_PTR(-ENOMEM);
}
/**
@@ -353,6 +368,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] 18+ messages in thread* [PATCH v6 09/10] of/overlay: rewrite /aliases path values to live-tree paths
2026-08-05 20:30 [PATCH v6 00/10] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
` (7 preceding siblings ...)
2026-08-05 20:31 ` [PATCH v6 08/10] of/overlay: return ERR_PTR from dup_and_fixup_symbol_prop() Abdurrahman Hussain
@ 2026-08-05 20:31 ` Abdurrahman Hussain
2026-08-05 20:31 ` [PATCH v6 10/10] of: unittest: cover /aliases updates from overlay apply/revert Abdurrahman Hussain
9 siblings, 0 replies; 18+ messages in thread
From: Abdurrahman Hussain @ 2026-08-05 20:31 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Frank Rowand, David S. Miller,
Shawn Guo, Grant Likely
Cc: devicetree, linux-kernel, Abdurrahman Hussain
An /aliases property added by an overlay references labeled nodes as
"&label", which dtc renders as an overlay-internal path such as
"/fragment@1/__overlay__/i2c@40000". The value is copied verbatim
into the live tree, of_find_node_by_path() cannot resolve it, and
of_alias_get_id() keeps returning -ENODEV.
/__symbols__ values follow the same convention and are already
rewritten by dup_and_fixup_symbol_prop(). Use it for properties of
the /aliases node too:
- a value that resolves inside one of the overlay's fragments is
stored rewritten to the live-tree path
- -ENODEV (legacy string aliases, absolute live-tree paths) is
copied verbatim, as before this series
- -EINVAL (not a NUL-terminated string) is copied verbatim with a
warning; consumers validate before dereferencing, and an overlay
that applied before this series keeps applying
- only -ENOMEM fails the apply
Pseudo-properties are exempt: the is_pseudo_property() check at the
top of add_changeset_property() only covers live-tree targets, and a
phandle of a newly created /aliases node is a cell, not a string.
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 99cea094943d..d74e2a3d6920 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -370,6 +370,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] 18+ messages in thread* [PATCH v6 10/10] of: unittest: cover /aliases updates from overlay apply/revert
2026-08-05 20:30 [PATCH v6 00/10] of: teach overlay code to keep /aliases in sync Abdurrahman Hussain
` (8 preceding siblings ...)
2026-08-05 20:31 ` [PATCH v6 09/10] of/overlay: rewrite /aliases path values to live-tree paths Abdurrahman Hussain
@ 2026-08-05 20:31 ` Abdurrahman Hussain
9 siblings, 0 replies; 18+ messages in thread
From: Abdurrahman Hussain @ 2026-08-05 20:31 UTC (permalink / raw)
To: Rob Herring, Saravana Kannan, Frank Rowand, David S. Miller,
Shawn Guo, Grant Likely
Cc: devicetree, linux-kernel, Abdurrahman Hussain
Add overlay_alias.dtso and of_unittest_overlay_alias().
The overlay has two fragments: target-path="" grafts a labeled node
under the run-time target base, and target-path="/aliases" adds
`testcase-alias99 = &<label>` at the DT root. The runner applies it
with a non-NULL base via of_overlay_fdt_apply() (overlay_data_apply()
always passes NULL), asserts of_alias_get_id() on the grafted node
returns 99, reverts, and asserts of_alias_get_highest_id() reports
the stem gone. A precondition check runs before the apply.
This exercises the reconfig notifier, the absolute target-path
lookup, and the /aliases value rewrite together; any one missing
turns the get_id assertion into -ENODEV.
The grafted-node reference is dropped before of_overlay_remove():
overlay-created nodes must be back at refcount 1 when the changeset
is destroyed. The post-remove assertion uses of_alias_get_highest_id()
because it is keyed by stem and needs no node reference. The apply is
wrapped in EXPECT_BEGIN/END for the "memory leak will occur" warning
printed for properties added to nodes the overlay didn't create,
like the other overlay tests.
The overlay is not added to the fdtoverlay static build test:
fdtoverlay has no target-base concept, so the empty target-path
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] 18+ messages in thread