irqchip/gic-v3: fix resource leak in partition_domain_translate()

[PATCH] irqchip/gic-v3: fix resource leak in partition_domain_translate()

Posted by jackysliu 2 months, 2 weeks ago

There is a device node reference leak in partition_domain_translate().
After the function obtains the device node np via of_find_node_by_phandle,
it does not call of_node_put(np) to release the node reference
in both the error path and the normal return path.
This causes the node reference count to increase each time
the function is called, causing a resource leak.

This issue was detected by rule based static tools
developed by Tencent.

Fixes: 87228532e7e9 ("irqchip: Switch to of_fwnode_handle()")

Signed-off-by: jackysliu <1972843537@qq.com>
---
 drivers/irqchip/irq-gic-v3.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index efc791c43d44..61c1d404b726 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1821,12 +1821,16 @@ static int partition_domain_translate(struct irq_domain *d,
 		return -EINVAL;
 
 	ret = gic_irq_domain_translate(d, fwspec, &ppi_intid, type);
-	if (WARN_ON_ONCE(ret))
+	if (WARN_ON_ONCE(ret)) {
+		of_node_put(np);
 		return 0;
+	}
 
 	ppi_idx = __gic_get_ppi_index(ppi_intid);
 	ret = partition_translate_id(gic_data.ppi_descs[ppi_idx],
 				     of_fwnode_handle(np));
+	of_node_put(np);
+
 	if (ret < 0)
 		return ret;
 
-- 
2.43.5

Re: [PATCH] irqchip/gic-v3: fix resource leak in partition_domain_translate()

Posted by Marc Zyngier 2 months, 2 weeks ago

On Mon, 21 Jul 2025 08:28:04 +0100,
jackysliu <1972843537@qq.com> wrote:
> 
> There is a device node reference leak in partition_domain_translate().
> After the function obtains the device node np via of_find_node_by_phandle,
> it does not call of_node_put(np) to release the node reference
> in both the error path and the normal return path.
> This causes the node reference count to increase each time
> the function is called, causing a resource leak.
>
> This issue was detected by rule based static tools
> developed by Tencent.
> 
> Fixes: 87228532e7e9 ("irqchip: Switch to of_fwnode_handle()")
> 
> Signed-off-by: jackysliu <1972843537@qq.com>

Drop the spurious blank line.

> ---
>  drivers/irqchip/irq-gic-v3.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index efc791c43d44..61c1d404b726 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1821,12 +1821,16 @@ static int partition_domain_translate(struct irq_domain *d,
>  		return -EINVAL;
>  
>  	ret = gic_irq_domain_translate(d, fwspec, &ppi_intid, type);
> -	if (WARN_ON_ONCE(ret))
> +	if (WARN_ON_ONCE(ret)) {
> +		of_node_put(np);
>  		return 0;
> +	}
>  
>  	ppi_idx = __gic_get_ppi_index(ppi_intid);
>  	ret = partition_translate_id(gic_data.ppi_descs[ppi_idx],
>  				     of_fwnode_handle(np));
> +	of_node_put(np);
> +
>  	if (ret < 0)
>  		return ret;
>  

Frankly, this looks awful, and we have much better ways to solve this
whole class of problems. Why can't the (untested) patch below do the
right thing, without the ugliness?

	M.

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index efc791c43d441..c4839032ce8d0 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1808,8 +1808,8 @@ static int partition_domain_translate(struct irq_domain *d,
 				      unsigned long *hwirq,
 				      unsigned int *type)
 {
+	struct device_node *np __free(device_node) = NULL;
 	unsigned long ppi_intid;
-	struct device_node *np;
 	unsigned int ppi_idx;
 	int ret;
 

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] irqchip/gic-v3: fix resource leak in partition_domain_translate()

Posted by Jonathan Cameron 2 months, 2 weeks ago

On Mon, 21 Jul 2025 09:25:53 +0100
Marc Zyngier <maz@kernel.org> wrote:

> On Mon, 21 Jul 2025 08:28:04 +0100,
> jackysliu <1972843537@qq.com> wrote:
> > 
> > There is a device node reference leak in partition_domain_translate().
> > After the function obtains the device node np via of_find_node_by_phandle,
> > it does not call of_node_put(np) to release the node reference
> > in both the error path and the normal return path.
> > This causes the node reference count to increase each time
> > the function is called, causing a resource leak.
> >
> > This issue was detected by rule based static tools
> > developed by Tencent.
> > 
> > Fixes: 87228532e7e9 ("irqchip: Switch to of_fwnode_handle()")
> > 
> > Signed-off-by: jackysliu <1972843537@qq.com>  
> 
> Drop the spurious blank line.
> 
> > ---
> >  drivers/irqchip/irq-gic-v3.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index efc791c43d44..61c1d404b726 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -1821,12 +1821,16 @@ static int partition_domain_translate(struct irq_domain *d,
> >  		return -EINVAL;
> >  
> >  	ret = gic_irq_domain_translate(d, fwspec, &ppi_intid, type);
> > -	if (WARN_ON_ONCE(ret))
> > +	if (WARN_ON_ONCE(ret)) {
> > +		of_node_put(np);
> >  		return 0;
> > +	}
> >  
> >  	ppi_idx = __gic_get_ppi_index(ppi_intid);
> >  	ret = partition_translate_id(gic_data.ppi_descs[ppi_idx],
> >  				     of_fwnode_handle(np));
> > +	of_node_put(np);
> > +
> >  	if (ret < 0)
> >  		return ret;
> >    
> 
> Frankly, this looks awful, and we have much better ways to solve this
> whole class of problems. Why can't the (untested) patch below do the
> right thing, without the ugliness?
> 
> 	M.
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index efc791c43d441..c4839032ce8d0 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1808,8 +1808,8 @@ static int partition_domain_translate(struct irq_domain *d,
>  				      unsigned long *hwirq,
>  				      unsigned int *type)
>  {
> +	struct device_node *np __free(device_node) = NULL;

Linus has expressed fairly strongly that he really doesn't like separation
of the constructor and destructor.  See guidance in cleanup.h which was
based on that and bunch of other early discussion around this stuff.

Here it's easy to solve though. Just move that declaration down to
give something like:


>  	unsigned long ppi_intid;
> -	struct device_node *np;
>  	unsigned int ppi_idx;
>  	int ret;
>  
	if (!gic_data.ppi_descs)
		return -ENOMEM;

	struct device_node *np __free(device_node) =
		of_find_node_by_phandle(fwspec->param[3]);


	if (WARN_ON(!np))
		return -EINVAL;

Re: [PATCH] irqchip/gic-v3: fix resource leak in partition_domain_translate()

Posted by Marc Zyngier 2 months, 2 weeks ago

On Mon, 21 Jul 2025 11:27:18 +0100,
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> On Mon, 21 Jul 2025 09:25:53 +0100
> Marc Zyngier <maz@kernel.org> wrote:
> 
> > On Mon, 21 Jul 2025 08:28:04 +0100,
> > jackysliu <1972843537@qq.com> wrote:
> > > 
> > > There is a device node reference leak in partition_domain_translate().
> > > After the function obtains the device node np via of_find_node_by_phandle,
> > > it does not call of_node_put(np) to release the node reference
> > > in both the error path and the normal return path.
> > > This causes the node reference count to increase each time
> > > the function is called, causing a resource leak.
> > >
> > > This issue was detected by rule based static tools
> > > developed by Tencent.
> > > 
> > > Fixes: 87228532e7e9 ("irqchip: Switch to of_fwnode_handle()")
> > > 
> > > Signed-off-by: jackysliu <1972843537@qq.com>  
> > 
> > Drop the spurious blank line.
> > 
> > > ---
> > >  drivers/irqchip/irq-gic-v3.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > > index efc791c43d44..61c1d404b726 100644
> > > --- a/drivers/irqchip/irq-gic-v3.c
> > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > @@ -1821,12 +1821,16 @@ static int partition_domain_translate(struct irq_domain *d,
> > >  		return -EINVAL;
> > >  
> > >  	ret = gic_irq_domain_translate(d, fwspec, &ppi_intid, type);
> > > -	if (WARN_ON_ONCE(ret))
> > > +	if (WARN_ON_ONCE(ret)) {
> > > +		of_node_put(np);
> > >  		return 0;
> > > +	}
> > >  
> > >  	ppi_idx = __gic_get_ppi_index(ppi_intid);
> > >  	ret = partition_translate_id(gic_data.ppi_descs[ppi_idx],
> > >  				     of_fwnode_handle(np));
> > > +	of_node_put(np);
> > > +
> > >  	if (ret < 0)
> > >  		return ret;
> > >    
> > 
> > Frankly, this looks awful, and we have much better ways to solve this
> > whole class of problems. Why can't the (untested) patch below do the
> > right thing, without the ugliness?
> > 
> > 	M.
> > 
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index efc791c43d441..c4839032ce8d0 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -1808,8 +1808,8 @@ static int partition_domain_translate(struct irq_domain *d,
> >  				      unsigned long *hwirq,
> >  				      unsigned int *type)
> >  {
> > +	struct device_node *np __free(device_node) = NULL;
> 
> Linus has expressed fairly strongly that he really doesn't like separation
> of the constructor and destructor.  See guidance in cleanup.h which was
> based on that and bunch of other early discussion around this stuff.
> 
> Here it's easy to solve though. Just move that declaration down to
> give something like:
> 
> 
> >  	unsigned long ppi_intid;
> > -	struct device_node *np;
> >  	unsigned int ppi_idx;
> >  	int ret;
> >  
> 	if (!gic_data.ppi_descs)
> 		return -ENOMEM;
> 
> 	struct device_node *np __free(device_node) =
> 		of_find_node_by_phandle(fwspec->param[3]);
> 
> 
> 	if (WARN_ON(!np))
> 		return -EINVAL;
> 

Yeah, something like that -- it doesn't change a thing in this
context, but I can see how that could degenerate if we added other
cleanup statements.

In any case, this is the sort of thing I want to see going forward.
Nothing like the original patch.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] irqchip/gic-v3: fix resource leak in partition_domain_translate()

Posted by Herve Codina 2 months, 2 weeks ago

On Mon, 21 Jul 2025 15:28:04 +0800
jackysliu <1972843537@qq.com> wrote:

> There is a device node reference leak in partition_domain_translate().
> After the function obtains the device node np via of_find_node_by_phandle,
> it does not call of_node_put(np) to release the node reference
> in both the error path and the normal return path.
> This causes the node reference count to increase each time
> the function is called, causing a resource leak.
> 
> This issue was detected by rule based static tools
> developed by Tencent.
> 
> Fixes: 87228532e7e9 ("irqchip: Switch to of_fwnode_handle()")
> 
> Signed-off-by: jackysliu <1972843537@qq.com>
> ---
>  drivers/irqchip/irq-gic-v3.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index efc791c43d44..61c1d404b726 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1821,12 +1821,16 @@ static int partition_domain_translate(struct irq_domain *d,
>  		return -EINVAL;
>  
>  	ret = gic_irq_domain_translate(d, fwspec, &ppi_intid, type);
> -	if (WARN_ON_ONCE(ret))
> +	if (WARN_ON_ONCE(ret)) {
> +		of_node_put(np);
>  		return 0;
> +	}
>  
>  	ppi_idx = __gic_get_ppi_index(ppi_intid);
>  	ret = partition_translate_id(gic_data.ppi_descs[ppi_idx],
>  				     of_fwnode_handle(np));
> +	of_node_put(np);
> +
>  	if (ret < 0)
>  		return ret;
>  

Reviewed-by: Herve Codina <herve.codina@bootlin.com>

Best regards,
Hervé