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

[sashiko-bot@kernel.org]

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