Messages in this thread Patch in this message | | | Date | Fri, 17 Jun 2022 13:42:27 +0800 (CST) | From | "Liang He" <> | Subject | Re:Re: [PATCH] powerpc: powernv: Fix refcount leak bug in opal-powercap |
| |
At 2022-06-17 13:01:27, "Christophe JAILLET" <christophe.jaillet@wanadoo.fr> wrote: >Le 17/06/2022 à 06:20, Liang He a écrit : >> In opal_powercap_init(), of_find_compatible_node() will return >> a node pointer with refcount incremented. We should use of_node_put() >> in fail path or when it is not used anymore. >> >> Besides, for_each_child_of_node() will automatically *inc* and *dec* >> refcount during iteration. However, we should add the of_node_put() >> if there is a break. > >Hi, > >I'm not sure that your patch is right here. Because of this *inc* and >*dec* things, do we still need to of_node_put(powercap) once we have >entered for_each_child_of_node? > >I think that this reference will be released on the first iteration of >the loop. >
Hi, CJ,
Thanks for your reply and I want have a discuss.
Based on my review on the src of 'of_get_next_child', there is only *inc* for next and *dec* for pre as follow.
(|node| == powercap) ======__of_get_next_child( |node|, prev)====== ... next = prev? prev->sibling:|node|->child; of_node_get(next); of_node_put(prev); ... =========================
However, there is no any code to release the |node| (i.e., *powercap*).
Am I right? If I am wrong, please correct me, thanks.
> >Maybe of_node_put(powercap) should be duplicated everywhere it is >relevant and removed from the error handling path? >Or an additional reference should be taken before the loop? >Or adding a new label with "powercap = NULL" and branching there when >needed? > >CJ
If my understanding is right, I think current patch is right.
Otherwise, I will make a new patch to handle that, Thanks.
Liang
> >> >> Signed-off-by: Liang He <windhl@126.com> >> --- >> arch/powerpc/platforms/powernv/opal-powercap.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/platforms/powernv/opal-powercap.c b/arch/powerpc/platforms/powernv/opal-powercap.c >> index 64506b46e77b..b102477d3f95 100644 >> --- a/arch/powerpc/platforms/powernv/opal-powercap.c >> +++ b/arch/powerpc/platforms/powernv/opal-powercap.c >> @@ -153,7 +153,7 @@ void __init opal_powercap_init(void) >> pcaps = kcalloc(of_get_child_count(powercap), sizeof(*pcaps), >> GFP_KERNEL); >> if (!pcaps) >> - return; >> + goto out_powercap; >> >> powercap_kobj = kobject_create_and_add("powercap", opal_kobj); >> if (!powercap_kobj) { >> @@ -236,6 +236,9 @@ void __init opal_powercap_init(void) >> kfree(pcaps[i].pg.name); >> } >> kobject_put(powercap_kobj); >> + of_node_put(node); >> out_pcaps: >> kfree(pcaps); >> +out_powercap: >> + of_node_put(powercap); >> }
| |