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

[Dinh Nguyen <dinguyen@kernel.org>]

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@vger.kernel.org
>> Reviewed-by: Subhransu S. Prusty <subhransu.sekhar.prusty@altera.com>
>> Signed-off-by: Mahesh Vaidya <mahesh.vaidya@altera.com>
>> ---
>>   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