* [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; 5+ 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] 5+ 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; 5+ 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] 5+ 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
[not found] ` <20221121035335.809316-1-zengheng4@huawei.com>
0 siblings, 1 reply; 5+ 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] 5+ messages in thread
* Re: [PATCH v2] of: overlay: fix memory leak in add_changeset_node()
[not found] ` <20221121035335.809316-1-zengheng4@huawei.com>
@ 2022-11-21 6:07 ` Zeng Heng
0 siblings, 0 replies; 5+ 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] 5+ 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; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2022-12-01 22:54 UTC | newest]
Thread overview: 5+ 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
[not found] ` <20221121035335.809316-1-zengheng4@huawei.com>
2022-11-21 6:07 ` [PATCH v2] " Zeng Heng
2022-12-01 22:54 ` [PATCH] " Frank Rowand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).