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

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

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

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. emp->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, so when the SW activity LED is
in use and the port is stopped (driver unbind / module unload) while the
blink timer is 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 timer down in ahci_port_stop() before the port private data is
released; timer_shutdown_sync() also stops the self re-arm. The timers are
only set up (ahci_init_sw_activity()) when ATA_FLAG_SW_ACTIVITY is set, so
gate on that flag and skip slots whose timer was never initialised --
timer_shutdown_sync() on an uninitialised timer trips the debug-object and
lockdep checks. Sweep all EM_MAX_SLOTS em_priv slots rather than the
current links, so a detached Port Multiplier's fan-out timer is not missed.

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

Fixes: 18f7ba4c2f4b ("libata/ahci: enclosure management support")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 001a6f5e4172..3f6844acb413 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2605,9 +2605,32 @@ 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;
 	int rc;
 
+	/*
+	 * The SW activity LED blink timer (emp->timer) is armed from the I/O
+	 * path and re-arms itself, and the ahci_em_priv it lives in is part of
+	 * the devm-allocated ahci_port_priv; shut it down before that is freed
+	 * or it can fire against freed memory.  Only slots whose timer was set
+	 * up by ahci_init_sw_activity() may be touched -- timer_shutdown_sync()
+	 * on a never-initialised timer trips the debug-object/lockdep checks --
+	 * and all EM_MAX_SLOTS slots are swept, not just the current links, so a
+	 * detached Port Multiplier's fan-out timer is not missed.
+	 * timer_shutdown_sync() also stops the self re-arm.
+	 */
+	if (ap->flags & ATA_FLAG_SW_ACTIVITY) {
+		int i;
+
+		for (i = 0; i < EM_MAX_SLOTS; i++) {
+			struct ahci_em_priv *emp = &pp->em_priv[i];
+
+			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 v2] ata: libahci: delete the SW activity blink timer on port stop

[Niklas Cassel]

Hello Hyunjung,

On Mon, Nov 09, 2026 at 03:07:09PM +0900, hyunjungg wrote:
> From: Hyunjung Ko <hj351016@gmail.com>
> 
> 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. emp->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, so when the SW activity LED is
> in use and the port is stopped (driver unbind / module unload) while the
> blink timer is 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 timer down in ahci_port_stop() before the port private data is
> released; timer_shutdown_sync() also stops the self re-arm. The timers are
> only set up (ahci_init_sw_activity()) when ATA_FLAG_SW_ACTIVITY is set, so
> gate on that flag and skip slots whose timer was never initialised --
> timer_shutdown_sync() on an uninitialised timer trips the debug-object and
> lockdep checks. Sweep all EM_MAX_SLOTS em_priv slots rather than the
> current links, so a detached Port Multiplier's fan-out timer is not missed.
> 
> Not tested on real hardware (needs an AHCI controller with SGPIO
> enclosure management and the software activity LED enabled).
> 
> Fixes: 18f7ba4c2f4b ("libata/ahci: enclosure management support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
> ---

Where is the changelog?


Kind regards,
Niklas

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

[khj]

Sorry, I missed the changelog on v2 — resent it just now with the
"Changes in v2" list below the ---.
Thanks! :)

2026년 11월 9일 (월) 오후 6:20, Niklas Cassel <cassel@kernel.org>님이 작성:
>
> Hello Hyunjung,
>
> On Mon, Nov 09, 2026 at 03:07:09PM +0900, hyunjungg wrote:
> > From: Hyunjung Ko <hj351016@gmail.com>
> >
> > 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. emp->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, so when the SW activity LED is
> > in use and the port is stopped (driver unbind / module unload) while the
> > blink timer is 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 timer down in ahci_port_stop() before the port private data is
> > released; timer_shutdown_sync() also stops the self re-arm. The timers are
> > only set up (ahci_init_sw_activity()) when ATA_FLAG_SW_ACTIVITY is set, so
> > gate on that flag and skip slots whose timer was never initialised --
> > timer_shutdown_sync() on an uninitialised timer trips the debug-object and
> > lockdep checks. Sweep all EM_MAX_SLOTS em_priv slots rather than the
> > current links, so a detached Port Multiplier's fan-out timer is not missed.
> >
> > Not tested on real hardware (needs an AHCI controller with SGPIO
> > enclosure management and the software activity LED enabled).
> >
> > Fixes: 18f7ba4c2f4b ("libata/ahci: enclosure management support")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
> > ---
>
> Where is the changelog?
>
>
> Kind regards,
> Niklas

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

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

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. emp->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, so when the SW activity LED is
in use and the port is stopped (driver unbind / module unload) while the
blink timer is 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 timer down in ahci_port_stop() before the port private data is
released; timer_shutdown_sync() also stops the self re-arm. The timers are
only set up (ahci_init_sw_activity()) when ATA_FLAG_SW_ACTIVITY is set, so
gate on that flag and skip slots whose timer was never initialised --
timer_shutdown_sync() on an uninitialised timer trips the debug-object and
lockdep checks. Sweep all EM_MAX_SLOTS em_priv slots rather than the
current links, so a detached Port Multiplier's fan-out timer is not missed.

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

Fixes: 18f7ba4c2f4b ("libata/ahci: enclosure management support")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
Changes in v2 (thanks to the Sashiko AI review):
 - gate the shutdown loop on ATA_FLAG_SW_ACTIVITY and skip slots whose
   timer was never set up (emp->timer.function == NULL): calling
   timer_shutdown_sync() on an uninitialised timer trips the debug-object
   and lockdep checks
 - sweep all EM_MAX_SLOTS em_priv slots instead of the current links, so a
   detached Port Multiplier's fan-out timer is not missed
 - correct the Fixes: tag to 18f7ba4c2f4b (which introduced emp->timer and
   ahci_sw_activity_blink) instead of the later code-move commit
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 001a6f5e4172..3f6844acb413 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2605,9 +2605,32 @@ 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;
 	int rc;
 
+	/*
+	 * The SW activity LED blink timer (emp->timer) is armed from the I/O
+	 * path and re-arms itself, and the ahci_em_priv it lives in is part of
+	 * the devm-allocated ahci_port_priv; shut it down before that is freed
+	 * or it can fire against freed memory.  Only slots whose timer was set
+	 * up by ahci_init_sw_activity() may be touched -- timer_shutdown_sync()
+	 * on a never-initialised timer trips the debug-object/lockdep checks --
+	 * and all EM_MAX_SLOTS slots are swept, not just the current links, so a
+	 * detached Port Multiplier's fan-out timer is not missed.
+	 * timer_shutdown_sync() also stops the self re-arm.
+	 */
+	if (ap->flags & ATA_FLAG_SW_ACTIVITY) {
+		int i;
+
+		for (i = 0; i < EM_MAX_SLOTS; i++) {
+			struct ahci_em_priv *emp = &pp->em_priv[i];
+
+			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 v2] ata: libahci: delete the SW activity blink timer on port stop

[Niklas Cassel]

On Mon, Nov 09, 2026 at 06:24:15PM +0900, hyunjungg wrote:
> From: Hyunjung Ko <hj351016@gmail.com>
> 
> 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. emp->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, so when the SW activity LED is
> in use and the port is stopped (driver unbind / module unload) while the
> blink timer is 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 timer down in ahci_port_stop() before the port private data is
> released; timer_shutdown_sync() also stops the self re-arm. The timers are
> only set up (ahci_init_sw_activity()) when ATA_FLAG_SW_ACTIVITY is set, so
> gate on that flag and skip slots whose timer was never initialised --
> timer_shutdown_sync() on an uninitialised timer trips the debug-object and
> lockdep checks. Sweep all EM_MAX_SLOTS em_priv slots rather than the
> current links, so a detached Port Multiplier's fan-out timer is not missed.
> 
> Not tested on real hardware (needs an AHCI controller with SGPIO
> enclosure management and the software activity LED enabled).
> 
> Fixes: 18f7ba4c2f4b ("libata/ahci: enclosure management support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
> ---
> Changes in v2 (thanks to the Sashiko AI review):
>  - gate the shutdown loop on ATA_FLAG_SW_ACTIVITY and skip slots whose
>    timer was never set up (emp->timer.function == NULL): calling
>    timer_shutdown_sync() on an uninitialised timer trips the debug-object
>    and lockdep checks
>  - sweep all EM_MAX_SLOTS em_priv slots instead of the current links, so a
>    detached Port Multiplier's fan-out timer is not missed
>  - correct the Fixes: tag to 18f7ba4c2f4b (which introduced emp->timer and
>    ahci_sw_activity_blink) instead of the later code-move commit
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 001a6f5e4172..3f6844acb413 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2605,9 +2605,32 @@ 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;
>  	int rc;
>  
> +	/*
> +	 * The SW activity LED blink timer (emp->timer) is armed from the I/O
> +	 * path and re-arms itself, and the ahci_em_priv it lives in is part of
> +	 * the devm-allocated ahci_port_priv; shut it down before that is freed
> +	 * or it can fire against freed memory.  Only slots whose timer was set
> +	 * up by ahci_init_sw_activity() may be touched -- timer_shutdown_sync()
> +	 * on a never-initialised timer trips the debug-object/lockdep checks --
> +	 * and all EM_MAX_SLOTS slots are swept, not just the current links, so a
> +	 * detached Port Multiplier's fan-out timer is not missed.
> +	 * timer_shutdown_sync() also stops the self re-arm.
> +	 */
> +	if (ap->flags & ATA_FLAG_SW_ACTIVITY) {
> +		int i;
> +
> +		for (i = 0; i < EM_MAX_SLOTS; i++) {
> +			struct ahci_em_priv *emp = &pp->em_priv[i];
> +
> +			if (emp->timer.function)
> +				timer_shutdown_sync(&emp->timer);
> +		}
> +	}
> +
>  	/* de-initialize port */
>  	rc = ahci_deinit_port(ap, &emsg);
>  	if (rc)
> -- 
> 2.43.0
> 

Patch looks good to me, but the code comment is a bit overly verbose.


Perhaps something like:

        /*
         * Stop the SW activity blink timers before devres frees pp, skipping
         * the slots that ahci_init_sw_activity() never set up.
         */



Kind regards,
Niklas