* Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
2025-03-18 15:17 [PATCH] firmware: psci: Fix refcount leak in psci_dt_init Miaoqian Lin
@ 2025-03-19 10:28 ` Gavin Shan
2025-03-19 10:46 ` Sudeep Holla
2025-03-19 11:23 ` Mark Rutland
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Gavin Shan @ 2025-03-19 10:28 UTC (permalink / raw)
To: Miaoqian Lin, Mark Rutland, Lorenzo Pieralisi, Will Deacon,
Hanjun Guo, Catalin Marinas, linux-arm-kernel, linux-kernel
Hi Miaoqian,
On 3/19/25 1:17 AM, Miaoqian Lin wrote:
> Fix a reference counter leak in psci_dt_init() where of_node_put(np) was
> missing after of_find_matching_node_and_match() when np is unavailable.
>
> Fixes: bff60792f994 ("arm64: psci: factor invocation code to drivers")
> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
> ---
> drivers/firmware/psci/psci.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
I'm wandering if the fix tag is correct enough because !of_device_is_available(np)
wasn't added by bff60792f994.
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index a1ebbe9b73b1..38ca190d4a22 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -804,8 +804,10 @@ int __init psci_dt_init(void)
>
> np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
>
> - if (!np || !of_device_is_available(np))
> + if (!np || !of_device_is_available(np)) {
> + of_node_put(np);
> return -ENODEV;
> + }
The fix looks good to me. The duplicated of_node_put() can be avoided with
a 'out' tag added, something like below.
if (!np || !of_device_is_available(np)) {
ret = -ENODEV;
goto out;
}
:
out:
of_node_put(np);
return ret;
>
> init_fn = (psci_initcall_t)matched_np->data;
> ret = init_fn(np);
Thanks,
Gavin
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
2025-03-19 10:28 ` Gavin Shan
@ 2025-03-19 10:46 ` Sudeep Holla
2025-03-19 11:12 ` Mark Rutland
0 siblings, 1 reply; 8+ messages in thread
From: Sudeep Holla @ 2025-03-19 10:46 UTC (permalink / raw)
To: Miaoqian Lin, Gavin Shan
Cc: Mark Rutland, Sudeep Holla, Lorenzo Pieralisi, Will Deacon,
Hanjun Guo, Catalin Marinas, linux-arm-kernel, linux-kernel
On Wed, Mar 19, 2025 at 08:28:38PM +1000, Gavin Shan wrote:
> Hi Miaoqian,
>
> On 3/19/25 1:17 AM, Miaoqian Lin wrote:
> > Fix a reference counter leak in psci_dt_init() where of_node_put(np) was
> > missing after of_find_matching_node_and_match() when np is unavailable.
> >
> > Fixes: bff60792f994 ("arm64: psci: factor invocation code to drivers")
> > Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
> > ---
> > drivers/firmware/psci/psci.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
>
> I'm wandering if the fix tag is correct enough because !of_device_is_available(np)
> wasn't added by bff60792f994.
>
> > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > index a1ebbe9b73b1..38ca190d4a22 100644
> > --- a/drivers/firmware/psci/psci.c
> > +++ b/drivers/firmware/psci/psci.c
> > @@ -804,8 +804,10 @@ int __init psci_dt_init(void)
> > np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
> > - if (!np || !of_device_is_available(np))
> > + if (!np || !of_device_is_available(np)) {
> > + of_node_put(np);
> > return -ENODEV;
> > + }
>
> The fix looks good to me. The duplicated of_node_put() can be avoided with
> a 'out' tag added, something like below.
>
> if (!np || !of_device_is_available(np)) {
> ret = -ENODEV;
> goto out;
> }
>
> :
>
> out:
> of_node_put(np);
> return ret;
>
> > init_fn = (psci_initcall_t)matched_np->data;
> > ret = init_fn(np);
>
Any reason why we can't move to the new scoped usage like below?
--
Regards,
Sudeep
-->8
diff --git i/drivers/firmware/psci/psci.c w/drivers/firmware/psci/psci.c
index a1ebbe9b73b1..a4269078b2a2 100644
--- i/drivers/firmware/psci/psci.c
+++ w/drivers/firmware/psci/psci.c
@@ -797,12 +797,11 @@ static const struct of_device_id psci_of_match[] __initconst = {
int __init psci_dt_init(void)
{
- struct device_node *np;
const struct of_device_id *matched_np;
psci_initcall_t init_fn;
int ret;
-
- np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
+ struct device_node *np __free(device_node) =
+ of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
if (!np || !of_device_is_available(np))
return -ENODEV;
@@ -810,7 +809,6 @@ int __init psci_dt_init(void)
init_fn = (psci_initcall_t)matched_np->data;
ret = init_fn(np);
- of_node_put(np);
return ret;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
2025-03-19 10:46 ` Sudeep Holla
@ 2025-03-19 11:12 ` Mark Rutland
2025-03-19 11:39 ` Sudeep Holla
0 siblings, 1 reply; 8+ messages in thread
From: Mark Rutland @ 2025-03-19 11:12 UTC (permalink / raw)
To: Sudeep Holla
Cc: Miaoqian Lin, Gavin Shan, Lorenzo Pieralisi, Will Deacon,
Hanjun Guo, Catalin Marinas, linux-arm-kernel, linux-kernel
On Wed, Mar 19, 2025 at 10:46:49AM +0000, Sudeep Holla wrote:
> On Wed, Mar 19, 2025 at 08:28:38PM +1000, Gavin Shan wrote:
> Any reason why we can't move to the new scoped usage like below?
> diff --git i/drivers/firmware/psci/psci.c w/drivers/firmware/psci/psci.c
> index a1ebbe9b73b1..a4269078b2a2 100644
> --- i/drivers/firmware/psci/psci.c
> +++ w/drivers/firmware/psci/psci.c
> @@ -797,12 +797,11 @@ static const struct of_device_id psci_of_match[] __initconst = {
>
> int __init psci_dt_init(void)
> {
> - struct device_node *np;
> const struct of_device_id *matched_np;
> psci_initcall_t init_fn;
> int ret;
> -
> - np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
> + struct device_node *np __free(device_node) =
> + of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
>
> if (!np || !of_device_is_available(np))
> return -ENODEV;
> @@ -810,7 +809,6 @@ int __init psci_dt_init(void)
> init_fn = (psci_initcall_t)matched_np->data;
> ret = init_fn(np);
>
> - of_node_put(np);
> return ret;
> }
IIUC the bug has existed since before the scoped helpers were
introduced, so for the sake of keeping the backport simple I reckon we
should take Miaoqian Lin's patch as-is, and we can consider moving to
the scoped helpers as a later cleanup.
Mark.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
2025-03-19 11:12 ` Mark Rutland
@ 2025-03-19 11:39 ` Sudeep Holla
0 siblings, 0 replies; 8+ messages in thread
From: Sudeep Holla @ 2025-03-19 11:39 UTC (permalink / raw)
To: Mark Rutland
Cc: Miaoqian Lin, Gavin Shan, Lorenzo Pieralisi, Will Deacon,
Hanjun Guo, Catalin Marinas, linux-arm-kernel, linux-kernel
On Wed, Mar 19, 2025 at 11:12:02AM +0000, Mark Rutland wrote:
> On Wed, Mar 19, 2025 at 10:46:49AM +0000, Sudeep Holla wrote:
> > On Wed, Mar 19, 2025 at 08:28:38PM +1000, Gavin Shan wrote:
>
> > Any reason why we can't move to the new scoped usage like below?
>
> > diff --git i/drivers/firmware/psci/psci.c w/drivers/firmware/psci/psci.c
> > index a1ebbe9b73b1..a4269078b2a2 100644
> > --- i/drivers/firmware/psci/psci.c
> > +++ w/drivers/firmware/psci/psci.c
> > @@ -797,12 +797,11 @@ static const struct of_device_id psci_of_match[] __initconst = {
> >
> > int __init psci_dt_init(void)
> > {
> > - struct device_node *np;
> > const struct of_device_id *matched_np;
> > psci_initcall_t init_fn;
> > int ret;
> > -
> > - np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
> > + struct device_node *np __free(device_node) =
> > + of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
> >
> > if (!np || !of_device_is_available(np))
> > return -ENODEV;
> > @@ -810,7 +809,6 @@ int __init psci_dt_init(void)
> > init_fn = (psci_initcall_t)matched_np->data;
> > ret = init_fn(np);
> >
> > - of_node_put(np);
> > return ret;
> > }
>
> IIUC the bug has existed since before the scoped helpers were
> introduced, so for the sake of keeping the backport simple I reckon we
> should take Miaoqian Lin's patch as-is, and we can consider moving to
> the scoped helpers as a later cleanup.
>
Agreed.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
2025-03-18 15:17 [PATCH] firmware: psci: Fix refcount leak in psci_dt_init Miaoqian Lin
2025-03-19 10:28 ` Gavin Shan
@ 2025-03-19 11:23 ` Mark Rutland
2025-03-19 23:30 ` Gavin Shan
2025-04-29 20:27 ` Will Deacon
3 siblings, 0 replies; 8+ messages in thread
From: Mark Rutland @ 2025-03-19 11:23 UTC (permalink / raw)
To: Miaoqian Lin
Cc: Lorenzo Pieralisi, Will Deacon, Hanjun Guo, Catalin Marinas,
linux-arm-kernel, linux-kernel
On Tue, Mar 18, 2025 at 11:17:12PM +0800, Miaoqian Lin wrote:
> Fix a reference counter leak in psci_dt_init() where of_node_put(np) was
> missing after of_find_matching_node_and_match() when np is unavailable.
>
> Fixes: bff60792f994 ("arm64: psci: factor invocation code to drivers")
> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
The fixes tag is wrong. As of commit bff60792f994 the code was:
| np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
|
| if (!np)
| return -ENODEV;
... which was correct.
The bug was introduced later in commit:
d09a0011ec0d511b ("drivers: psci: Allow PSCI node to be disabled")
... which added the of_device_is_available() check.
Other than that, this looks fine. With the fixes tag corrected:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> drivers/firmware/psci/psci.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index a1ebbe9b73b1..38ca190d4a22 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -804,8 +804,10 @@ int __init psci_dt_init(void)
>
> np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
>
> - if (!np || !of_device_is_available(np))
> + if (!np || !of_device_is_available(np)) {
> + of_node_put(np);
> return -ENODEV;
> + }
>
> init_fn = (psci_initcall_t)matched_np->data;
> ret = init_fn(np);
> --
> 2.39.5 (Apple Git-154)
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
2025-03-18 15:17 [PATCH] firmware: psci: Fix refcount leak in psci_dt_init Miaoqian Lin
2025-03-19 10:28 ` Gavin Shan
2025-03-19 11:23 ` Mark Rutland
@ 2025-03-19 23:30 ` Gavin Shan
2025-04-29 20:27 ` Will Deacon
3 siblings, 0 replies; 8+ messages in thread
From: Gavin Shan @ 2025-03-19 23:30 UTC (permalink / raw)
To: Miaoqian Lin, Mark Rutland, Lorenzo Pieralisi, Will Deacon,
Hanjun Guo, Catalin Marinas, linux-arm-kernel, linux-kernel
On 3/19/25 1:17 AM, Miaoqian Lin wrote:
> Fix a reference counter leak in psci_dt_init() where of_node_put(np) was
> missing after of_find_matching_node_and_match() when np is unavailable.
>
> Fixes: bff60792f994 ("arm64: psci: factor invocation code to drivers")
> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
> ---
> drivers/firmware/psci/psci.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
With the fix tag corrected to the one, suggested by Mark.
Reviewed-by: Gavin Shan <gshan@redhat.com>
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index a1ebbe9b73b1..38ca190d4a22 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -804,8 +804,10 @@ int __init psci_dt_init(void)
>
> np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
>
> - if (!np || !of_device_is_available(np))
> + if (!np || !of_device_is_available(np)) {
> + of_node_put(np);
> return -ENODEV;
> + }
>
> init_fn = (psci_initcall_t)matched_np->data;
> ret = init_fn(np);
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
2025-03-18 15:17 [PATCH] firmware: psci: Fix refcount leak in psci_dt_init Miaoqian Lin
` (2 preceding siblings ...)
2025-03-19 23:30 ` Gavin Shan
@ 2025-04-29 20:27 ` Will Deacon
3 siblings, 0 replies; 8+ messages in thread
From: Will Deacon @ 2025-04-29 20:27 UTC (permalink / raw)
To: Mark Rutland, Lorenzo Pieralisi, Hanjun Guo, Catalin Marinas,
linux-arm-kernel, linux-kernel, Miaoqian Lin
Cc: kernel-team, Will Deacon
On Tue, 18 Mar 2025 23:17:12 +0800, Miaoqian Lin wrote:
> Fix a reference counter leak in psci_dt_init() where of_node_put(np) was
> missing after of_find_matching_node_and_match() when np is unavailable.
>
>
Applied to arm64 (for-next/psci), thanks!
[1/1] firmware: psci: Fix refcount leak in psci_dt_init
https://git.kernel.org/arm64/c/7ff37d29fd5c
Cheers,
--
Will
https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
^ permalink raw reply [flat|nested] 8+ messages in thread