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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but callers of `of_overlay_fdt_apply()` fail to call `of_overlay_remove()` when the application fails, leading to a memory leak.
- [High] This problem wasn't introduced by this patch, but if `of_changeset_attach_node()` fails in `add_changeset_node()`, the newly allocated device node `tchild` is permanently leaked.
- [High] This problem wasn't introduced by this patch, but if `init_overlay_changeset()` fails after successfully parsing one or more fragments, the OF node references for those initialized fragments are leaked.
--

--- Patch [5]: [PATCH v5 5/9] of/overlay: don't keep a negative id in ovcs->id on idr_alloc() failure ---
commit bdff881a51ac56990164b0a5479bf6e47e64305a
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

    of/overlay: don't keep a negative id in ovcs->id on idr_alloc() failure

This commit fixes a bug where a negative return value from idr_alloc()
is stored in ovcs->id. On the error path, free_overlay_changeset() checks
if ovcs->id is non-zero, leading to idr_remove() being called with a
negative id and list_del() operating on an uninitialized list.

> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 284c9bc6c9cfc..f1aa8574069a6 100644
> --- 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;
>  		goto err_free_ovcs;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but callers of
of_overlay_fdt_apply() fail to call of_overlay_remove() when the
application fails, leading to a memory leak.

If of_overlay_apply() fails inside of_overlay_fdt_apply(), the function
intentionally leaves the returned changeset ID set via the pointer,
expecting the caller to handle cleanup by calling of_overlay_remove().
However, callers simply return the error code upon failure without
calling of_overlay_remove().

For example, in tilcdc_panel_legacy_probe():

drivers/gpu/drm/tilcdc/tilcdc_panel_legacy.c:tilcdc_panel_legacy_probe() {
    ...
	ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &ovcs_id, NULL);
	if (ret)
		return ret;
    ...
}

And in lan966x_pci_probe():

drivers/misc/lan966x_pci.c:lan966x_pci_probe() {
    ...
	ret = lan966x_pci_load_overlay(data);
	if (ret)
		return ret;
    ...
}

The helper returns the result directly to lan966x_pci_probe(), which bails
on error without invoking the corresponding lan966x_pci_unload_overlay().

Can this result in a permanent memory and ID leak of the overlay changeset,
especially since probe functions can be repeatedly triggered via sysfs
bind/unbind or deferred probing?


[Severity: High]
This isn't a bug introduced by this patch, but if of_changeset_attach_node()
fails in add_changeset_node(), is the newly allocated device node
permanently leaked?

In add_changeset_node(), __of_node_dup() creates a new node (tchild) with a
reference count of 1:

drivers/of/overlay.c:add_changeset_node() {
    ...
		ret = of_changeset_attach_node(&ovcs->cset, tchild);
		if (ret)
			return ret;
    ...
}

If of_changeset_attach_node() fails, the function returns the error
immediately without dropping the reference to tchild via of_node_put().

Could this leak the device node structure and its associated name string?


[Severity: High]
This isn't a bug introduced by this patch, but if init_overlay_changeset()
fails after successfully parsing one or more fragments, are the OF node
references for those initialized fragments leaked?

In init_overlay_changeset(), the function iterates over overlay fragments
and acquires references via of_get_child_by_name() and find_target(). If
find_target() fails for a subsequent fragment, it jumps to err_out without
dropping the current overlay_node reference or updating ovcs->count:

drivers/of/overlay.c:init_overlay_changeset() {
    ...
		fragment->target = find_target(node, target_base);
		if (!fragment->target) {
			of_node_put(node);
			ret = -EINVAL;
			goto err_out;
		}
    ...
}

Because ovcs->count remains 0, free_overlay_changeset() skips the cleanup
loop entirely:

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);
	}
    ...
}

Could this skip cleanup and leak the successfully acquired OF nodes from
previous iterations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-nh-of-alias-overlay-v5-0-2abe2bb9cdbc@nexthop.ai?part=5