From: Miaoqian Lin <linmq006@gmail.com> To: Stefano Stabellini <sstabellini@kernel.org> Cc: Russell King <linux@armlinux.org.uk>, Catalin Marinas <catalin.marinas@arm.com>, Shannon Zhao <shannon.zhao@linaro.org>, Julien Grall <julien.grall@arm.com>, xen-devel@lists.xenproject.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] arm/xen: Fix refcount leak in xen_dt_guest_init Date: Wed, 6 Apr 2022 02:19:05 +0000 [thread overview] Message-ID: <20220406021905.GA30436@8345e2a12d0c> (raw) In-Reply-To: <alpine.DEB.2.22.394.2203111756000.3497@ubuntu-linux-20-04-desktop> 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.
WARNING: multiple messages have this Message-ID (diff)
From: Miaoqian Lin <linmq006@gmail.com> To: Stefano Stabellini <sstabellini@kernel.org> Cc: Russell King <linux@armlinux.org.uk>, Catalin Marinas <catalin.marinas@arm.com>, Shannon Zhao <shannon.zhao@linaro.org>, Julien Grall <julien.grall@arm.com>, xen-devel@lists.xenproject.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] arm/xen: Fix refcount leak in xen_dt_guest_init Date: Wed, 6 Apr 2022 02:19:05 +0000 [thread overview] Message-ID: <20220406021905.GA30436@8345e2a12d0c> (raw) In-Reply-To: <alpine.DEB.2.22.394.2203111756000.3497@ubuntu-linux-20-04-desktop> 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-04-06 14:53 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-09 10:24 [PATCH] arm/xen: Fix refcount leak in xen_dt_guest_init Miaoqian Lin 2022-03-09 10:24 ` Miaoqian Lin 2022-03-12 2:01 ` Stefano Stabellini 2022-03-12 2:01 ` Stefano Stabellini 2022-04-06 2:19 ` Miaoqian Lin [this message] 2022-04-06 2:19 ` Miaoqian Lin 2022-04-20 1:49 ` [PATCH v2] arm/xen: Fix some refcount leaks Miaoqian Lin 2022-04-20 1:49 ` Miaoqian Lin 2022-04-22 0:36 ` Stefano Stabellini 2022-04-22 0:36 ` Stefano Stabellini
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220406021905.GA30436@8345e2a12d0c \ --to=linmq006@gmail.com \ --cc=catalin.marinas@arm.com \ --cc=julien.grall@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=shannon.zhao@linaro.org \ --cc=sstabellini@kernel.org \ --cc=xen-devel@lists.xenproject.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.