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