[PATCH] irqchip/msi-lib: Fix fwnode refcount in msi_lib_irq_domain_select()

[PATCH] irqchip/msi-lib: Fix fwnode refcount in msi_lib_irq_domain_select()

[Lorenzo Pieralisi]

Commit 8b65db1e93a2 ("irqchip/msi-lib: Add IRQ_DOMAIN_FLAG_FWNODE_PARENT
handling") added logic in msi_lib_irq_domain_select() to match the
domain fwnode against the fwnode parent of the fwspec.fwnode.

The fwnode_get_parent() caller must call fwnode_handle_put() on the
returned pointer value, lest fwnode refcounting for the parent ends
up being out of kilter.

Fix this by relying on the fwnode_handle clean-up handlers and by
incrementing the fwnode refcount regardless of whether we use
parent matching or not (the domain selection code already holds a
reference before calling msi_lib_irq_domain_select() but to make
the exit path more uniform if IRQ_DOMAIN_FLAG_FWNODE_PARENT is not
set fwnode_handle_get() is called again on fwspec.fwnode so that
the clean-up code is the same for the two matching patterns).

Fixes: 8b65db1e93a2 ("irqchip/msi-lib: Add IRQ_DOMAIN_FLAG_FWNODE_PARENT handling")
Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Rob Herring <robh@kernel.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
---
Hi Thomas, Marc,

Noticed while adding GICv5 ACPI support - I tested this patch on arm64 with
DT (GICv5 and v3) and ACPI (GICv3).

msi_lib_irq_domain_select() is used in other arches, I could not
test on those (don't know if they have non-[DT/irqchip/acpi] specific
fwnodes) - from a fwnode interface perspective I think that this patch
does the right thing, it should not add any issue to existing code
to the best of my knowledge but it has to be verified.

Thanks,
Lorenzo

 drivers/irqchip/irq-msi-lib.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-msi-lib.c b/drivers/irqchip/irq-msi-lib.c
index 454c7f16dd4d..908944009c21 100644
--- a/drivers/irqchip/irq-msi-lib.c
+++ b/drivers/irqchip/irq-msi-lib.c
@@ -133,13 +133,13 @@ int msi_lib_irq_domain_select(struct irq_domain *d, struct irq_fwspec *fwspec,
 {
 	const struct msi_parent_ops *ops = d->msi_parent_ops;
 	u32 busmask = BIT(bus_token);
-	struct fwnode_handle *fwh;
 
 	if (!ops)
 		return 0;
 
-	fwh = d->flags & IRQ_DOMAIN_FLAG_FWNODE_PARENT ? fwnode_get_parent(fwspec->fwnode)
-						       : fwspec->fwnode;
+	struct fwnode_handle *fwh __free(fwnode_handle) =
+		d->flags & IRQ_DOMAIN_FLAG_FWNODE_PARENT ? fwnode_get_parent(fwspec->fwnode)
+							 : fwnode_handle_get(fwspec->fwnode);
 	if (fwh != d->fwnode || fwspec->param_count != 0)
 		return 0;
 
-- 
2.48.0

Re: [PATCH] irqchip/msi-lib: Fix fwnode refcount in msi_lib_irq_domain_select()

[Thomas Gleixner]

On Mon, Aug 04 2025 at 16:55, Lorenzo Pieralisi wrote:
>
> msi_lib_irq_domain_select() is used in other arches, I could not
> test on those (don't know if they have non-[DT/irqchip/acpi] specific
> fwnodes) - from a fwnode interface perspective I think that this patch
> does the right thing, it should not add any issue to existing code
> to the best of my knowledge but it has to be verified.

fwnode handles are architecture and firmware agnostic.

[tip: irq/urgent] irqchip/msi-lib: Fix fwnode refcount in msi_lib_irq_domain_select()

[tip-bot2 for Lorenzo Pieralisi]

The following commit has been merged into the irq/urgent branch of tip:

Commit-ID:     02cbf8e0692bd30717b35a3ff5e46460d1d5d471
Gitweb:        https://git.kernel.org/tip/02cbf8e0692bd30717b35a3ff5e46460d1d5d471
Author:        Lorenzo Pieralisi <lpieralisi@kernel.org>
AuthorDate:    Mon, 04 Aug 2025 16:55:53 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 05 Aug 2025 10:31:46 +02:00

irqchip/msi-lib: Fix fwnode refcount in msi_lib_irq_domain_select()

Commit 8b65db1e93a2 ("irqchip/msi-lib: Add IRQ_DOMAIN_FLAG_FWNODE_PARENT
handling") added logic in msi_lib_irq_domain_select() to match the domain
fwnode against the fwnode parent of the fwspec.fwnode.

The fwnode_get_parent() caller must call fwnode_handle_put() on the
returned pointer value, lest fwnode refcounting for the parent ends up
being out of kilter.

Fix this by relying on the fwnode_handle clean-up handlers and by
incrementing the fwnode refcount regardless of whether parent matching is
used or not (the domain selection code already holds a reference before
calling msi_lib_irq_domain_select() but to make the exit path more uniform
if IRQ_DOMAIN_FLAG_FWNODE_PARENT is not set fwnode_handle_get() is called
again on fwspec.fwnode so that the clean-up code is the same for the two
matching patterns).

Fixes: 8b65db1e93a2 ("irqchip/msi-lib: Add IRQ_DOMAIN_FLAG_FWNODE_PARENT handling")
Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/all/20250804145553.795065-1-lpieralisi@kernel.org
---
 drivers/irqchip/irq-msi-lib.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-msi-lib.c b/drivers/irqchip/irq-msi-lib.c
index 454c7f1..9089440 100644
--- a/drivers/irqchip/irq-msi-lib.c
+++ b/drivers/irqchip/irq-msi-lib.c
@@ -133,13 +133,13 @@ int msi_lib_irq_domain_select(struct irq_domain *d, struct irq_fwspec *fwspec,
 {
 	const struct msi_parent_ops *ops = d->msi_parent_ops;
 	u32 busmask = BIT(bus_token);
-	struct fwnode_handle *fwh;
 
 	if (!ops)
 		return 0;
 
-	fwh = d->flags & IRQ_DOMAIN_FLAG_FWNODE_PARENT ? fwnode_get_parent(fwspec->fwnode)
-						       : fwspec->fwnode;
+	struct fwnode_handle *fwh __free(fwnode_handle) =
+		d->flags & IRQ_DOMAIN_FLAG_FWNODE_PARENT ? fwnode_get_parent(fwspec->fwnode)
+							 : fwnode_handle_get(fwspec->fwnode);
 	if (fwh != d->fwnode || fwspec->param_count != 0)
 		return 0;
 

Re: [PATCH] irqchip/msi-lib: Fix fwnode refcount in msi_lib_irq_domain_select()

[Lorenzo Pieralisi]

On Tue, Aug 05, 2025 at 10:31:32AM +0200, Thomas Gleixner wrote:
> On Mon, Aug 04 2025 at 16:55, Lorenzo Pieralisi wrote:
> >
> > msi_lib_irq_domain_select() is used in other arches, I could not
> > test on those (don't know if they have non-[DT/irqchip/acpi] specific
> > fwnodes) - from a fwnode interface perspective I think that this patch
> > does the right thing, it should not add any issue to existing code
> > to the best of my knowledge but it has to be verified.
> 
> fwnode handles are architecture and firmware agnostic.

Yep, though to make sure this does not trigger regressions I started
checking (ie I am adding an additional fwnode_handle_get/put() in there),
some fwnode helpers (eg fwnode_find_reference()) returns an error
pointer rather than NULL on error, it looks like calling
fwnode_handle_put() on that value when OF is in use is not a good idea
(ie of_node_put() checks for NULL and dereference).

There is code out there that implicitly assumes what fwnode types
are used behind the fwnode_* interface or I am missing something.

It is not arch dependent but it looks like it depends on what fwnodes
arches use - that's where my caution stems from, nothing else.

Thanks,
Lorenzo

Re: [PATCH] irqchip/msi-lib: Fix fwnode refcount in msi_lib_irq_domain_select()

[Jonathan Cameron]

On Tue, 5 Aug 2025 11:23:04 +0200
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:

> On Tue, Aug 05, 2025 at 10:31:32AM +0200, Thomas Gleixner wrote:
> > On Mon, Aug 04 2025 at 16:55, Lorenzo Pieralisi wrote:  
> > >
> > > msi_lib_irq_domain_select() is used in other arches, I could not
> > > test on those (don't know if they have non-[DT/irqchip/acpi] specific
> > > fwnodes) - from a fwnode interface perspective I think that this patch
> > > does the right thing, it should not add any issue to existing code
> > > to the best of my knowledge but it has to be verified.  
> > 
> > fwnode handles are architecture and firmware agnostic.  
> 
> Yep, though to make sure this does not trigger regressions I started
> checking (ie I am adding an additional fwnode_handle_get/put() in there),
> some fwnode helpers (eg fwnode_find_reference()) returns an error
> pointer rather than NULL on error, it looks like calling
> fwnode_handle_put() on that value when OF is in use is not a good idea
> (ie of_node_put() checks for NULL and dereference).
> 
> There is code out there that implicitly assumes what fwnode types
> are used behind the fwnode_* interface or I am missing something.
> 
> It is not arch dependent but it looks like it depends on what fwnodes
> arches use - that's where my caution stems from, nothing else.
> 

For the many DEFINE_FREE() uses there is a check of IS_ERR_OR_NULL()

E.g. Here it would be 
DEFINE_FREE(fwnode_handle, struct fwnode_handle *, if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T));

IIRC this one was an early use of DEFINE_FREE() and later discussions
argued for always adding that check purely to allow the compiler
to potentially optimize away the call.  Sounds like it would be
more generally helpful here and I can't immediately spot any negatives.

Jonathan


> Thanks,
> Lorenzo
> 

Re: [PATCH] irqchip/msi-lib: Fix fwnode refcount in msi_lib_irq_domain_select()

[Lorenzo Pieralisi]

On Wed, Aug 06, 2025 at 03:08:18PM +0100, Jonathan Cameron wrote:
> On Tue, 5 Aug 2025 11:23:04 +0200
> Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> > On Tue, Aug 05, 2025 at 10:31:32AM +0200, Thomas Gleixner wrote:
> > > On Mon, Aug 04 2025 at 16:55, Lorenzo Pieralisi wrote:  
> > > >
> > > > msi_lib_irq_domain_select() is used in other arches, I could not
> > > > test on those (don't know if they have non-[DT/irqchip/acpi] specific
> > > > fwnodes) - from a fwnode interface perspective I think that this patch
> > > > does the right thing, it should not add any issue to existing code
> > > > to the best of my knowledge but it has to be verified.  
> > > 
> > > fwnode handles are architecture and firmware agnostic.  
> > 
> > Yep, though to make sure this does not trigger regressions I started
> > checking (ie I am adding an additional fwnode_handle_get/put() in there),
> > some fwnode helpers (eg fwnode_find_reference()) returns an error
> > pointer rather than NULL on error, it looks like calling
> > fwnode_handle_put() on that value when OF is in use is not a good idea
> > (ie of_node_put() checks for NULL and dereference).
> > 
> > There is code out there that implicitly assumes what fwnode types
> > are used behind the fwnode_* interface or I am missing something.
> > 
> > It is not arch dependent but it looks like it depends on what fwnodes
> > arches use - that's where my caution stems from, nothing else.
> > 
> 
> For the many DEFINE_FREE() uses there is a check of IS_ERR_OR_NULL()
> 
> E.g. Here it would be 
> DEFINE_FREE(fwnode_handle, struct fwnode_handle *, if (!IS_ERR_OR_NULL(_T)) fwnode_handle_put(_T));
> 
> IIRC this one was an early use of DEFINE_FREE() and later discussions
> argued for always adding that check purely to allow the compiler
> to potentially optimize away the call.  Sounds like it would be
> more generally helpful here and I can't immediately spot any negatives.

Neither can I - at present I don't think that's a real problem
(ie we would have noticed) but we can add the additional check
you suggested above.

Thanks,
Lorenzo

> 
> Jonathan
> 
> 
> > Thanks,
> > Lorenzo
> > 
>