ata: libata: Do not leave ata_host_stop() registered when activation fails
Subsystems (2)
Niklas Cassel
maintainerPATCH v2 1/1 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, 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.
---
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(-)
drivers/ata/libahci.c +11 -2
@@ -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; }/**
drivers/ata/libata-core.c +63 -6
102 lines not on this page · raw message
drivers/ata/libata-sff.c +4 -2
@@ -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; }
drivers/ata/sata_fsl.c +6 -2
@@ -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;
drivers/ata/sata_nv.c +5 -1
@@ -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
drivers/ata/sata_qstor.c +6 -2
@@ -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);
include/linux/libata.h +1 -0
@@ -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,
Signature · 1 line
2.55.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
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 <[email protected]>
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().
[ ... ]
Quoted 9 lines
drivers/ata/libata-sff.c +0
-1@@ -2338,10 +2338,12 @@ int ata_pci_sff_activate_host(struct ata_host *host, rc =ata_host_register (host, sht); out: -if (rc ==0 )
drivers/ata/libata-sff.c int ata_pci_sff_activate_host(struct ata_host *host, · line 2349
Quoted 10 lines
drivers/ata/libata-sff.c +4
-1+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?
[ ... ]
Quoted 8 lines
drivers/ata/sata_fsl.c +0
-0@@ -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 */
drivers/ata/sata_fsl.c static int sata_fsl_probe(struct platform_device *ofdev) · line 1500
Quoted 10 lines
drivers/ata/sata_fsl.c +6
-2-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);
}Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1