Re: [PATCH v5 5/9] of/overlay: don't keep a negative id in ovcs->id on idr_alloc() failure

[Geert Uytterhoeven]

Hi Abdurrahman,

Thanks for your patch!

On Thu, 23 Jul 2026 at 05:26, Abdurrahman Hussain
<abdurrahman@xxxxxxxxxx> wrote:
> of_overlay_fdt_apply() stores the idr_alloc() return value in
> ovcs->id before checking it. On failure the stored id is negative and
> the error path runs free_overlay_changeset(), whose "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.

Nice catch!

>
> Reset ovcs->id to 0 before taking the error path.
>
> Fixes: 61b4de4e0b38 ("of: overlay: minor restructuring")
> Assisted-by: Claude:claude-fable-5 [Claude Code]
> Signed-off-by: Abdurrahman Hussain <abdurrahman@xxxxxxxxxx>

> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -1038,6 +1038,8 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
>         ovcs->id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
>         if (ovcs->id <= 0) {
>                 ret = ovcs->id;
> +               /* free_overlay_changeset() treats a set id as registered */
> +               ovcs->id = 0;

(Alternatively, free_overlay_changeset() could check for a
strict-positive id)

However, I am wondering if this is the right fix?

All error cases above just return an error code, without doing
any cleanup. Failing to allocate an ID doesn't add any extra state...

>                 goto err_free_ovcs;

... so shouldn't this just  be changed to "goto out_unlock" instead?

>         }
>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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