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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] ata: libahci: ahci_port_stop() shuts down uninitialized
  emp->timer
- [Medium] ata: libahci: ahci_port_stop() misses detached PMP emp->timer
  slots
- [Low] ata: libahci: inaccurate Fixes trailer in ahci_port_stop()
  commit
--

commit 8312e7f06863279e24cc7d045b296589047833e6
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 timer down in ahci_port_stop() for every link's em_priv before
> the port private data is released. Because the callback re-arms itself,
> use timer_shutdown_sync() so a concurrent re-arm cannot resurrect it.
> em_priv entries without an armed timer are zero-initialised, for which
> timer_shutdown_sync() is a no-op.
>
> 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]
Commit 365cfa1ed5a3 ("ahci: Move generic code into libahci") only moved the
existing code into libahci.c.  Should the Fixes: tag point to the commit
that introduced emp->timer and ahci_sw_activity_blink() instead:

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

> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 001a6f5e41723..873645c982496 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2605,9 +2605,23 @@ 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;
>  
> +	/*
> +	 * The SW activity LED blink timer (emp->timer) is armed from the I/O
> +	 * path and re-arms itself; shut it down before the devm-allocated
> +	 * ahci_port_priv it lives in is freed, or it can fire against freed
> +	 * memory.  timer_shutdown_sync() also stops the self re-arm.
> +	 */
> +	ata_for_each_link(link, ap, EDGE) {
> +		emp = &pp->em_priv[link->pmp];
> +		timer_shutdown_sync(&emp->timer);
> +	}

[Severity: Medium]
Does this call timer_shutdown_sync() on an uninitialized emp->timer when
ATA_FLAG_SW_ACTIVITY is not set?

ahci_start_port() only calls ahci_init_sw_activity(), which runs
timer_setup(&emp->timer, ...), when ap->flags & ATA_FLAG_SW_ACTIVITY is
set.  Even when ATA_FLAG_SW_ACTIVITY is set, ahci_port_start() ->
ahci_start_port() runs before a Port Multiplier is attached, and
ahci_pmp_attach() never calls ahci_init_sw_activity(), leaving
pp->em_priv[1 .. ap->nr_pmp_links - 1].timer zero-initialized unless
ahci_port_resume() runs later.

timer_shutdown_sync() is not a no-op on zero-initialized devm_kzalloc()
memory: __timer_delete_sync() calls lock_map_acquire(&timer->lockdep_map),
which triggers "INFO: trying to register non-static key" and disables
lockdep under CONFIG_LOCKDEP=y, and __try_to_del_timer_sync() triggers a
debug_assert_init() warning under CONFIG_DEBUG_OBJECTS_TIMERS=y.

[Severity: Medium]
Can ata_for_each_link(link, ap, EDGE) also miss pending emp->timer instances
after a Port Multiplier detach?

If a Port Multiplier had pp->em_priv[pmp].timer initialized and armed on a
fan-out link (pmp >= 1) and was then detached before ahci_port_stop(),
sata_pmp_detach() sets ap->nr_pmp_links = 0 without stopping those timers.
ata_for_each_link(link, ap, EDGE) then visits only &ap->link (pmp == 0) and
skips pp->em_priv[1 .. EM_MAX_SLOTS - 1], so a still-pending blink timer can
fire after pp and ap->pmp_link are freed.

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