| Message ID | 20220309102442.14726-1-linmq006@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/xen: Fix refcount leak in xen_dt_guest_init | expand |
On Wed, 9 Mar 2022, Miaoqian Lin wrote: > The of_find_compatible_node() function returns a node pointer with > refcount incremented, We should use of_node_put() on it when done > Add the missing of_node_put() to release the refcount. > > Fixes: 9b08aaa3199a ("ARM: XEN: Move xen_early_init() before efi_init()") > Signed-off-by: Miaoqian Lin <linmq006@gmail.com> Thanks for the patch! > --- > arch/arm/xen/enlighten.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c > index ec5b082f3de6..262f45f686b6 100644 > --- a/arch/arm/xen/enlighten.c > +++ b/arch/arm/xen/enlighten.c > @@ -424,6 +424,7 @@ static void __init xen_dt_guest_init(void) > > if (of_address_to_resource(xen_node, GRANT_TABLE_INDEX, &res)) { > pr_err("Xen grant table region is not found\n"); > + of_node_put(xen_node); > return; > } This is adding a call to of_node_put on the error path. Shouldn't it be called also in the non-error path? Also, there is another instance of of_address_to_resource being called in this file (in arch_xen_unpopulated_init), does it make sense to call of_node_put there too?
Hi, On Fri, Mar 11, 2022 at 06:01:11PM -0800, Stefano Stabellini wrote: > On Wed, 9 Mar 2022, Miaoqian Lin wrote: > > The of_find_compatible_node() function returns a node pointer with > > refcount incremented, We should use of_node_put() on it when done > > Add the missing of_node_put() to release the refcount. > > > > Fixes: 9b08aaa3199a ("ARM: XEN: Move xen_early_init() before efi_init()") > > Signed-off-by: Miaoqian Lin <linmq006@gmail.com> > > Thanks for the patch! > > > > --- > > arch/arm/xen/enlighten.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c > > index ec5b082f3de6..262f45f686b6 100644 > > --- a/arch/arm/xen/enlighten.c > > +++ b/arch/arm/xen/enlighten.c > > @@ -424,6 +424,7 @@ static void __init xen_dt_guest_init(void) > > > > if (of_address_to_resource(xen_node, GRANT_TABLE_INDEX, &res)) { > > pr_err("Xen grant table region is not found\n"); > > + of_node_put(xen_node); > > return; > > } > > This is adding a call to of_node_put on the error path. Shouldn't it > be called also in the non-error path? You're right. It should be called also in the non-error path. I made a mistake. > Also, there is another instance of of_address_to_resource being called > in this file (in arch_xen_unpopulated_init), does it make sense to call > of_node_put there too? I think so, becase device node pointer np is a local variable. So the reference it taken should be released in the scope. I look into the whole codebase for this kind of usage pattern ($ret=of_find_compatible_node();of_address_to_resource($ret,_,_), $ret is a local variable), Most of them call of_node_put() when done. And document of of_find_compatible_node() also mentions > Return: A node pointer with refcount incremented, use > of_node_put() on it when done. But I am not sure, Since I am unfamiliar with other code logic. It better if the developers could double check. I found some similar cases in arch/arm.
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c index ec5b082f3de6..262f45f686b6 100644 --- a/arch/arm/xen/enlighten.c +++ b/arch/arm/xen/enlighten.c @@ -424,6 +424,7 @@ static void __init xen_dt_guest_init(void) if (of_address_to_resource(xen_node, GRANT_TABLE_INDEX, &res)) { pr_err("Xen grant table region is not found\n"); + of_node_put(xen_node); return; } xen_grant_frames = res.start;
The of_find_compatible_node() function returns a node pointer with refcount incremented, We should use of_node_put() on it when done Add the missing of_node_put() to release the refcount. Fixes: 9b08aaa3199a ("ARM: XEN: Move xen_early_init() before efi_init()") Signed-off-by: Miaoqian Lin <linmq006@gmail.com> --- arch/arm/xen/enlighten.c | 1 + 1 file changed, 1 insertion(+)