[PATCH] PCI: plda: Synchronize chained IRQs during deinitialization

[PATCH] PCI: plda: Synchronize chained IRQs during deinitialization

[Ali Tariq]

During driver unbind or probe failure teardown,
plda_pcie_irq_domain_deinit() unlinks the chained interrupt
handlers for the main, MSI, and INTx interrupts using
irq_set_chained_handler_and_data(). However, this function
only updates the handler pointers and does not wait for any
in-flight interrupt handlers running on other CPUs to finish.

If a PCIe interrupt fires concurrently with the teardown
process, the handler could continue running on another CPU.
If the hardware clocks are disabled shortly after in
host_deinit(), the executing handler will attempt to read
un-clocked PCIe registers, triggering a fatal system bus
fault (external abort) or kernel panic.

Add synchronize_irq() after clearing each chained handler to
guarantee that any executing handlers have fully completed
before proceeding with interrupt domain removal and hardware
deinitialization.

Fixes: 76c911396807 ("PCI: plda: Add host init/deinit and map bus functions")
Signed-off-by: Ali Tariq <alitariq45892@gmail.com>
---
 drivers/pci/controller/plda/pcie-plda-host.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c
index f9a34f323ad8..f6759e255c75 100644
--- a/drivers/pci/controller/plda/pcie-plda-host.c
+++ b/drivers/pci/controller/plda/pcie-plda-host.c
@@ -560,8 +560,13 @@ EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems);
 static void plda_pcie_irq_domain_deinit(struct plda_pcie_rp *pcie)
 {
 	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
+	synchronize_irq(pcie->irq);
+
 	irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL);
+	synchronize_irq(pcie->msi_irq);
+
 	irq_set_chained_handler_and_data(pcie->intx_irq, NULL, NULL);
+	synchronize_irq(pcie->intx_irq);
 
 	irq_domain_remove(pcie->msi.dev_domain);
 
-- 
2.34.1

Re: [PATCH] PCI: plda: Synchronize chained IRQs during deinitialization

[Bjorn Helgaas]

On Tue, Jul 14, 2026 at 10:43:45PM +0500, Ali Tariq wrote:
> During driver unbind or probe failure teardown,
> plda_pcie_irq_domain_deinit() unlinks the chained interrupt
> handlers for the main, MSI, and INTx interrupts using
> irq_set_chained_handler_and_data(). However, this function
> only updates the handler pointers and does not wait for any
> in-flight interrupt handlers running on other CPUs to finish.
> 
> If a PCIe interrupt fires concurrently with the teardown
> process, the handler could continue running on another CPU.
> If the hardware clocks are disabled shortly after in
> host_deinit(), the executing handler will attempt to read
> un-clocked PCIe registers, triggering a fatal system bus
> fault (external abort) or kernel panic.
> 
> Add synchronize_irq() after clearing each chained handler to
> guarantee that any executing handlers have fully completed
> before proceeding with interrupt domain removal and hardware
> deinitialization.
> 
> Fixes: 76c911396807 ("PCI: plda: Add host init/deinit and map bus functions")
> Signed-off-by: Ali Tariq <alitariq45892@gmail.com>
> ---
>  drivers/pci/controller/plda/pcie-plda-host.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c
> index f9a34f323ad8..f6759e255c75 100644
> --- a/drivers/pci/controller/plda/pcie-plda-host.c
> +++ b/drivers/pci/controller/plda/pcie-plda-host.c
> @@ -560,8 +560,13 @@ EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems);
>  static void plda_pcie_irq_domain_deinit(struct plda_pcie_rp *pcie)
>  {
>  	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
> +	synchronize_irq(pcie->irq);
> +
>  	irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL);
> +	synchronize_irq(pcie->msi_irq);
> +
>  	irq_set_chained_handler_and_data(pcie->intx_irq, NULL, NULL);
> +	synchronize_irq(pcie->intx_irq);

Several other drivers call irq_set_chained_handler_and_data(..., NULL)
without synchronize_irq().  Do they need similar fixes?

>  	irq_domain_remove(pcie->msi.dev_domain);
>  
> -- 
> 2.34.1
> 

Re: [PATCH] PCI: plda: Synchronize chained IRQs during deinitialization

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The added `synchronize_irq()` calls fail to wait for chained interrupt handlers to complete, leaving the target race condition unresolved.

Pre-existing issues:
- [High] IRQ domains are removed without disposing of active hardware IRQ mappings, causing IRQ descriptor memory leaks and triggering a WARN_ON.
- [High] Invalid teardown order between `devm_request_irq()` and `irq_domain_remove()` causes a Use-After-Free during driver removal.
- [High] Error paths bypass deinitialization, leaking IRQ domains and creating a Use-After-Free condition.
--

--- Patch [1]: [PATCH] PCI: plda: Synchronize chained IRQs during deinitialization ---
commit acb2d28887c68b108c1cc032991a465e12053c05
Author: Ali Tariq <alitariq45892@gmail.com>
Subject: PCI: plda: Synchronize chained IRQs during deinitialization

This commit aims to fix a race condition during driver unbind or probe
failure teardown by adding synchronize_irq() calls after clearing chained
handlers. The intention is to guarantee that executing handlers have fully
completed before proceeding with interrupt domain removal.

> diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c
> index f9a34f323ad8c..f6759e255c755 100644
> --- a/drivers/pci/controller/plda/pcie-plda-host.c
> +++ b/drivers/pci/controller/plda/pcie-plda-host.c
> @@ -560,8 +560,13 @@ EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems);
>  static void plda_pcie_irq_domain_deinit(struct plda_pcie_rp *pcie)
>  {
>  	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
> +	synchronize_irq(pcie->irq);

[Severity: High]
Does synchronize_irq() actually wait for chained interrupt handlers to
complete on chips lacking irq_get_irqchip_state?

Chained interrupt handlers execute directly via desc->handle_irq and bypass
flow handlers like handle_irq_event(), meaning they never set the
IRQD_IRQ_INPROGRESS flag.

Because plda_event_irq_chip does not implement the .irq_get_irqchip_state()
callback, synchronize_irq() may return immediately without waiting, leaving
the original race condition unmitigated.

> +
>  	irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL);
> +	synchronize_irq(pcie->msi_irq);
> +
>  	irq_set_chained_handler_and_data(pcie->intx_irq, NULL, NULL);
> +	synchronize_irq(pcie->intx_irq);
>  
>  	irq_domain_remove(pcie->msi.dev_domain);

[Severity: High]
This is a pre-existing issue, but does this code permanently leak irq_desc
memory?

irq_domain_remove() explicitly requires that all mappings be disposed first.
Because plda_pcie_irq_domain_deinit() removes the domains without calling
irq_dispose_mapping() for the mappings created in plda_init_interrupts(),
this leaves dangling mappings that trigger a WARN_ON and leak memory.

[Severity: High]
This is also a pre-existing issue, but does explicitly destroying
event_domain here cause a use-after-free during driver removal?

The generic event IRQs mapped from event_domain are requested using devres
(devm_request_irq()), so their actions are automatically freed by the devres
cleanup loop after the driver's remove function returns.

However, plda_pcie_irq_domain_deinit() destroys event_domain during the
remove function. When devres eventually calls free_irq(), the interrupt core
executes irq_domain_deactivate_irq(), which dereferences irq_data->domain,
leading to a use-after-free.

[Severity: High]
This is a pre-existing issue, but does plda_pcie_host_init() leak IRQ
domains on probe failure?

In plda_pcie_host_init(), if plda_init_interrupts() fails, the code jumps
to err_host:, which intentionally bypasses plda_pcie_irq_domain_deinit():

drivers/pci/controller/plda/pcie-plda-host.c:plda_pcie_host_init() {
    ...
err_probe:
	plda_pcie_irq_domain_deinit(port);
err_host:
	if (port->host_ops && port->host_ops->host_deinit)
		port->host_ops->host_deinit(port);
    ...
}

The IRQ domains created by plda_pcie_init_irq_domains() are permanently
leaked. Additionally, because these domains use the devres-managed port
structure as their host_data, their internal pointers become dangling once
the probe fails and devres frees port, leading to a use-after-free if the
domains are queried later.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714174345.1073909-1-alitariq45892@gmail.com?part=1

Re: [PATCH] PCI: plda: Synchronize chained IRQs during deinitialization

[Ali Tariq]

On 7/14/26 10:58 PM, Bjorn Helgaas wrote:
> On Tue, Jul 14, 2026 at 10:43:45PM +0500, Ali Tariq wrote:
>> During driver unbind or probe failure teardown,
>> plda_pcie_irq_domain_deinit() unlinks the chained interrupt
>> handlers for the main, MSI, and INTx interrupts using
>> irq_set_chained_handler_and_data(). However, this function
>> only updates the handler pointers and does not wait for any
>> in-flight interrupt handlers running on other CPUs to finish.
>>
>> If a PCIe interrupt fires concurrently with the teardown
>> process, the handler could continue running on another CPU.
>> If the hardware clocks are disabled shortly after in
>> host_deinit(), the executing handler will attempt to read
>> un-clocked PCIe registers, triggering a fatal system bus
>> fault (external abort) or kernel panic.
>>
>> Add synchronize_irq() after clearing each chained handler to
>> guarantee that any executing handlers have fully completed
>> before proceeding with interrupt domain removal and hardware
>> deinitialization.
>>
>> Fixes: 76c911396807 ("PCI: plda: Add host init/deinit and map bus functions")
>> Signed-off-by: Ali Tariq <alitariq45892@gmail.com>
>> ---
>>   drivers/pci/controller/plda/pcie-plda-host.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c
>> index f9a34f323ad8..f6759e255c75 100644
>> --- a/drivers/pci/controller/plda/pcie-plda-host.c
>> +++ b/drivers/pci/controller/plda/pcie-plda-host.c
>> @@ -560,8 +560,13 @@ EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems);
>>   static void plda_pcie_irq_domain_deinit(struct plda_pcie_rp *pcie)
>>   {
>>   	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
>> +	synchronize_irq(pcie->irq);
>> +
>>   	irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL);
>> +	synchronize_irq(pcie->msi_irq);
>> +
>>   	irq_set_chained_handler_and_data(pcie->intx_irq, NULL, NULL);
>> +	synchronize_irq(pcie->intx_irq);
> 
> Several other drivers call irq_set_chained_handler_and_data(..., NULL)
> without synchronize_irq().  Do they need similar fixes?
> 
>>   	irq_domain_remove(pcie->msi.dev_domain);
>>   
>> -- 
>> 2.34.1
>>

Hi Bjorn,

Thanks for the question. I looked at the wider pattern across the tree,
and severity does seem to depend heavily on what happens immediately
after the teardown in each specific driver, so I don't think a single
change is safe to apply broadly. I'll follow up separately with more
detail on what I found there.

For this specific driver, though, I think my patch needs to be
withdrawn. Tracing through kernel/irq/manage.c, synchronize_irq()'s
wait relies on the IRQD_IRQ_INPROGRESS flag, which is set and cleared
by handle_irq_event() in the normal interrupt dispatch path. Chained
handlers bypass that path entirely (dispatch goes straight to
desc->handle_irq), so this flag is never touched for them. The
fallback in that case is to query the irqchip directly via
.irq_get_irqchip_state(), but none of this driver's irq_chip
structures implement that callback, so the fallback also does
nothing.

The practical effect is that synchronize_irq() here returns
immediately regardless of whether a chained handler is still
executing, so this patch doesn't actually close the race it was meant
to fix.

I'd like to withdraw this specific patch and rework it, likely with an 
explicit in-driver flag around the chained handlers rather than relying 
on synchronize_irq(), since I don't think .irq_get_irqchip_state() maps
cleanly onto this driver's handler structure either (the status
register gets acked partway through handling, before the rest of the
dispatch work is done).

Regards,
Ali