[PATCH v3] ata: libahci: delete the SW activity blink timer on port stop

[PATCH v3] ata: libahci: delete the SW activity blink timer on port stop

[Hyunjung Ko]

The software activity LED support arms a per-EM-port timer (emp->timer,
ahci_sw_activity_blink) from ahci_sw_activity() on I/O, and the timer
callback re-arms itself while the LED is blinking. The timer lives in
struct ahci_em_priv inside the port's ahci_port_priv, which is allocated
with devm_kzalloc() and freed when the port is torn down.

ahci_port_stop() never deletes this timer - nothing in libahci does - so
when the SW activity LED is in use and the AHCI port is stopped (driver
unbind / module unload) while the blink timer is still pending,
ahci_sw_activity_blink() runs after ahci_port_priv has been freed and
dereferences the freed ahci_em_priv (use-after-free).

Shut the timers down in ahci_port_stop() before the port private data is
released, only when ATA_FLAG_SW_ACTIVITY is set and skipping em_priv slots
that ahci_init_sw_activity() never set up. Because the callback re-arms
itself, use timer_shutdown_sync() so a concurrent re-arm cannot resurrect
it.

Not tested on real hardware (needs an AHCI controller with SGPIO
enclosure management and the software activity LED enabled).

Fixes: 365cfa1ed5a3 ("ahci: Move generic code into libahci")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
Changes in v3:
- Shorten the code comment (Niklas Cassel).

Changes in v2:
- Gate the shutdown loop on ATA_FLAG_SW_ACTIVITY and skip em_priv slots
  whose timer was never set up (emp->timer.function unset), so
  timer_shutdown_sync() is not called on an uninitialised timer
  (Sashiko AI review).

v2: https://lore.kernel.org/linux-ide/20261109060709.1231946-1-hj351016@gmail.com/

 drivers/ata/libahci.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 001a6f5..873645c 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2605,9 +2605,24 @@ static void ahci_port_stop(struct ata_port *ap)
 {
 	const char *emsg = NULL;
 	struct ahci_host_priv *hpriv = ap->host->private_data;
+	struct ahci_port_priv *pp = ap->private_data;
 	void __iomem *host_mmio = hpriv->mmio;
+	struct ata_link *link;
+	struct ahci_em_priv *emp;
 	int rc;

+	/*
+	 * Stop the SW activity blink timers before devres frees pp, skipping
+	 * the slots that ahci_init_sw_activity() never set up.
+	 */
+	if (ap->flags & ATA_FLAG_SW_ACTIVITY) {
+		ata_for_each_link(link, ap, EDGE) {
+			emp = &pp->em_priv[link->pmp];
+			if (emp->timer.function)
+				timer_shutdown_sync(&emp->timer);
+		}
+	}
+
 	/* de-initialize port */
 	rc = ahci_deinit_port(ap, &emsg);
 	if (rc)
--
2.43.0

Re: [PATCH v3] ata: libahci: delete the SW activity blink timer on port stop

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ata: libahci: ahci_port_stop() skips timers on detached PMP
  slots
- [Low] ata: libahci: Fixes trailer cites code movement commit
  365cfa1ed5a3
--

commit b31d9e35a39c06af6f90ba34e4a622ba2889d52f
Author: Hyunjung Ko <hj351016@gmail.com>

> ata: libahci: delete the SW activity blink timer on port stop
>
> The software activity LED support arms a per-EM-port timer (emp->timer,
> ahci_sw_activity_blink) from ahci_sw_activity() on I/O, and the timer
> callback re-arms itself while the LED is blinking. The timer lives in
> struct ahci_em_priv inside the port's ahci_port_priv, which is allocated
> with devm_kzalloc() and freed when the port is torn down.
>
> ahci_port_stop() never deletes this timer - nothing in libahci does - so
> when the SW activity LED is in use and the AHCI port is stopped (driver
> unbind / module unload) while the blink timer is still pending,
> ahci_sw_activity_blink() runs after ahci_port_priv has been freed and
> dereferences the freed ahci_em_priv (use-after-free).
>
> Shut the timers down in ahci_port_stop() before the port private data is
> released, only when ATA_FLAG_SW_ACTIVITY is set and skipping em_priv slots
> that ahci_init_sw_activity() never set up. Because the callback re-arms
> itself, use timer_shutdown_sync() so a concurrent re-arm cannot resurrect
> it.
>
> Not tested on real hardware (needs an AHCI controller with SGPIO
> enclosure management and the software activity LED enabled).
>
> Fixes: 365cfa1ed5a3 ("ahci: Move generic code into libahci")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>

[Severity: Low]
365cfa1ed5a3 ("ahci: Move generic code into libahci") only moved the existing
AHCI code from drivers/ata/ahci.c into drivers/ata/libahci.c.  Should the
Fixes tag cite the commit that introduced emp->timer without stopping it in
ahci_port_stop()?

  Fixes: 18f7ba4c2f4b ("libata/ahci: enclosure management support")

> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 5ce34683d4683..cf1b50f6dc52c 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2605,9 +2605,24 @@ static void ahci_port_stop(struct ata_port *ap)
>  {
>  	const char *emsg = NULL;
>  	struct ahci_host_priv *hpriv = ap->host->private_data;
> +	struct ahci_port_priv *pp = ap->private_data;
>  	void __iomem *host_mmio = hpriv->mmio;
> +	struct ata_link *link;
> +	struct ahci_em_priv *emp;
>  	int rc;
>  
> +	/*
> +	 * Stop the SW activity blink timers before devres frees pp, skipping
> +	 * the slots that ahci_init_sw_activity() never set up.
> +	 */
> +	if (ap->flags & ATA_FLAG_SW_ACTIVITY) {
> +		ata_for_each_link(link, ap, EDGE) {
> +			emp = &pp->em_priv[link->pmp];
> +			if (emp->timer.function)
> +				timer_shutdown_sync(&emp->timer);
> +		}
> +	}

[Severity: Medium]
Can ata_for_each_link(link, ap, EDGE) miss armed timers in
pp->em_priv[1..EM_MAX_SLOTS - 1] if a Port Multiplier (PMP) was detached
before ahci_port_stop() runs?

During EH recovery in sata_pmp_eh_recover(), ata_eh_recover() can issue
internal commands on PMP fan-out links and arm pp->em_priv[k].timer via
ahci_qc_issue() -> ahci_sw_activity().  If recovery then exhausts pmp_tries,
sata_pmp_eh_recover() calls sata_pmp_detach(), which sets ap->nr_pmp_links = 0
without stopping those timers.  When ahci_port_stop() runs,
ata_for_each_link(link, ap, EDGE) sees !sata_pmp_attached(ap) and only visits
&ap->link (pmp 0), leaving any pending fan-out link timer to fire after pp and
ap->pmp_link are freed (use-after-free).

Since the loop already checks emp->timer.function, iterating over all
EM_MAX_SLOTS entries in pp->em_priv[] directly would shut down all initialized
timers regardless of whether a PMP was detached.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110090303.2415681-1-hj351016@gmail.com?part=1

Re: [PATCH v3] ata: libahci: delete the SW activity blink timer on port stop

[Niklas Cassel]

On Tue, Nov 10, 2026 at 06:03:03PM +0900, Hyunjung Ko wrote:
> The software activity LED support arms a per-EM-port timer (emp->timer,
> ahci_sw_activity_blink) from ahci_sw_activity() on I/O, and the timer
> callback re-arms itself while the LED is blinking. The timer lives in
> struct ahci_em_priv inside the port's ahci_port_priv, which is allocated
> with devm_kzalloc() and freed when the port is torn down.
> 
> ahci_port_stop() never deletes this timer - nothing in libahci does - so
> when the SW activity LED is in use and the AHCI port is stopped (driver
> unbind / module unload) while the blink timer is still pending,
> ahci_sw_activity_blink() runs after ahci_port_priv has been freed and
> dereferences the freed ahci_em_priv (use-after-free).
> 
> Shut the timers down in ahci_port_stop() before the port private data is
> released, only when ATA_FLAG_SW_ACTIVITY is set and skipping em_priv slots
> that ahci_init_sw_activity() never set up. Because the callback re-arms
> itself, use timer_shutdown_sync() so a concurrent re-arm cannot resurrect
> it.
> 
> Not tested on real hardware (needs an AHCI controller with SGPIO
> enclosure management and the software activity LED enabled).
> 
> Fixes: 365cfa1ed5a3 ("ahci: Move generic code into libahci")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
> ---
> Changes in v3:
> - Shorten the code comment (Niklas Cassel).

It seems like you modified v1 instead of v2.

v3's index line is 001a6f5..873645c, exactly v1's. Applying v3
to blob 001a6f5 actually produces ace66cd, so the patch was most
likely hand-edited from v1 rather than regenerated.

Please use git format-patch.


Kind regards,
Niklas