Re: [PATCH v2 2/2] PCI: altera: Fix resource leaks on probe failure

[Dinh Nguyen]

On 9/3/26 01:56, Bjorn Helgaas wrote:
> On Thu, Apr 30, 2026 at 01:43:30PM -0700, Mahesh Vaidya wrote:
> > The chained IRQ handler is installed during probe but is only removed
> > from the remove path. If pci_host_probe() fails, the handler and INTx IRQ
> > domain remain installed even though the devm-managed host bridge storage
> > containing struct altera_pcie will be released, leaving the handler with
> > a stale data pointer.
> >
> > Interrupts are also enabled before pci_host_probe() is called. If probe
> > fails after that point, the controller interrupt source should be disabled
> > before the chained handler and INTx domain are removed.
> >
> > Install the chained handler only after the INTx domain has been created.
> > Disable controller interrupts during IRQ teardown, and tear the IRQ setup
> > down if pci_host_probe() fails.
> >
> > Fixes: c63aed7334c2 ("PCI: altera: Use pci_host_probe() to register host")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Reviewed-by: Subhransu S. Prusty <subhransu.sekhar.prusty@xxxxxxxxxx>
> > Signed-off-by: Mahesh Vaidya <mahesh.vaidya@xxxxxxxxxx>
> > ---
> >   drivers/pci/controller/pcie-altera.c | 35 ++++++++++++++++++++++++++--
> >   1 file changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
> > index 3d3519b8d88f..902ae2d81763 100644
> > --- a/drivers/pci/controller/pcie-altera.c
> > +++ b/drivers/pci/controller/pcie-altera.c
> > @@ -864,8 +864,23 @@ static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
> >   	return 0;
> >   }
> >   
> > +static void altera_pcie_disable_irq(struct altera_pcie *pcie)
> > +{
> > +	if (pcie->pcie_data->version == ALTERA_PCIE_V1 ||
> > +	    pcie->pcie_data->version == ALTERA_PCIE_V2) {
> > +		/* Disable all P2A interrupts */
> > +		cra_writel(pcie, 0, P2A_INT_ENABLE);
> > +	} else if (pcie->pcie_data->version == ALTERA_PCIE_V3) {
> > +		/* Disable port-level interrupts (CFG_AER, etc.) */
> > +		writel(0, pcie->hip_base +
> > +			  pcie->pcie_data->port_conf_offset +
> > +			  pcie->pcie_data->port_irq_enable_offset);
> > +	}
> > +}
> > +
> >   static void altera_pcie_irq_teardown(struct altera_pcie *pcie)
> >   {
> > +	altera_pcie_disable_irq(pcie);
> >   	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
> >   	irq_domain_remove(pcie->irq_domain);
> >   }
>
> This patch appeared in v7.2 as 7a94138caeb2 ("PCI: altera: Fix
> resource leaks on probe failure").
>
> While considering this for backporting, sashiko came up with the
> questions below.  Can you take a look and see if they make sense?
>
>    This is a pre-existing issue, but does altera_pcie_irq_teardown()
>    lack synchronization with in-flight interrupts?
>
>    If a device interrupt fires just before altera_pcie_disable_irq()
>    masks it, could the chained ISR (altera_pcie_isr) execute
>    concurrently with altera_pcie_irq_teardown() on another CPU?
>
>    Because irq_set_chained_handler_and_data() unregisters the handler
>    but does not wait for currently executing ISRs on other CPUs, could
>    irq_domain_remove() free the domain while the ISR is still using it
>    to call generic_handle_domain_irq(), leading to a use-after-free and
>    a kernel panic?
>
>    Would adding a call to synchronize_irq() before removing the IRQ
>    domain prevent this race during driver removal or when
>    pci_host_probe() fails?

In this case, because the driver uses chained interrupts, 
synchronize_irq() is not effective.

Chained handlers execute directly via desc->handle_irq() and bypass the
generic handle_irq_event() function, which means the IRQD_IRQ_INPROGRESS
flag is never set for them. synchronize_irq() relies on this flag, so it 
will return immediately, effectively being a no-op.

A correct fix for this potential use-after-free would be to convert the 
driver to use a normal requested IRQ instead of the chained handler. 
I'll work on a patch for that.


Thanks,
Dinh