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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Race condition with hardware completions leading to double-release of DMA and PM resources.
- [High] Unconditional hardware link recovery for non-timeout START STOP UNIT failures.
--

--- Patch [2]: [PATCH 2/3] ufs: core: Fix a race condition triggered by ufshcd_eh_timed_out() ---
commit 5124f897a6830560d1ea1c644750541f1ef1fc0b
Author: Bart Van Assche <[email protected]>

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

This patch moves ufshcd_link_recovery() out of the timeout handler and into
ufshcd_set_dev_pwr_mode() to prevent deadlocks. It also removes the checking
of SCMD_STATE_COMPLETE before command cleanup in the timeout handler.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 6b94f0d91790c..cdfcd3d4c4c97 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9550,34 +9550,18 @@ 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;
> -			struct request *rq = scsi_cmd_to_rq(scmd);
> +	if (!hba->mcq_enabled) {
> +		struct request *rq = scsi_cmd_to_rq(scmd);
>  
> -			spin_lock_irqsave(&hba->outstanding_lock, flags);
> +		scoped_guard(spinlock_irqsave, &hba->outstanding_lock)
>  			__clear_bit(rq->tag, &hba->outstanding_reqs);
> -			spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> -		}
> -
> -		set_host_byte(scmd, DID_TIME_OUT);
> -		if (ufshcd_is_scsi_cmd(scmd))
> -			ufshcd_release_scsi_cmd(hba, scmd);
> -		scsi_done(scmd);
>  	}
>  
> +	set_host_byte(scmd, DID_TIME_OUT);
> +	if (ufshcd_is_scsi_cmd(scmd))
> +		ufshcd_release_scsi_cmd(hba, scmd);

[Severity: Critical]
Since the synchronous ufshcd_link_recovery() call has been removed from this
handler, the controller is still running and could complete this command
concurrently.

By removing the check for SCMD_STATE_COMPLETE, doesn't this create a race
where both the timeout handler and the hardware completion interrupt
(via ufshcd_compl_one_cqe()) might call ufshcd_release_scsi_cmd() on the
same command simultaneously?

This could lead to double-unmapping DMA regions and decrementing PM clock
gating refcounts twice.

> +	scsi_done(scmd);
> +
>  	return SCSI_EH_DONE;
>  }
>  
> @@ -10091,9 +10075,13 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>  		sdev_printk(KERN_WARNING, sdp,
>  			    "START_STOP failed for power mode: %d, result %x\n",
>  			    pwr_mode, ret);
> +
> +		ufshcd_link_recovery(hba);

[Severity: High]
Does this unconditionally trigger a full hardware link recovery for any
non-zero return value from scsi_execute_cmd()?

The scsi_execute_cmd() call can fail for reasons other than timeouts,
such as memory pressure returning -ENOMEM or normal device check conditions.
Calling ufshcd_link_recovery() in these scenarios seems like it would cause
severe performance degradation and disruptive device resets for routine errors.

Should this check the specific failure reason, like DID_TIME_OUT, before
initiating recovery?

> +
>  		if (ret > 0) {
>  			if (scsi_sense_valid(&sshdr))
>  				scsi_print_sense_hdr(sdp, NULL, &sshdr);
> +
>  			ret = -EIO;
>  		}
>  	} else {

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