ata: libata: Do not leave ata_host_stop() registered when activation fails
Subsystems (2)
Niklas Cassel
maintainerPATCH 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.
--- 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(-)
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; }
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 2 potential issue(s) to consider:
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 <[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. 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.
Quoted 14 lines
drivers/ata/libahci.c +1
-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);
drivers/ata/libahci.c static int ahci_host_activate_multi_irqs(struct ata_host *host, · line 2739
Quoted 10 lines
drivers/ata/libahci.c +10
-0+ 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.
Quoted 7 lines
drivers/ata/libahci.c +0
-0}/** drivers/ata/libata-core.c +0
-0
[ ... ]
Quoted 10 lines
drivers/ata/libata-core.c +10
-0+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]; +
drivers/ata/libata-core.c int ata_host_start(struct ata_host *host) · line 6164
Quoted 10 lines
drivers/ata/libata-core.c +10
-0+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?
Quoted 4 lines
drivers/ata/libata-core.c +4
-0+ + host->flags &= ~ATA_HOST_STARTED ; +} +EXPORT_SYMBOL_GPL (ata_host_undo_start);
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1