Re: [PATCH] scsi: libsas: Fix SMP IO deadlock during HA resume

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Bypassing PM reference counting via a global state flag creates a race condition where concurrent independent threads can submit SMP I/Os without holding a PM reference, risking premature hardware suspend.

Pre-existing issues:
- [High] The return value of pm_runtime_get_sync() is ignored, which can lead to accessing powered-off hardware on failure.
--

commit ecfcb25c960bd684b2b9ddb5e30db0c49c3a78b4
Author: Xingui Yang <[email protected]>
scsi: libsas: Fix SMP IO deadlock during HA resume

This commit attempts to fix a deadlock during sas_resume_ha() by
skipping pm_runtime_get_sync() and pm_runtime_put_sync() when the
SAS_HA_RESUMING flag is set. It targets expander-attached ATA devices
triggered by ATA EH via sas_phy_reset().

> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 811c9eb4fef11..77ae2ae7b2c45 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -61,8 +61,17 @@ static int smp_execute_task_sg(struct domain_device *dev,
>  	struct sas_internal *i =
>  		to_sas_internal(dev->port->ha->shost->transportt);
>  	struct sas_ha_struct *ha = dev->port->ha;
> -
> -	pm_runtime_get_sync(ha->dev);
> +	bool skip_pm = test_bit(SAS_HA_RESUMING, &ha->state);

[Severity: High]
Does bypassing PM reference counting via a global state flag create a race
condition here in smp_execute_task_sg() in drivers/scsi/libsas/sas_expander.c?

Concurrent independent threads could submit SMP I/Os without holding a PM
reference, risking premature hardware suspend.

For example, if a concurrent userspace SMP request via bsg executes during
sas_resume_ha(), smp_execute_task_sg() could evaluate test_bit for
SAS_HA_RESUMING to true and skip pm_runtime_get_sync(). Meanwhile,
sas_resume_ha() might unblock SCSI requests via scsi_unblock_requests() and
eventually clear SAS_HA_RESUMING, dropping the resume thread's PM reference.

Could this allow the device to autosuspend while the BSG thread is still
waiting for the SMP IO to complete, potentially causing a system crash, bus
fault, or IO timeout?

> +
> +	/*
> +	 * Skip PM get/put during HA resume to avoid deadlock: the host is
> +	 * RPM_RESUMING and the drain waits for this SMP IO to finish, but
> +	 * pm_runtime_get_sync() would block on RPM_RESUMING. Safe because
> +	 * hardware is already initialized by the LLDD before call
> +	 * sas_resume_ha().
> +	 */
> +	if (!skip_pm)
> +		pm_runtime_get_sync(ha->dev);

[Severity: High]
This isn't a bug introduced by this patch, but the return value of
pm_runtime_get_sync() is ignored here in smp_execute_task_sg().

If pm_runtime_get_sync() fails to resume the device due to a hardware fault
or disabled runtime PM, the function will blindly proceed to allocate and
issue SMP tasks to the LLDD. Accessing suspended or unclocked hardware
registers can cause unrecoverable bus faults or system lockups.

Should the negative error code be checked before proceeding to access
hardware?

>  	mutex_lock(&dev->ex_dev.cmd_mutex);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1