[PATCH] of/overlay: don't pass the changeset's own id to of_overlay_remove()

[PATCH] of/overlay: don't pass the changeset's own id to of_overlay_remove()

[Abdurrahman Hussain]

of_overlay_remove_all() passes &ovcs->id to of_overlay_remove(), which
zeroes *ovcs_id once the changeset is reverted, before calling
free_overlay_changeset(). Through the aliased pointer that zeroes
ovcs->id itself, so free_overlay_changeset()'s "ovcs->id > 0" guard
is false and both idr_remove() and list_del() are skipped: the
overlay_changeset is kfree()d while still linked in ovcs_list and
registered in ovcs_idr.

Every overlay removed through of_overlay_remove_all() thus leaves a
dangling list node and idr entry behind. The next ovcs_list iteration
or idr lookup walks freed memory, and the ids are never returned to
the idr. Nothing in-tree calls of_overlay_remove_all() today, but it
is the documented API for removing every overlay in one go
(Documentation/devicetree/overlay-notes.rst), so any module user hits
this deterministically.

Pass a local copy of the id, like every other of_overlay_remove()
caller does.

Reported-by: Sashiko AI <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/20260901014554.83A8D1F000E9@smtp.kernel.org
Fixes: 24789c5ce5a3 ("of: overlay: detect cases where device tree may become corrupt")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-fable-5 [Claude Code]
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/of/overlay.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 83c56dd9b5d8..f66f16cddc79 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -1297,7 +1297,9 @@ int of_overlay_remove_all(void)
 
 	/* the tail of list is guaranteed to be safe to remove */
 	list_for_each_entry_safe_reverse(ovcs, ovcs_n, &ovcs_list, ovcs_list) {
-		ret = of_overlay_remove(&ovcs->id);
+		int id = ovcs->id;
+
+		ret = of_overlay_remove(&id);
 		if (ret)
 			return ret;
 	}

---
base-commit: 21c89ff1fc86ffa517f3a9ca1ba5c48e9e37c1ba
change-id: 20260917-b4-of-overlay-remove-all-fix-9fafc8104ea0

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

Re: [PATCH] of/overlay: don't pass the changeset's own id to of_overlay_remove()

[Rob Herring]

On Thu, Sep 17, 2026 at 07:28:16PM -0700, Abdurrahman Hussain wrote:
> of_overlay_remove_all() passes &ovcs->id to of_overlay_remove(), which
> zeroes *ovcs_id once the changeset is reverted, before calling
> free_overlay_changeset(). Through the aliased pointer that zeroes
> ovcs->id itself, so free_overlay_changeset()'s "ovcs->id > 0" guard
> is false and both idr_remove() and list_del() are skipped: the
> overlay_changeset is kfree()d while still linked in ovcs_list and
> registered in ovcs_idr.
> 
> Every overlay removed through of_overlay_remove_all() thus leaves a
> dangling list node and idr entry behind. The next ovcs_list iteration
> or idr lookup walks freed memory, and the ids are never returned to
> the idr. Nothing in-tree calls of_overlay_remove_all() today, but it
> is the documented API for removing every overlay in one go
> (Documentation/devicetree/overlay-notes.rst), so any module user hits
> this deterministically.
> 
> Pass a local copy of the id, like every other of_overlay_remove()
> caller does.

If every caller passes a local and updating the value is not needed, 
then we should just update the API to pass the value rather than a 
reference.

Additionally, if there are no callers we should perhaps just remove the 
API.

Rob

Re: [PATCH] of/overlay: don't pass the changeset's own id to of_overlay_remove()

[Abdurrahman Hussain]

On Mon Sep 28, 2026 at 11:26 AM PDT, Rob Herring wrote:
> On Thu, Sep 17, 2026 at 07:28:16PM -0700, Abdurrahman Hussain wrote:
>> of_overlay_remove_all() passes &ovcs->id to of_overlay_remove(), which
>> zeroes *ovcs_id once the changeset is reverted, before calling
>> free_overlay_changeset(). Through the aliased pointer that zeroes
>> ovcs->id itself, so free_overlay_changeset()'s "ovcs->id > 0" guard
>> is false and both idr_remove() and list_del() are skipped: the
>> overlay_changeset is kfree()d while still linked in ovcs_list and
>> registered in ovcs_idr.
>>
>> Every overlay removed through of_overlay_remove_all() thus leaves a
>> dangling list node and idr entry behind. The next ovcs_list iteration
>> or idr lookup walks freed memory, and the ids are never returned to
>> the idr. Nothing in-tree calls of_overlay_remove_all() today, but it
>> is the documented API for removing every overlay in one go
>> (Documentation/devicetree/overlay-notes.rst), so any module user hits
>> this deterministically.
>>
>> Pass a local copy of the id, like every other of_overlay_remove()
>> caller does.
>
> If every caller passes a local and updating the value is not needed,
> then we should just update the API to pass the value rather than a
> reference.
>

The value is used, just not by much. of_overlay_remove() zeroes
*ovcs_id only once the changeset is really reverted, and unittest.c
leans on that: overlay_18 injects a pre-remove notifier error and
checks ovcs_id survives the -EXDEV, overlay_19 injects a post-remove
error and checks ovcs_id got zeroed. The return code says the same
thing, so by value I'd drop those two checks and keep the ret ones.

The zeroing is also the double-remove guard, via "if (*ovcs_id == 0)
return 0;". lan966x_pci keeps the id in data->ovcs_id and
of_overlay_fdt_apply_kunit() removes from a kunit exit action, so both
would have to clear their own copy or start getting -ENODEV and a
pr_err() on a second call. Easy enough, but it's a behaviour change,
not just a signature change.

> Additionally, if there are no callers we should perhaps just remove the
> API.

No in-tree callers, yes. It's the EXPORT_SYMBOL_GPL(), a paragraph in
overlay-notes.rst and the same paragraph in the zh_CN translation.

If it goes, the bug here is unreachable and the patch is pointless, so:

  1. just remove of_overlay_remove_all() and drop this patch, or
  2. this one-liner first (only piece that can go to stable), then the
     removal, then the by-value conversion.

Which do you want? I'll send 1 unless you think the backport is worth
having.

Abdurrahman