[PATCH] ata: libata: Do not leave ata_host_stop() registered when activation fails

[PATCH] ata: libata: Do not leave ata_host_stop() registered when activation fails

[Niklas Cassel]

ata_host_start() registers ata_host_stop() as a devres action as soon as
it has succeeded, which hands the release of the host resources over to
devres: ->port_stop() and ->host_stop() are then called by the driver
core when probe() fails.

ata_host_activate(), ahci_host_activate_multi_irqs() and
ata_pci_sff_activate_host() can all fail after ata_host_start() has
succeeded, e.g. if devm_request_irq() or ata_host_register() fails, and
they return the error with the devres action still registered. A driver
which releases the host resources in its probe() error path therefore
releases them twice: once itself and once through ->host_stop().

All ahci-platform drivers are in that situation, e.g. ahci_probe() calls
ahci_platform_disable_resources() while ahci_host_stop() does the same
through devres. This gives refcount underflow warnings from the clk,
regulator and phy cores and, for shared resources, can disable resources
which are still in use by other devices.

Add ata_host_undo_start(), which stops the ports and drops the devres
action without calling ->host_stop(), and call it from the three
activation helpers when they fail. Releasing the host resources on
failure is then always left to the caller, which is what all callers
having a probe() error path already assume.

Fixes: 1896b15eddb4 ("ahci_platform: perform platform exit in host_stop() hook")
Cc: stable@vger.kernel.org
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libahci.c     | 13 ++++++--
 drivers/ata/libata-core.c | 69 +++++++++++++++++++++++++++++++++++----
 drivers/ata/libata-sff.c  |  6 ++--
 include/linux/libata.h    |  1 +
 4 files changed, 79 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 6d72eb017b49..7ac1fcdd6d0d 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2723,11 +2723,20 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host,
 				0, pp->irq_desc, host->ports[i]);
 
 		if (rc)
-			return rc;
+			goto undo_start;
 		ata_port_desc_misc(host->ports[i], irq);
 	}
 
-	return ata_host_register(host, sht);
+	rc = ata_host_register(host, sht);
+	if (rc)
+		goto undo_start;
+
+	return 0;
+
+undo_start:
+	ata_host_undo_start(host);
+
+	return rc;
 }
 
 /**
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index f482c0a6d7e9..4e54cc8fd82a 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6127,6 +6127,46 @@ int ata_host_start(struct ata_host *host)
 }
 EXPORT_SYMBOL_GPL(ata_host_start);
 
+/**
+ *	ata_host_undo_start - undo ata_host_start()
+ *	@host: ATA host to undo_start
+ *
+ *	Stop the ports of @host and drop the devres action registered by
+ *	ata_host_start(), without calling ->host_stop().  Nothing is done if
+ *	@host has not been started.
+ *
+ *	This gives the release of the host resources back to the caller, which
+ *	is what a driver whose probe() error path releases those resources
+ *	itself needs when starting or activating the host fails.
+ *
+ *	LOCKING:
+ *	Inherited from calling layer (may sleep).
+ */
+void ata_host_undo_start(struct ata_host *host)
+{
+	int i;
+
+	if (!(host->flags & ATA_HOST_STARTED))
+		return;
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		if (ap->ops->port_stop)
+			ap->ops->port_stop(ap);
+	}
+
+	/*
+	 * Drop the action added by ata_host_start() without calling it.
+	 * It does not exist if neither ->port_stop() nor ->host_stop() is
+	 * implemented, in which case there is nothing to drop.
+	 */
+	devres_destroy(host->dev, ata_host_stop, NULL, NULL);
+
+	host->flags &= ~ATA_HOST_STARTED;
+}
+EXPORT_SYMBOL_GPL(ata_host_undo_start);
+
 /**
  *	ata_host_init - Initialize a host struct for sas (ipr, libsas)
  *	@host:	host to initialize
@@ -6294,6 +6334,10 @@ EXPORT_SYMBOL_GPL(ata_host_register);
  *	have set polling mode on the port. In this case, @irq_handler
  *	should be NULL.
  *
+ *	On failure, the ports are stopped again and the devres action
+ *	registered by ata_host_start() is dropped without calling
+ *	->host_stop(), so releasing the host resources is left to the caller.
+ *
  *	LOCKING:
  *	Inherited from calling layer (may sleep).
  *
@@ -6314,27 +6358,40 @@ int ata_host_activate(struct ata_host *host, int irq,
 	/* Special case for polling mode */
 	if (!irq) {
 		WARN_ON(irq_handler);
-		return ata_host_register(host, sht);
+		rc = ata_host_register(host, sht);
+		if (rc)
+			goto undo_start;
+
+		return 0;
 	}
 
 	irq_desc = devm_kasprintf(host->dev, GFP_KERNEL, "%s[%s]",
 				  dev_driver_string(host->dev),
 				  dev_name(host->dev));
-	if (!irq_desc)
-		return -ENOMEM;
+	if (!irq_desc) {
+		rc = -ENOMEM;
+		goto undo_start;
+	}
 
 	rc = devm_request_irq(host->dev, irq, irq_handler, irq_flags,
 			      irq_desc, host);
 	if (rc)
-		return rc;
+		goto undo_start;
 
 	for (i = 0; i < host->n_ports; i++)
 		ata_port_desc_misc(host->ports[i], irq);
 
 	rc = ata_host_register(host, sht);
-	/* if failed, just free the IRQ and leave ports alone */
-	if (rc)
+	if (rc) {
+		/* if failed, just free the IRQ and leave ports alone */
 		devm_free_irq(host->dev, irq, host);
+		goto undo_start;
+	}
+
+	return 0;
+
+undo_start:
+	ata_host_undo_start(host);
 
 	return rc;
 }
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 976e4e160494..02a43b5368ef 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -2348,10 +2348,12 @@ int ata_pci_sff_activate_host(struct ata_host *host,
 
 	rc = ata_host_register(host, sht);
 out:
-	if (rc == 0)
+	if (rc == 0) {
 		devres_remove_group(dev, NULL);
-	else
+	} else {
 		devres_release_group(dev, NULL);
+		ata_host_undo_start(host);
+	}
 
 	return rc;
 }
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 313e96173b19..b1debcef4947 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1147,6 +1147,7 @@ extern struct ata_host *ata_host_alloc_pinfo(struct device *dev,
 extern void ata_host_get(struct ata_host *host);
 extern void ata_host_put(struct ata_host *host);
 extern int ata_host_start(struct ata_host *host);
+extern void ata_host_undo_start(struct ata_host *host);
 extern int ata_host_register(struct ata_host *host,
 			     const struct scsi_host_template *sht);
 extern int ata_host_activate(struct ata_host *host, int irq,
-- 
2.55.0

Re: [PATCH] ata: libata: Do not leave ata_host_stop() registered when activation fails

[sashiko-bot]

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

New issues:
- [High] Bypassing `->host_stop()` on activation failure via `ata_host_undo_start()` breaks legacy PCI and OF drivers that rely on devres for hardware and resource cleanup, leading to state/memory leaks and interrupt storms.

Pre-existing issues:
- [High] Missing IRQ cleanup in ahci_host_activate_multi_irqs error path leads to active interrupts during hardware teardown.
--

commit 33ab74bb11edbc6ab549b2a41db1917c6750487f
Author: Niklas Cassel <cassel@kernel.org>

ata: libata: Do not leave ata_host_stop() registered when activation fails

ata_host_start() registers ata_host_stop() as a devres action. This patch
adds ata_host_undo_start() to stop the ports and drop the devres action
without calling ->host_stop() when activation helpers fail, returning
resource release responsibility to the caller.

> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 6d72eb017b498..7ac1fcdd6d0d7 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2723,11 +2723,20 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host,
>  				0, pp->irq_desc, host->ports[i]);
>  
>  		if (rc)
> -			return rc;
> +			goto undo_start;
>  		ata_port_desc_misc(host->ports[i], irq);
>  	}
>  
> -	return ata_host_register(host, sht);
> +	rc = ata_host_register(host, sht);
> +	if (rc)
> +		goto undo_start;
> +
> +	return 0;
> +
> +undo_start:
> +	ata_host_undo_start(host);
> +
> +	return rc;

[Severity: High]
This is a pre-existing issue, but by not freeing the successfully requested
IRQs in this error path before returning, can we cause a kernel panic?

If devm_request_irq() fails for one port, or if ata_host_register() fails,
this jumps to undo_start and returns an error without freeing the IRQs.

The caller (ahci_platform_init_host) and platform driver will then disable
clocks and PHYs via ahci_platform_disable_resources(). If a pending interrupt
fires before the generic devres cleanup eventually frees the IRQs, the
interrupt handler (ahci_multi_irqs_intr_hard) will attempt to access unclocked
AHCI registers.

>  }
>  
>  /**
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 59e87b987d73e..4c0ee207f4c64 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
[ ... ]
> +void ata_host_undo_start(struct ata_host *host)
> +{
> +	int i;
> +
> +	if (!(host->flags & ATA_HOST_STARTED))
> +		return;
> +
> +	for (i = 0; i < host->n_ports; i++) {
> +		struct ata_port *ap = host->ports[i];
> +
> +		if (ap->ops->port_stop)
> +			ap->ops->port_stop(ap);
> +	}
> +
> +	/*
> +	 * Drop the action added by ata_host_start() without calling it.
> +	 * It does not exist if neither ->port_stop() nor ->host_stop() is
> +	 * implemented, in which case there is nothing to drop.
> +	 */
> +	devres_destroy(host->dev, ata_host_stop, NULL, NULL);

[Severity: High]
Does bypassing ->host_stop() here break legacy PCI and OF drivers that rely
entirely on the devres action for hardware and resource cleanup?

For instance, sata_qstor enables hardware interrupts in qs_ata_init_one():

	qs_host_init(host, board_idx);
	pci_set_master(pdev);
	return ata_host_activate(host, pdev->irq, qs_intr, IRQF_SHARED,
				 &qs_ata_sht);

If ata_host_activate() fails, the driver relies on qs_host_stop() to disable
interrupts. By skipping ->host_stop(), wouldn't unhandled hardware interrupts
remain enabled, potentially causing an interrupt storm?

Similarly, sata_nv programs ADMA configuration into PCI registers before
calling ata_pci_sff_activate_host() in nv_init_one(). If activation fails,
nv_adma_host_stop() is bypassed. Does this leak the hardware ADMA
configuration state?

Also, in sata_fsl_probe(), the return value of ata_host_activate() is ignored,
leaving the device bound despite activation failure. With ->host_stop()
bypassed, will this cause resources to leak on unbind?

> +
> +	host->flags &= ~ATA_HOST_STARTED;
> +}
> +EXPORT_SYMBOL_GPL(ata_host_undo_start);

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