Re: [PATCH 1/2] PCI: dw-rockchip: Bail out if the INTx irq domain creation fails

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a missing IRQ domain and chained handler cleanup on initialization failure paths, leading to a memory leak and potential Use-After-Free.
- [High] This problem wasn't introduced by this patch, but there is an unprotected lockless iteration over the PCI bus devices list in the threaded IRQ handler.
--

commit e7543706c2d1058d78b54feb3252321efa4a390b
Author: Shawn Lin <shawn.lin@rock-chips.com>

PCI: dw-rockchip: Bail out if the INTx irq domain creation fails

rockchip_pcie_init_irq_domain() can fail, e.g. when the DT node is
missing its "legacy-interrupt-controller" child node. This commit
propagates the error to abort probing instead of continuing with a
NULL irq_domain, which previously caused a panic on the first INTx.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index af26a07c0c9e5..50824c8b1d83e 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -432,8 +432,10 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
>  	pci->dbi_base2 = pci->dbi_base + PCIE_TYPE0_HDR_DBI2_OFFSET;
>  
>  	ret = rockchip_pcie_init_irq_domain(rockchip);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		dev_err(dev, "failed to init irq domain\n");
> +		return ret;
> +	}
>  
>  	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
>  					 rockchip);

[Severity: High]
This is a pre-existing regression, but there is a missing IRQ
domain and chained handler cleanup on initialization failure paths,
which can cause a memory leak and use-after-free.

If rockchip_pcie_host_init() succeeds but a subsequent step in
dw_pcie_host_init() fails (e.g., link training), the probe aborts and
devres frees the rockchip struct. Because the driver lacks a .deinit
callback in rockchip_pcie_host_ops, the IRQ domain allocated via
irq_domain_create_linear() is leaked.

Additionally, the chained handler mapped via
irq_set_chained_handler_and_data() is never removed. Can a subsequent
legacy interrupt or spurious trigger invoke rockchip_pcie_intx_handler()
with the devm-freed rockchip pointer, causing a kernel panic?

Does rockchip_pcie_host_ops need a .deinit callback to properly unwind
these resources on failure paths?


While reviewing this file, another pre-existing regression was noticed in
rockchip_pcie_rc_sys_irq_thread():

drivers/pci/controller/dwc/pcie-dw-rockchip.c:rockchip_pcie_rc_sys_irq_thread() {
[ ... ]
	if (reg & PCIE_LINK_REQ_RST_NOT_INT) {
		dev_dbg(dev, "hot reset or link-down reset\n");
		for_each_pci_bridge(port, pp->bridge->bus) {
			if (pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
				pci_host_handle_link_down(port);
		}
	}
[ ... ]
}

[Severity: High]
This is a pre-existing regression, but there is an unprotected lockless
iteration over the PCI bus devices list in this threaded IRQ handler.

When a hot-reset or link-down event occurs, the handler traverses
pp->bridge->bus->devices using for_each_pci_bridge(). This traversal is
performed locklessly without acquiring the required down_read(&pci_bus_sem).

If a concurrent PCI device removal operation modifies the list (e.g., via
sysfs or hotplug), could this lockless traversal access poisoned pointers
or freed device structures, potentially causing list corruption or a panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1788403477-71491-1-git-send-email-shawn.lin@rock-chips.com?part=1