Re: [PATCH v1] scsi: core: pair EH runtime PM get and put

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unprotected concurrent modification of the `eh_noresume` bitfield causes data corruption and torn writes of adjacent bitfields in `struct Scsi_Host`.
- [High] If `scsi_autopm_get_host()` persistently returns an error, the SCSI error handler thread enters an infinite busy loop, completely locking up a CPU core.
--

commit ac1a188a1cc95a2e48943738d61761f7fb9d8f35
Author: Hongjie Fang <hongjiefang@asrmicro.com>

scsi: core: pair EH runtime PM get and put

This patch fixes a potential runtime PM usage count underflow by ensuring
scsi_autopm_put_host() is only called if the corresponding get operation
in the same iteration succeeded. It introduces a boolean variable to
track this state.

> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 147127fb4db9c..c78ea64641a98 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c

[ ... ]

> @@ -2383,12 +2384,16 @@ int scsi_error_handler(void *data)
>  		 * what we need to do to get it up and online again (if we can).
>  		 * If we fail, we end up taking the thing offline.
>  		 */
> -		if (!shost->eh_noresume && scsi_autopm_get_host(shost) != 0) {
> -			SCSI_LOG_ERROR_RECOVERY(1,
> -				shost_printk(KERN_ERR, shost,
> -					     "scsi_eh_%d: unable to autoresume\n",
> -					     shost->host_no));
> -			continue;
> +		autopm_put = false;
> +		if (!shost->eh_noresume) {

[Severity: High]
This isn't a bug introduced by this patch, but could this concurrent read
and update of shost->eh_noresume be unsafe?

Looking at include/scsi/scsi_host.h, eh_noresume is a 1-bit bitfield that
shares the same memory word as tmf_in_progress and other state flags:

	unsigned tmf_in_progress:1;
	unsigned eh_noresume:1;

If eh_noresume is modified directly (for example in
ufshcd_set_dev_pwr_mode() in drivers/ufs/core/ufshcd.c) without holding
shost->host_lock, could this cause torn writes that corrupt other adjacent
bitfields modified by concurrent threads holding the lock?

> +			if (scsi_autopm_get_host(shost) != 0) {
> +				SCSI_LOG_ERROR_RECOVERY(1,
> +					shost_printk(KERN_ERR, shost,
> +						     "scsi_eh_%d: unable to autoresume\n",
> +						     shost->host_no));
> +				continue;

[Severity: High]
This is a pre-existing issue, but if scsi_autopm_get_host() persistently
returns an error, won't this continue statement cause an infinite CPU-hogging
loop?

Since continue bypasses the rest of the loop, the cleanup step
(shost->host_failed = 0) is never executed.

On the next iteration, the initial sleep condition at the top of the loop
will evaluate to false because host_failed is still equal to
scsi_host_busy(). This bypasses schedule() and immediately loops to retry
the resume operation ad infinitum.

> +			}
> +			autopm_put = true;
>  		}
>  
>  		if (shost->transportt->eh_strategy_handler)

[ ... ]

> @@ -2407,7 +2412,7 @@ int scsi_error_handler(void *data)
>  		 * which are still online.
>  		 */
>  		scsi_restart_operations(shost);
> -		if (!shost->eh_noresume)
> +		if (autopm_put)
>  			scsi_autopm_put_host(shost);
>  	}
>  	__set_current_state(TASK_RUNNING);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623074408.1639663-1-hongjiefang@asrmicro.com?part=1