Re: [PATCH 3/3] PCI: dra7xx: Fix device links leak when dra7xx_pcie_enable_phy() fails

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

Pre-existing issues:
- [High] Missing `.remove` callback in platform driver leads to memory and state leaks on module unload.
- [High] Chained IRQ handler and IRQ domain are not cleaned up on probe failure, causing memory leaks and a Use-After-Free.
- [Medium] Improper asynchronous runtime PM cleanup in probe error paths leaks the active power state.
--

--- Patch [3]: [PATCH 3/3] PCI: dra7xx: Fix device links leak when dra7xx_pcie_enable_phy() fails ---
commit 10c75b44424d11b46ba7f7d99b796469558a6366
Author: Felix Gu <[email protected]>

PCI: dra7xx: Fix device links leak when dra7xx_pcie_enable_phy() fails

When dra7xx_pcie_enable_phy() fails, the code returned immediately and left
the already created device links behind. This commit fixes the issue by
jumping to the err_link cleanup path instead.

> diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
> index bf32b14436f97..5bd9d1df80ad3 100644
> --- a/drivers/pci/controller/dwc/pci-dra7xx.c
> +++ b/drivers/pci/controller/dwc/pci-dra7xx.c
> @@ -766,7 +766,7 @@ static int dra7xx_pcie_probe(struct platform_device *pdev)
>  	ret = dra7xx_pcie_enable_phy(dra7xx);
>  	if (ret) {
>  		dev_err(dev, "failed to enable phy\n");
> -		return ret;
> +		goto err_link;
>  	}
>  
>  	platform_set_drvdata(pdev, dra7xx);

[Severity: High]
This is a pre-existing issue, but does the lack of a remove callback in the
platform driver lead to memory and state leaks on module unload?

The platform driver definition lacks a remove callback to pair with manual
state allocations in probe:

drivers/pci/controller/dwc/pci-dra7xx.c:
static struct platform_driver dra7xx_pcie_driver = {
	.probe = dra7xx_pcie_probe,
	...

If the module is unloaded, could all manual allocations and device states
(such as stateless device links, phy power state, and dwc core state) be
leaked since devres ignores manual resource acquisitions?


[Severity: Medium]
This isn't a bug introduced by this patch, but is the asynchronous runtime pm
cleanup in probe error paths handled correctly?

If probe fails after successfully calling pm_runtime_get_sync(), the error
path calls pm_runtime_put() followed immediately by pm_runtime_disable():

drivers/pci/controller/dwc/pci-dra7xx.c:dra7xx_pcie_probe() {
    ...
err_get_sync:
	pm_runtime_put(dev);
	pm_runtime_disable(dev);
    ...
}

pm_runtime_put() schedules an asynchronous suspend request, but the immediate
call to pm_runtime_disable() cancels any pending asynchronous requests. Will
this leave the device and its power domains trapped in the resumed state?
Should pm_runtime_put_sync() be used here instead?


[Severity: High]
This is a pre-existing issue, but are the chained irq handler and irq domain
properly cleaned up on probe failure?

dra7xx_pcie_init_irq_domain() installs the chained handler with pp as data:

drivers/pci/controller/dwc/pci-dra7xx.c:dra7xx_pcie_init_irq_domain() {
    ...
	irq_set_chained_handler_and_data(pp->irq, dra7xx_pcie_msi_irq_handler,
					 pp);
    ...
}

If dw_pcie_host_init() subsequently fails:

drivers/pci/controller/dwc/pci-dra7xx.c:dra7xx_add_pcie_port() {
    ...
	ret = dw_pcie_host_init(pp);
	if (ret) {
		dev_err(dev, "failed to initialize host\n");
		return ret;
	}
    ...
}

The probe error path jumps to err_gpio, bypassing any cleanup for the
chained handler or irq domain. Devres will then free the pci structure
containing pp. If a hardware interrupt fires after this probe failure, could
this result in a use-after-free when the interrupt executes
dra7xx_pcie_msi_irq_handler with the freed pp pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3