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

[Niklas Cassel <cassel@kernel.org>]

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