1. Patchew
  2. linux
  3. View series

[PATCH] firmware: psci: Fix refcount leak in psci_dt_init

Miaoqian Lin posted 1 patch 9 months ago 
drivers/firmware/psci/psci.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
[PATCH] firmware: psci: Fix refcount leak in psci_dt_init
Posted by Miaoqian Lin 9 months ago 
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(-)

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)
Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
Posted by Will Deacon 7 months, 3 weeks ago 
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
Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
Posted by Gavin Shan 9 months ago 
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);
Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
Posted by Mark Rutland 9 months ago 
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)
>
Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
Posted by Gavin Shan 9 months ago 
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
Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
Posted by Sudeep Holla 9 months ago 
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;
 }
Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
Posted by Mark Rutland 9 months ago 
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.
Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init
Posted by Sudeep Holla 9 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.