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

[PATCH v2] 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.

This changes the semantics for the drivers which implement ->host_stop()
while also completely lacking error handling for the activate host call.
Add activate host error handling for sata_qstor, sata_nv and sata_fsl.

Fixes: 1896b15eddb4 ("ahci_platform: perform platform exit in host_stop() hook")
Cc: stable@vger.kernel.org
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
Changes since v1:
-Add explicit call to ->host_stop() for those drivers that did not have
 proper error handling in probe().

 drivers/ata/libahci.c     | 13 ++++++--
 drivers/ata/libata-core.c | 69 +++++++++++++++++++++++++++++++++++----
 drivers/ata/libata-sff.c  |  6 ++--
 drivers/ata/sata_fsl.c    |  8 +++--
 drivers/ata/sata_nv.c     |  6 +++-
 drivers/ata/sata_qstor.c  |  8 +++--
 include/linux/libata.h    |  1 +
 7 files changed, 96 insertions(+), 15 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/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index 70b210afd291..299b2c886cbe 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1490,8 +1490,12 @@ static int sata_fsl_probe(struct platform_device *ofdev)
 	 * device discovery process, invoking our port_start() handler &
 	 * error_handler() to execute a dummy Softreset EH session
 	 */
-	ata_host_activate(host, irq, sata_fsl_interrupt, SATA_FSL_IRQ_FLAG,
-			  &sata_fsl_sht);
+	retval = ata_host_activate(host, irq, sata_fsl_interrupt,
+				   SATA_FSL_IRQ_FLAG, &sata_fsl_sht);
+	if (retval) {
+		sata_fsl_host_stop(host);
+		return retval;
+	}
 
 	host_priv->intr_coalescing.show = fsl_sata_intr_coalescing_show;
 	host_priv->intr_coalescing.store = fsl_sata_intr_coalescing_store;
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 8624cd4e88cc..29ec216e7803 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -2415,7 +2415,11 @@ static int nv_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	}
 
 	pci_set_master(pdev);
-	return ata_pci_sff_activate_host(host, ipriv->irq_handler, ipriv->sht);
+	rc = ata_pci_sff_activate_host(host, ipriv->irq_handler, ipriv->sht);
+	if (rc && host->ops->host_stop)
+		host->ops->host_stop(host);
+
+	return rc;
 }
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/drivers/ata/sata_qstor.c b/drivers/ata/sata_qstor.c
index 4e7f5b2ff3f6..f38310b84f51 100644
--- a/drivers/ata/sata_qstor.c
+++ b/drivers/ata/sata_qstor.c
@@ -584,8 +584,12 @@ static int qs_ata_init_one(struct pci_dev *pdev,
 	qs_host_init(host, board_idx);
 
 	pci_set_master(pdev);
-	return ata_host_activate(host, pdev->irq, qs_intr, IRQF_SHARED,
-				 &qs_ata_sht);
+	rc = ata_host_activate(host, pdev->irq, qs_intr, IRQF_SHARED,
+			       &qs_ata_sht);
+	if (rc)
+		qs_host_stop(host);
+
+	return rc;
 }
 
 module_pci_driver(qs_ata_pci_driver);
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 v2] ata: libata: Do not leave ata_host_stop() registered when activation fails

[sashiko-bot]

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

New issues:
- [Medium] The generic helper ata_pci_init_one() skips .host_stop cleanup on activation failure because ata_host_undo_start() destroys the devres action.
- [Medium] Missing ata_host_undo_start() on devres_open_group() failure in ata_pci_sff_activate_host() leaves the devres action registered, causing double host_stop() execution.

Pre-existing issues:
- [High] Pre-existing double-unmap and double-free in sata_fsl.c on device_create_file() failure.
--

commit 19ca4d6912533c209fda6772c9bbd7b4176f4fb0
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 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. A driver which releases the host resources in its probe() error
path therefore releases them twice: once itself and once through ->host_stop().

Fixes: 1896b15eddb4 ("ahci_platform: perform platform exit in host_stop() hook")
Cc: stable@vger.kernel.org
Signed-off-by: Niklas Cassel <cassel@kernel.org>

[ ... ]

> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 734e7c88439a7..faa20d817c850 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -2338,10 +2338,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;
>  }

[Severity: Medium]
When ata_host_undo_start() drops the devres action, does this cause
driver-specific teardown logic to be permanently skipped for generic callers?

For example, when pata_amd.c uses ata_pci_bmdma_init_one(), which in turn
calls ata_pci_init_one():

drivers/ata/libata-sff.c:ata_pci_init_one() {
    ...
    rc = ata_pci_sff_activate_host(host, ata_sff_interrupt, sht);
out:
    if (rc == 0)
        devres_remove_group(&pdev->dev, NULL);
    else
        devres_release_group(&pdev->dev, NULL);

    return rc;
}

Because ata_pci_init_one() relies on devres cleanup and does not manually call
host->ops->host_stop(), it appears this teardown logic will now be skipped on
activation failure.

[Severity: Medium]
Can a failure earlier in ata_pci_sff_activate_host() leak the devres action?

If devres_open_group() fails due to memory pressure, it returns directly:

drivers/ata/libata-sff.c:ata_pci_sff_activate_host() {
    ...
    rc = ata_host_start(host);
    if (rc)
        return rc;

    if (!devres_open_group(dev, NULL, GFP_KERNEL))
        return -ENOMEM;
    ...
}

This bypasses the out: label and skips ata_host_undo_start(). Since callers
like nv_init_one() have been updated by this patch to manually call
host_stop() on error:

drivers/ata/sata_nv.c:nv_init_one() {
    ...
    rc = ata_pci_sff_activate_host(host, ipriv->irq_handler, ipriv->sht);
    if (rc && host->ops->host_stop)
        host->ops->host_stop(host);
    ...
}

Will this result in a double execution of the hardware teardown logic during
driver core devres cleanup?

[ ... ]

> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index 70b210afd2911..299b2c886cbed 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -1490,8 +1490,12 @@ static int sata_fsl_probe(struct platform_device *ofdev)
>  	 * device discovery process, invoking our port_start() handler &
>  	 * error_handler() to execute a dummy Softreset EH session
>  	 */
> -	ata_host_activate(host, irq, sata_fsl_interrupt, SATA_FSL_IRQ_FLAG,
> -			  &sata_fsl_sht);
> +	retval = ata_host_activate(host, irq, sata_fsl_interrupt,
> +				   SATA_FSL_IRQ_FLAG, &sata_fsl_sht);
> +	if (retval) {
> +		sata_fsl_host_stop(host);
> +		return retval;
> +	}
>  
>  	host_priv->intr_coalescing.show = fsl_sata_intr_coalescing_show;

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but does sata_fsl_probe() suffer from a double free and double
unmap on later error paths?

If ata_host_activate() succeeds, the devres action for ata_host_stop is
successfully registered. If a later step fails (like device_create_file()),
the code jumps to error_exit_with_cleanup:

drivers/ata/sata_fsl.c:sata_fsl_probe() {
    ...
error_exit_with_cleanup:
    if (host)
        ata_host_detach(host);

    if (hcr_base)
        iounmap(hcr_base);
    kfree(host_priv);

    return retval;
}

When the driver probe fails, devres cleanup will automatically execute
sata_fsl_host_stop(), which attempts to free and unmap the exact same
resources again:

drivers/ata/sata_fsl.c:sata_fsl_host_stop() {
    struct sata_fsl_host_priv *host_priv = host->private_data;

    iounmap(host_priv->hcr_base);
    kfree(host_priv);
}

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