All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] of: overlay: fix memory leak in add_changeset_node()
@ 2022-11-18 10:53 Zeng Heng
  2022-11-21  2:00 ` Frank Rowand
  2022-12-01 22:54 ` [PATCH] " Frank Rowand
  0 siblings, 2 replies; 10+ messages in thread
From: Zeng Heng @ 2022-11-18 10:53 UTC (permalink / raw)
  To: frowand.list, pantelis.antoniou, grant.likely, robh+dt
  Cc: liwei391, devicetree, linux-kernel

In of_changeset_action(), we have called of_node_get() to increase
refcount of a node.

Therefore, when tchild (duplicated by __of_node_dup()) is done,
of_node_put() needs to call and release the device_node.

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: 7518b5890d8a ("of/overlay: Introduce DT overlay support")
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;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] of: overlay: fix memory leak in add_changeset_node()
  2022-11-18 10:53 [PATCH] of: overlay: fix memory leak in add_changeset_node() Zeng Heng
@ 2022-11-21  2:00 ` Frank Rowand
  2022-11-21  3:37   ` Zeng Heng
  2022-12-01 22:54 ` [PATCH] " Frank Rowand
  1 sibling, 1 reply; 10+ messages in thread
From: Frank Rowand @ 2022-11-21  2:00 UTC (permalink / raw)
  To: Zeng Heng, pantelis.antoniou, grant.likely, robh+dt
  Cc: liwei391, devicetree, linux-kernel

On 11/18/22 04:53, Zeng Heng wrote:
> In of_changeset_action(), we have called of_node_get() to increase
> refcount of a node.
> 
> Therefore, when tchild (duplicated by __of_node_dup()) is done,
> of_node_put() needs to call and release the device_node.
> 
> 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: 7518b5890d8a ("of/overlay: Introduce DT overlay support")
> 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;
> +		}

By visual inspection of the code (Linux 6.1-rc1), this does not appear to be
correct.  For the only case where of_changeset_action(),
called by of_changeset_attach_node(), returns an error, of_node_get() has not
yet occurred on tchild.

>  
>  		target_child.np = tchild;
>  		target_child.in_livetree = false;

For which version of Linux were the memory leaks detected?  Were any additional
patches applied?

-Frank

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] of: overlay: fix memory leak in add_changeset_node()
  2022-11-21  2:00 ` Frank Rowand
@ 2022-11-21  3:37   ` Zeng Heng
  2022-11-21  3:53     ` [PATCH v2] " Zeng Heng
  0 siblings, 1 reply; 10+ messages in thread
From: Zeng Heng @ 2022-11-21  3:37 UTC (permalink / raw)
  To: Frank Rowand, pantelis.antoniou, grant.likely, robh+dt
  Cc: liwei391, devicetree, linux-kernel


On 2022/11/21 10:00, Frank Rowand wrote:
> On 11/18/22 04:53, Zeng Heng wrote:
>> In of_changeset_action(), we have called of_node_get() to increase
>> refcount of a node.
>>
>> Therefore, when tchild (duplicated by __of_node_dup()) is done,
>> of_node_put() needs to call and release the device_node.
>>
>> 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: 7518b5890d8a ("of/overlay: Introduce DT overlay support")
>> 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;
>> +		}
> By visual inspection of the code (Linux 6.1-rc1), this does not appear to be
> correct.  For the only case where of_changeset_action(),
> called by of_changeset_attach_node(), returns an error, of_node_get() has not
> yet occurred on tchild.

The correct explanation should be like:

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.


The patch is fine, but feel sorry to apologize for the incorrect comment 
of the patch.

I would update the comment in the patch v2.


With Best Regards,

Zeng Heng

>>   
>>   		target_child.np = tchild;
>>   		target_child.in_livetree = false;
> For which version of Linux were the memory leaks detected?  Were any additional
> patches applied?
>
> -Frank

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH v2] of: overlay: fix memory leak in add_changeset_node()
  2022-11-21  3:37   ` Zeng Heng
@ 2022-11-21  3:53     ` Zeng Heng
  2022-11-21  6:07       ` Zeng Heng
  2022-11-23  0:29       ` Frank Rowand
  0 siblings, 2 replies; 10+ messages in thread
From: Zeng Heng @ 2022-11-21  3:53 UTC (permalink / raw)
  To: robh+dt, frowand.list; +Cc: devicetree, zengheng4, liwei391

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.

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")
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;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] of: overlay: fix memory leak in add_changeset_node()
  2022-11-21  3:53     ` [PATCH v2] " Zeng Heng
@ 2022-11-21  6:07       ` Zeng Heng
  2022-11-23  0:29       ` Frank Rowand
  1 sibling, 0 replies; 10+ messages in thread
From: Zeng Heng @ 2022-11-21  6:07 UTC (permalink / raw)
  To: robh+dt, frowand.list, Frank Rowand, pantelis.antoniou,
	grant.likely, robh+dt
  Cc: devicetree, liwei391, Linux Kernel Mailing List

+cc linux-kernel-mail-list


On 2022/11/21 11: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.
>
> 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")
> 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;

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] of: overlay: fix memory leak in add_changeset_node()
  2022-11-21  3:53     ` [PATCH v2] " Zeng Heng
  2022-11-21  6:07       ` Zeng Heng
@ 2022-11-23  0:29       ` Frank Rowand
  2022-11-23  1:16         ` Frank Rowand
  2022-11-23  1:41         ` Zeng Heng
  1 sibling, 2 replies; 10+ messages in thread
From: Frank Rowand @ 2022-11-23  0:29 UTC (permalink / raw)
  To: Zeng Heng, robh+dt; +Cc: devicetree, liwei391

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;


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] of: overlay: fix memory leak in add_changeset_node()
  2022-11-23  0:29       ` Frank Rowand
@ 2022-11-23  1:16         ` Frank Rowand
  2022-11-23  1:52           ` Zeng Heng
  2022-11-23  1:41         ` Zeng Heng
  1 sibling, 1 reply; 10+ messages in thread
From: Frank Rowand @ 2022-11-23  1:16 UTC (permalink / raw)
  To: Zeng Heng, robh+dt; +Cc: devicetree, liwei391

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@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;
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] of: overlay: fix memory leak in add_changeset_node()
  2022-11-23  0:29       ` Frank Rowand
  2022-11-23  1:16         ` Frank Rowand
@ 2022-11-23  1:41         ` Zeng Heng
  1 sibling, 0 replies; 10+ messages in thread
From: Zeng Heng @ 2022-11-23  1:41 UTC (permalink / raw)
  To: Frank Rowand, robh+dt; +Cc: devicetree, liwei391


On 2022/11/23 8: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...

Get it. Thanks for your suggestion.

With B. R.,

Zeng Heng

>
> 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;

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] of: overlay: fix memory leak in add_changeset_node()
  2022-11-23  1:16         ` Frank Rowand
@ 2022-11-23  1:52           ` Zeng Heng
  0 siblings, 0 replies; 10+ messages in thread
From: Zeng Heng @ 2022-11-23  1:52 UTC (permalink / raw)
  To: Frank Rowand, robh+dt; +Cc: devicetree, liwei391


On 2022/11/23 9:16, Frank Rowand wrote:
> 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

Many thanks to your patient review.


My work is injecting fault(like ENOMEM by failslab) into every corners 
per single time,

so i would send the corresponding patch even if the probability of error 
is low.


And continue digging.


With best regards,

Zeng Heng

>
>>> 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;

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] of: overlay: fix memory leak in add_changeset_node()
  2022-11-18 10:53 [PATCH] of: overlay: fix memory leak in add_changeset_node() Zeng Heng
  2022-11-21  2:00 ` Frank Rowand
@ 2022-12-01 22:54 ` Frank Rowand
  1 sibling, 0 replies; 10+ messages in thread
From: Frank Rowand @ 2022-12-01 22:54 UTC (permalink / raw)
  To: Zeng Heng, pantelis.antoniou, grant.likely, robh+dt
  Cc: liwei391, devicetree, linux-kernel

On 11/18/22 04:53, Zeng Heng wrote:
> In of_changeset_action(), we have called of_node_get() to increase
> refcount of a node.
> 
> Therefore, when tchild (duplicated by __of_node_dup()) is done,
> of_node_put() needs to call and release the device_node.
> 
> 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: 7518b5890d8a ("of/overlay: Introduce DT overlay support")
> 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;

Pending updated Fixes: tag (mentioned in a previous reply).

Reviewed-by: Frank Rowand <frowand.list@gmail.com>
Tested-by: Frank Rowand <frowand.list@gmail.com>

The testing was my normal testing, but did not replicate the triggered warning
to verify that this patch eliminated the warning.  I am depending upon Zeng having
verified the elimination of the warning.

-Frank


^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-12-01 22:54 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-18 10:53 [PATCH] of: overlay: fix memory leak in add_changeset_node() Zeng Heng
2022-11-21  2:00 ` Frank Rowand
2022-11-21  3:37   ` Zeng Heng
2022-11-21  3:53     ` [PATCH v2] " Zeng Heng
2022-11-21  6:07       ` Zeng Heng
2022-11-23  0:29       ` Frank Rowand
2022-11-23  1:16         ` Frank Rowand
2022-11-23  1:52           ` Zeng Heng
2022-11-23  1:41         ` Zeng Heng
2022-12-01 22:54 ` [PATCH] " Frank Rowand

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.