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

[sashiko-bot@kernel.org]

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