Re: [PATCH v2 1/2] ufs: core: Fix a race condition triggered by ufshcd_eh_timed_out()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Force-completing timed-out SCSI commands without aborting the hardware transaction causes fatal DMA double-unmaps and tag reuse corruption.
- [High] Unconditional invocation of `ufshcd_link_recovery()` for any positive return value erroneously triggers full hardware resets on non-fatal SCSI responses.
--

--- Patch [1]: [PATCH v2 1/2] ufs: core: Fix a race condition triggered by ufshcd_eh_timed_out() ---
commit dde417683cb3b7c051cd6097ff9874af45826071
Author: Bart Van Assche <bvanassche@acm.org>

ufs: core: Fix a race condition triggered by ufshcd_eh_timed_out()

If a START STOP UNIT command times out, the ufshcd_link_recovery() call
in ufshcd_eh_timed_out() may call scsi_done() for that command via the
force-completion mechanism. This may cause ufshcd_set_dev_pwr_mode() to
return before link recovery has finished. Fix this race condition by
moving the ufshcd_link_recovery() call into ufshcd_set_dev_pwr_mode().

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 69d92387e9d29..63ca6d489be02 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9550,18 +9550,6 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
>  		return SCSI_EH_NOT_HANDLED;
>  	}
>  
> -	/*
> -	 * Handle the timeout directly to prevent a deadlock between
> -	 * ufshcd_set_dev_pwr_mode() and ufshcd_err_handler().
> -	 */
> -	ufshcd_link_recovery(hba);
> -	dev_info(hba->dev, "%s() finished; outstanding_tasks = %#lx.\n",
> -		 __func__, hba->outstanding_tasks);
> -
> -	/*
> -	 * ufshcd_link_recovery() may already have completed @scmd, e.g. via
> -	 * the existing MCQ force-completion path.
> -	 */
>  	if (!test_bit(SCMD_STATE_COMPLETE, &scmd->state)) {
>  		if (!hba->mcq_enabled) {
>  			unsigned long flags;

[Severity: Critical]
By removing ufshcd_link_recovery() here, does this allow the software to
complete timed-out SCSI commands without aborting the hardware transaction?

In the subsequent code within ufshcd_eh_timed_out(), the driver unmaps DMA
buffers and calls scsi_done(scmd) to complete the command:

	if (ufshcd_is_scsi_cmd(scmd)) {
		set_host_byte(scmd, DID_REQUEUE);
		ufshcd_release_scsi_cmd(hba, scmd);
	} else {
		set_host_byte(scmd, DID_TIME_OUT);
	}

	scsi_done(scmd);

Since the hardware controller is no longer stopped, won't it continue
executing the command and potentially perform DMA writes to the now-freed
memory buffers?

If the controller completes the command before the caller executes
ufshcd_link_recovery(), this might process a completion for an already freed
tag, causing memory corruption or a use-after-free.

[ ... ]

> @@ -10098,6 +10086,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>  		if (ret > 0) {
>  			if (scsi_sense_valid(&sshdr))
>  				scsi_print_sense_hdr(sdp, NULL, &sshdr);
> +			ufshcd_link_recovery(hba);
>  			ret = -EIO;
>  		}
>  	} else {

[Severity: High]
Does this unconditionally execute a full link recovery for any positive return
value?

When scsi_execute_cmd() executes the start/stop unit command, it can return a
positive status code if the device rejects the command with a standard SCSI
error (such as SAM_STAT_CHECK_CONDITION).

By executing ufshcd_link_recovery() here for all ret > 0, we seem to be
triggering a full hardware reset on non-fatal SCSI responses instead of
isolating actual timeout codes. Could this lead to unintended host resets
and system latency spikes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786569828.git.bvanassche@acm.org?part=1