On 11/22/22 18:29, Frank Rowand wrote:
> 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.
After reading throught the code some more, and confusing myself a bit,
I think the proposed patch of adding the of_node_put(tchild) is correct.
I'll run it through my tests and then reply again, hopefully tommorrow.
-Frank
>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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;
>
