Re: [PATCH] ata: libahci: Do not use an EM slot for the PMP control port

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] ata: libahci: active timer reinitialization across
  suspend/resume in ahci_init_sw_activity() [1]
--

commit 05fb8ee9732c9d4452a8089e2686c99e78769965
Author: Niklas Cassel <cassel@kernel.org>

ata: libahci: Do not use an EM slot for the PMP control port

This commit skips enclosure management software activity updates when
link->pmp is at or above EM_MAX_SLOTS, avoiding an out-of-bounds access on
pp->em_priv[] for the PMP control port.

> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 9e970408916d1..ef760d81d1d1f 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1033,11 +1033,16 @@ static void ahci_sw_activity(struct ata_link *link)
>  {
>  	struct ata_port *ap = link->ap;
>  	struct ahci_port_priv *pp = ap->private_data;
> -	struct ahci_em_priv *emp = &pp->em_priv[link->pmp];
> +	struct ahci_em_priv *emp;
>  
>  	if (!(link->flags & ATA_LFLAG_SW_ACTIVITY))
>  		return;
>  
> +	/* The PMP control port (SATA_PMP_CTRL_PORT) has no EM slot */
> +	if (link->pmp >= EM_MAX_SLOTS)
> +		return;
> +
> +	emp = &pp->em_priv[link->pmp];
>  	emp->activity++;
>  	if (!timer_pending(&emp->timer))
>  		mod_timer(&emp->timer, jiffies + msecs_to_jiffies(10));

[Severity: High]
This problem wasn't introduced by this patch, but can emp->timer remain
armed across port suspend and be reinitialized while still queued on
resume [1]?

During port suspend, ata_eh_handle_port_suspend() issues a standby command
through ahci_qc_issue() -> ahci_sw_activity(), arming emp->timer, and
ahci_sw_activity_blink() re-arms it for another 100ms on expiration.
ahci_port_suspend() -> ahci_deinit_port() does not cancel emp->timer.

If ahci_port_resume() -> ahci_start_port() -> ahci_init_sw_activity() runs
while emp->timer is still pending, timer_setup() overwrites the queued
timer's hlist linkage:

drivers/ata/libahci.c:ahci_init_sw_activity() {
	...
	emp->saved_activity = emp->activity = 0;
	emp->link = link;
	timer_setup(&emp->timer, ahci_sw_activity_blink, 0);
	...
}

When that timer later expires, __hlist_del() hits a NULL pointer
dereference.  If the timer instead expires while the port is suspended,
ahci_sw_activity_blink() -> ahci_transmit_led_message() ->
ahci_rpm_get_port() calls pm_runtime_get_sync() from timer softirq context,
sleeping in atomic context and touching a powered-down controller.

[1] https://sashiko.dev/#/bug/linux-b496ab4d-84b4-4d7d-8517-70f1a3e845a2

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110124742.275022-2-cassel@kernel.org?part=1