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

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

Pre-existing issues:
- [High] Lockless modification of the `eh_noresume` bitfield causes a read-modify-write data race that can silently corrupt adjacent critical bitfields.
- [High] If `scsi_autopm_get_host()` persistently fails, the SCSI EH kthread enters a 100% CPU uninterruptible infinite loop.
--

--- Patch [1]: [PATCH v2] scsi: core: pair EH runtime PM get and put ---
commit 78d8d003e96d61ac7f551e9faf3bb914d7c82cad
Author: Hongjie Fang <[email protected]>

scsi: core: pair EH runtime PM get and put

This commit fixes a race condition where a power-management error path alters
shost->eh_noresume while the SCSI EH thread is running. It ensures that
runtime PM get and put operations in the error handler are correctly paired
by snapshotting shost->eh_noresume per iteration.

> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 984ddcb4786d6..d4c53e6657625 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -2213,7 +2214,8 @@ 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) {
> +		eh_noresume = shost->eh_noresume;

[Severity: High]
This isn't a bug introduced by this patch, but does the lockless modification
of the eh_noresume bitfield elsewhere cause a read-modify-write data race
that can silently corrupt adjacent bitfields?

For instance, eh_noresume is defined as a bitfield sharing a machine word
with other critical flags:

include/scsi/scsi_host.h:struct Scsi_Host {
    ...
    unsigned host_self_blocked:1;
    unsigned reverse_ordering:1;
    unsigned tmf_in_progress:1;
    unsigned async_scan:1;
    unsigned eh_noresume:1;
    ...
}

When ufshcd_set_dev_pwr_mode() locklessly modifies hba->host->eh_noresume = 1
after dropping host_lock, could concurrent modifications to other bitfields
in the same word (like host_self_blocked during scsi_block_requests()) cause
lost state updates due to non-atomic read-modify-write operations?

> +		if (!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;

[Severity: High]
This is a pre-existing issue, but if scsi_autopm_get_host() persistently
fails, does this continue statement cause the SCSI EH kthread to enter an
uninterruptible infinite loop?

If scsi_autopm_get_host() fails (e.g. -ENODEV due to dead hardware) and the
code skips scsi_unjam_host() by executing continue, shost->host_failed
remains non-zero. The loop restarts and checks its wait condition:

drivers/scsi/scsi_error.c:scsi_error_handler() {
    ...
    if ((shost->host_failed == 0 && shost->host_eh_scheduled == 0) ||
        shost->host_failed != atomic_read(&shost->host_busy)) {
        ...
        schedule();
        continue;
    }
    ...
}

Because host_failed is not 0 and host_failed == host_busy, this condition
evaluates to false, skipping schedule(). Will the thread then immediately
retry scsi_autopm_get_host(), fail again, and spin a CPU core infinitely
without yielding?

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