[PATCH v2] PCI: mediatek: Fix IRQ domain leak when port fails to enable

[PATCH v2] PCI: mediatek: Fix IRQ domain leak when port fails to enable

[Manivannan Sadhasivam]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

When mtk_pcie_enable_port() fails, mtk_pcie_port_free() removes the port
from pcie->ports and frees the port structure. However, the IRQ domains set
up earlier by mtk_pcie_init_irq_domain() are never freed.

Fix this by refactoring mtk_pcie_irq_teardown() into a per-port helper,
mtk_pcie_irq_teardown_port(), and calling it from mtk_pcie_setup() when
mtk_pcie_enable_port() fails. Since the IRQ teardown must only happen in
the probe error path (during resume, child devices may have active MSI
mappings and the NOIRQ context prohibits sleeping locks),
mtk_pcie_enable_port() is changed to return an error code so callers can
distinguish the two paths and act accordingly.

This issue was reported by Sashiko while reviewing the EcoNet EN7528 SoC
support series.

Cc: stable@vger.kernel.org # 5.10
Cc: Caleb James DeLisle <cjd@cjdns.fr>
Fixes: b099631df160 ("PCI: mediatek: Add controller support for MT2712 and MT7622")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---

Changes in v2:

* Used a different approach by refactoring mtk_pcie_irq_teardown() and calling
  mtk_pcie_irq_teardown_port() from mtk_pcie_setup(), as Sashiko flagged some
  potential issues with v1.

 drivers/pci/controller/pcie-mediatek.c | 63 ++++++++++++++++----------
 1 file changed, 40 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 75722524fe74..907ae4285ecb 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -529,23 +529,27 @@ static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
 	writel(val, port->base + PCIE_INT_MASK);
 }
 
-static void mtk_pcie_irq_teardown(struct mtk_pcie *pcie)
+static void mtk_pcie_irq_teardown_port(struct mtk_pcie_port *port)
 {
-	struct mtk_pcie_port *port, *tmp;
+	irq_set_chained_handler_and_data(port->irq, NULL, NULL);
 
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
-		irq_set_chained_handler_and_data(port->irq, NULL, NULL);
+	if (port->irq_domain)
+		irq_domain_remove(port->irq_domain);
 
-		if (port->irq_domain)
-			irq_domain_remove(port->irq_domain);
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		if (port->inner_domain)
+			irq_domain_remove(port->inner_domain);
+	}
 
-		if (IS_ENABLED(CONFIG_PCI_MSI)) {
-			if (port->inner_domain)
-				irq_domain_remove(port->inner_domain);
-		}
+	irq_dispose_mapping(port->irq);
+}
 
-		irq_dispose_mapping(port->irq);
-	}
+static void mtk_pcie_irq_teardown(struct mtk_pcie *pcie)
+{
+	struct mtk_pcie_port *port, *tmp;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
+		mtk_pcie_irq_teardown_port(port);
 }
 
 static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
@@ -865,7 +869,7 @@ static int mtk_pcie_startup_port_an7583(struct mtk_pcie_port *port)
 	return mtk_pcie_startup_port_v2(port);
 }
 
-static void mtk_pcie_enable_port(struct mtk_pcie_port *port)
+static int mtk_pcie_enable_port(struct mtk_pcie_port *port)
 {
 	struct mtk_pcie *pcie = port->pcie;
 	struct device *dev = pcie->dev;
@@ -874,7 +878,7 @@ static void mtk_pcie_enable_port(struct mtk_pcie_port *port)
 	err = clk_prepare_enable(port->sys_ck);
 	if (err) {
 		dev_err(dev, "failed to enable sys_ck%d clock\n", port->slot);
-		goto err_sys_clk;
+		return err;
 	}
 
 	err = clk_prepare_enable(port->ahb_ck);
@@ -922,11 +926,15 @@ static void mtk_pcie_enable_port(struct mtk_pcie_port *port)
 		goto err_phy_on;
 	}
 
-	if (!pcie->soc->startup(port))
-		return;
+	err = pcie->soc->startup(port);
+	if (err) {
+		dev_info(dev, "Port%d link down\n", port->slot);
+		goto err_soc_startup;
+	}
 
-	dev_info(dev, "Port%d link down\n", port->slot);
+	return 0;
 
+err_soc_startup:
 	phy_power_off(port->phy);
 err_phy_on:
 	phy_exit(port->phy);
@@ -942,8 +950,8 @@ static void mtk_pcie_enable_port(struct mtk_pcie_port *port)
 	clk_disable_unprepare(port->ahb_ck);
 err_ahb_clk:
 	clk_disable_unprepare(port->sys_ck);
-err_sys_clk:
-	mtk_pcie_port_free(port);
+
+	return err;
 }
 
 static int mtk_pcie_parse_port(struct mtk_pcie *pcie,
@@ -1109,8 +1117,13 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie)
 		return err;
 
 	/* enable each port, and then check link status */
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
-		mtk_pcie_enable_port(port);
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		err = mtk_pcie_enable_port(port);
+		if (err) {
+			mtk_pcie_irq_teardown_port(port);
+			mtk_pcie_port_free(port);
+		}
+	}
 
 	/* power down PCIe subsys if slots are all empty (link down) */
 	if (list_empty(&pcie->ports))
@@ -1209,14 +1222,18 @@ static int mtk_pcie_resume_noirq(struct device *dev)
 {
 	struct mtk_pcie *pcie = dev_get_drvdata(dev);
 	struct mtk_pcie_port *port, *tmp;
+	int err;
 
 	if (list_empty(&pcie->ports))
 		return 0;
 
 	clk_prepare_enable(pcie->free_ck);
 
-	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
-		mtk_pcie_enable_port(port);
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		err = mtk_pcie_enable_port(port);
+		if (err)
+			mtk_pcie_port_free(port);
+	}
 
 	/* In case of EP was removed while system suspend. */
 	if (list_empty(&pcie->ports))
-- 
2.48.1

Re: [PATCH v2] PCI: mediatek: Fix IRQ domain leak when port fails to enable

[sashiko-bot]

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

Pre-existing issues:
- [High] Freeing the `port` structure in `mtk_pcie_resume_noirq()` on failure without IRQ teardown leaves dangling pointers in active IRQ domains and chained handlers.
- [High] Probe error paths fail to tear down IRQ domains and chained handlers if an error occurs after a port's IRQs are configured, leading to a Use-After-Free.
- [High] `mtk_pcie_resume_noirq()` illegally calls `clk_prepare_enable()`, which acquires a sleeping mutex, inside a NOIRQ suspend/resume callback.
- [High] A double powerdown occurs during driver removal if all ports failed to link up during probe.
--

commit 9862107995188d91184cf0a5c63549b0feed587c
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI: mediatek: Fix IRQ domain leak when port fails to enable

This commit fixes an IRQ domain leak by refactoring IRQ teardown into a
per-port helper and calling it when mtk_pcie_enable_port() fails.
mtk_pcie_enable_port() is modified to return an error code to distinguish
between probe and resume error paths.

Cc: stable@vger.kernel.org # 5.10
Cc: Caleb James DeLisle <cjd@cjdns.fr>

> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 75722524fe744..907ae4285ecb1 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c

[ ... ]

> @@ -1109,8 +1117,13 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie)
>  		return err;
>  
>  	/* enable each port, and then check link status */
> -	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> -		mtk_pcie_enable_port(port);
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +		err = mtk_pcie_enable_port(port);
> +		if (err) {
> +			mtk_pcie_irq_teardown_port(port);
> +			mtk_pcie_port_free(port);
> +		}
> +	}

[Severity: High]
This is a pre-existing issue, but do probe error paths fail to tear down
IRQ domains and chained handlers if an error occurs after a port's IRQs are
configured?

Earlier in mtk_pcie_setup(), if mtk_pcie_parse_port() fails for a
subsequent port, or if mtk_pcie_subsys_powerup() fails, the function
returns the error directly. 

This causes mtk_pcie_probe() to exit without calling mtk_pcie_irq_teardown().
The devm subsystem automatically frees the allocated port structures, but the
IRQ domains and chained handlers initialized for the previously successful
ports are never torn down. Does this leave them active with dangling pointers
to the freed port memory, leading to a use-after-free on the next interrupt?

>  	/* power down PCIe subsys if slots are all empty (link down) */
>  	if (list_empty(&pcie->ports))

[Severity: High]
This is a pre-existing issue, but does a double powerdown occur during
driver removal if all ports failed to link up during probe?

If all ports fail to link up during mtk_pcie_setup(), the driver calls
mtk_pcie_subsys_powerdown(pcie) but still successfully returns 0 from probe.

Later, if the driver is unbound, mtk_pcie_remove() unconditionally calls
mtk_pcie_put_resources(), which invokes mtk_pcie_subsys_powerdown(pcie)
again. Does this disable the clocks (e.g., free_ck) and call
pm_runtime_put_sync() for a second time, underflowing their respective
reference counts?

[ ... ]

> @@ -1209,14 +1222,18 @@ static int mtk_pcie_resume_noirq(struct device *dev)
>  {
>  	struct mtk_pcie *pcie = dev_get_drvdata(dev);
>  	struct mtk_pcie_port *port, *tmp;
> +	int err;
>  
>  	if (list_empty(&pcie->ports))
>  		return 0;
>  
>  	clk_prepare_enable(pcie->free_ck);

[Severity: High]
This is a pre-existing issue, but does mtk_pcie_resume_noirq() illegally
call clk_prepare_enable() inside a NOIRQ suspend/resume callback?

The NOIRQ power management callbacks execute in an atomic context with local
hardware interrupts disabled. However, mtk_pcie_resume_noirq() calls
clk_prepare_enable() (both directly for pcie->free_ck and indirectly via
mtk_pcie_enable_port() for sys_ck, ahb_ck, etc.).

Since clk_prepare() inherently acquires a global mutex (prepare_lock), which
sleeps, will this trigger a 'sleeping in atomic context' bug?

>  
> -	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> -		mtk_pcie_enable_port(port);
> +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +		err = mtk_pcie_enable_port(port);
> +		if (err)
> +			mtk_pcie_port_free(port);
> +	}

[Severity: High]
This is a pre-existing issue, but does freeing the port structure in
mtk_pcie_resume_noirq() on failure without IRQ teardown leave dangling
pointers?

If a port fails to resume, the driver frees the port structure using
mtk_pcie_port_free(). However, because tearing down IRQs requires sleeping
locks, mtk_pcie_irq_teardown_port() is skipped here.

Does this leave active IRQ domains and chained interrupt handlers pointing
to freed memory, causing a use-after-free crash on subsequent hardware
interrupts or interactions with the IRQ domain?

>  	/* In case of EP was removed while system suspend. */
>  	if (list_empty(&pcie->ports))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521174617.17692-1-mani@kernel.org?part=1

Re: [PATCH v2] PCI: mediatek: Fix IRQ domain leak when port fails to enable

[Manivannan Sadhasivam]

On Thu, 21 May 2026 23:16:17 +0530, Manivannan Sadhasivam wrote:
> When mtk_pcie_enable_port() fails, mtk_pcie_port_free() removes the port
> from pcie->ports and frees the port structure. However, the IRQ domains set
> up earlier by mtk_pcie_init_irq_domain() are never freed.
> 
> Fix this by refactoring mtk_pcie_irq_teardown() into a per-port helper,
> mtk_pcie_irq_teardown_port(), and calling it from mtk_pcie_setup() when
> mtk_pcie_enable_port() fails. Since the IRQ teardown must only happen in
> the probe error path (during resume, child devices may have active MSI
> mappings and the NOIRQ context prohibits sleeping locks),
> mtk_pcie_enable_port() is changed to return an error code so callers can
> distinguish the two paths and act accordingly.
> 
> [...]

Applied, thanks!

[1/1] PCI: mediatek: Fix IRQ domain leak when port fails to enable
      commit: f865a57896bd92d7662eb2818d8f48872e2cbbc7

Best regards,
-- 
Manivannan Sadhasivam <mani@kernel.org>