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

[Bjorn Helgaas]

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?