[PATCH v2 0/2] ufs: core: Improve error handling

[PATCH v2 0/2] ufs: core: Improve error handling

[Bart Van Assche]

Hi Martin,

This patch series fixes two subtle race conditions related to UFS error handling.
Please consider these changes for the next merge window.

Thanks,

Bart.

Changes compared to v1: fixed three Sashiko complaints.

Bart Van Assche (2):
  ufs: core: Fix a race condition triggered by ufshcd_eh_timed_out()
  ufs: core: Do not forcibly complete SCSI commands

 drivers/ufs/core/ufshcd.c | 34 ++++------------------------------
 1 file changed, 4 insertions(+), 30 deletions(-)

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

[Bart Van Assche]

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().

Fixes: 7029e2151a7c ("scsi: ufs: Fix a deadlock between PM and the SCSI error handler")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index a51e071916cf..d719100bfe38 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -9582,18 +9582,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;
@@ -10130,6 +10118,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 {

[PATCH v2 2/2] ufs: core: Do not forcibly complete SCSI commands

[Bart Van Assche]

The SCSI core error handler is responsible for deciding whether to abort or
to requeue SCSI commands. A SCSI LLD shouldn't interfere with this decision.
Hence, do not call scsi_done() from inside the UFS error handler.

Also call ufshcd_mcq_compl_all_cqes_lock() for reserved commands - there
is no reason to skip this call for reserved commands.

Fix the comment above ufshcd_mcq_compl_pending_transfer().

This patch makes the MCQ behavior consistent with the behavior for legacy mode.

Fixes: ab248643d3d6 ("scsi: ufs: core: Add error handling for MCQ mode")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d719100bfe38..b1d8900f9821 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -6020,28 +6020,13 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
 
 static bool ufshcd_mcq_force_compl_one(struct request *rq, void *priv)
 {
-	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
 	struct scsi_device *sdev = rq->q->queuedata;
 	struct Scsi_Host *shost = sdev->host;
 	struct ufs_hba *hba = shost_priv(shost);
 	struct ufs_hw_queue *hwq = ufshcd_mcq_req_to_hwq(hba, rq);
 
-	if (blk_mq_is_reserved_rq(rq) || !hwq)
-		return true;
-
-	ufshcd_mcq_compl_all_cqes_lock(hba, hwq);
-
-	/*
-	 * For those cmds of which the cqes are not present in the cq, complete
-	 * them explicitly.
-	 */
-	scoped_guard(spinlock_irqsave, &hwq->cq_lock) {
-		if (!test_bit(SCMD_STATE_COMPLETE, &cmd->state)) {
-			set_host_byte(cmd, DID_REQUEUE);
-			ufshcd_release_scsi_cmd(hba, cmd);
-			scsi_done(cmd);
-		}
-	}
+	if (hwq)
+		ufshcd_mcq_compl_all_cqes_lock(hba, hwq);
 
 	return true;
 }
@@ -6068,7 +6053,7 @@ static bool ufshcd_mcq_compl_one(struct request *rq, void *priv)
  * @hba: per adapter instance
  * @force_compl: This flag is set to true when invoked
  * from ufshcd_host_reset_and_restore() in which case it requires special
- * handling because the host controller has been reset by ufshcd_hba_stop().
+ * handling because the host controller has been disabled by ufshcd_hba_stop().
  */
 static void ufshcd_mcq_compl_pending_transfer(struct ufs_hba *hba,
 					      bool force_compl)

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

[sashiko-bot]

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

Re: [PATCH v2 2/2] ufs: core: Do not forcibly complete SCSI commands

[sashiko-bot]

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

New issues:
- [High] Removing explicit SCSI command completion in MCQ error handler strands commands, causing 30s I/O stalls and resource leaks.

Pre-existing issues:
- [Critical] Reading the CQ tail pointer when the host is disabled can return 0xFFFFFFFF, causing an out-of-bounds array access and kernel panic.
- [High] Redundant queue polling causes O(N*M) complexity under spinlock and floods the console with errors.
--

--- Patch [2]: [PATCH v2 2/2] ufs: core: Do not forcibly complete SCSI commands ---
commit bd53d1703220f927f3a5c88813e22aa95471d9d4
Author: Bart Van Assche <bvanassche@acm.org>
Subject: ufs: core: Do not forcibly complete SCSI commands

This commit prevents the UFS error handler from explicitly completing SCSI 
commands, delegating the decision to the SCSI core error handler. It removes
the explicit scsi_done() call from the MCQ error handling path.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 63ca6d489be02..8e82fa6882021 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -6020,28 +6020,13 @@ static int ufshcd_poll(struct Scsi_Host *shost, unsigned int queue_num)
>  
>  static bool ufshcd_mcq_force_compl_one(struct request *rq, void *priv)
>  {
[ ... ]
> -	if (blk_mq_is_reserved_rq(rq) || !hwq)
> -		return true;
> -
> -	ufshcd_mcq_compl_all_cqes_lock(hba, hwq);
> -
> -	/*
> -	 * For those cmds of which the cqes are not present in the cq, complete
> -	 * them explicitly.
> -	 */
> -	scoped_guard(spinlock_irqsave, &hwq->cq_lock) {
> -		if (!test_bit(SCMD_STATE_COMPLETE, &cmd->state)) {
> -			set_host_byte(cmd, DID_REQUEUE);
> -			ufshcd_release_scsi_cmd(hba, cmd);
> -			scsi_done(cmd);
> -		}
> -	}
> +	if (hwq)
> +		ufshcd_mcq_compl_all_cqes_lock(hba, hwq);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
ufshcd_mcq_compl_all_cqes_lock() here cause an O(N*M) loop under spinlock?

Since this function is invoked by blk_mq_tagset_busy_iter() for every busy
request, it appears we scan the entire hardware queue (M slots) for every one
of the N busy requests.

Furthermore, ufshcd_mcq_compl_all_cqes_lock() unconditionally iterates
max_entries times:

drivers/ufs/core/ufs-mcq.c:ufshcd_mcq_compl_all_cqes_lock() {
    ...
    while (entries > 0) {
        ufshcd_mcq_process_cqe(hba, hwq);
        ufshcd_mcq_inc_cq_head_slot(hwq);
        entries--;
    }
    ...
}

Inside ufshcd_mcq_process_cqe(), empty slots log "Abnormal CQ entry!".
Will this flood the console with N * M error messages, potentially causing
hard lockup warnings when interrupts are disabled for so long?

[Severity: Critical]
This is also a pre-existing issue, but is it safe to read the CQ tail pointer
here when the host might be disabled or disconnected?

In ufshcd_mcq_compl_all_cqes_lock(), the tail pointer is read from MMIO:

drivers/ufs/core/ufs-mcq.c:ufshcd_mcq_compl_all_cqes_lock() {
    ...
    ufshcd_mcq_update_cq_tail_slot(hwq);
    hwq->cq_head_slot = hwq->cq_tail_slot;
    ...
}

If the bus is inaccessible (e.g. surprise removal or PCIe master abort),
the register read might return 0xFFFFFFFF. Does this assign an out-of-bounds
index to cq_head_slot, which would then be dereferenced by ufshcd_mcq_cur_cqe()
on the next iteration of the tagset iterator, leading to a kernel panic?

>  
>  	return true;

[Severity: High]
By removing the explicit completion and ufshcd_release_scsi_cmd() here, do
these commands get stranded in flight?

If the host controller was reset, these commands will have no CQE and will
remain stuck until the block layer timer expires (typically 30 seconds),
causing a severe I/O stall.

When SCSI EH eventually aborts them, ufshcd_mcq_abort() returns SUCCESS
without calling ufshcd_release_scsi_cmd():

drivers/ufs/core/ufs-mcq.c:ufshcd_mcq_abort() {
    ...
    return SUCCESS;
}

Does this result in permanent leakage of crypto PRDTs and clock scaling busy
counts?

The commit message states this aligns MCQ with legacy mode, but doesn't
legacy mode implicitly complete all commands during a reset because the
doorbell register reads as 0, tricking ufshcd_transfer_req_compl() into
completing them all?

>  }

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