Re: [PATCH v2] of: overlay: fix memory leak in add_changeset_node()

[Frank Rowand <frowand.list@gmail.com>]

Hi Zeng,

In the future, please do not send a new version of a patch series as a reply
to a previous thread.  For people who leave threads collapsed in their email
client (like me), there is a good chance that the new patch version email
will not be noticed.

More below...

On 11/20/22 21:53, Zeng Heng wrote:

> When of_changeset_attach_node() returns fail and tchild is
> over of life cycle which is duplicated by __of_node_dup(),
> it needs to call of_node_put() to release tchild in
> error handle route.

This does not seem correct.  I will explain this in the patch v1
thread.

> 
> Otherwise, there are some memory leak reported about the node:
> 
> unreferenced object 0xffff88810cd1e800 (size 256):
>   backtrace:
>     kmalloc_trace
>     __of_node_dup
>     add_changeset_node (inlined)
>     build_changeset_next_level
> 
> unreferenced object 0xffff888113721240 (size 16):
>   backtrace:
>     __kmalloc_node_track_caller
>     kstrdup
>     __of_node_dup
>     add_changeset_node (inlined)
>     build_changeset_next_level
> 
> unreferenced object 0xffff88810a38d400 (size 128):
>   backtrace:
>     kmalloc_trace
>     __of_prop_dup
>     add_changeset_property
>     build_changeset_next_level
> 

> Fixes: 0290c4ca2536 ("of: overlay: rename identifiers to more reflect what they do")

You have to dig deeper.  The code that introduced the issue is even older:

7518b5890d8a of/overlay: Introduce DT overlay support

-Frank


> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> ---
>  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 bd8ff4df723d..a5189a0ec0a3 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -436,8 +436,10 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
>  		of_node_set_flag(tchild, OF_OVERLAY);
>  
>  		ret = of_changeset_attach_node(&ovcs->cset, tchild);
> -		if (ret)
> +		if (ret) {
> +			of_node_put(tchild);
>  			return ret;
> +		}
>  
>  		target_child.np = tchild;
>  		target_child.in_livetree = false;