[PATCH] scsi: ufs: mcq: Reap aborted CQEs before returning from abort

[PATCH] scsi: ufs: mcq: Reap aborted CQEs before returning from abort

[Jeuk Kim]

ufshcd_mcq_abort() can return before the OCS_ABORTED CQE is reaped.
If the command is retried or reused, the late CQE can unmap its new
DMA buffers or access a freed SG table, triggering a KASAN
null-ptr-deref in dma_direct_unmap_sg().

Reap the CQE and serialize resource release with CQ processing before
returning. On timeout, wait for a host reset to invalidate late CQEs;
a successful LU reset alone allows the SCSI core to reuse the command.

Reproduced with QEMU UFS in MCQ mode using fault injection and a
test-only delay in OCS_ABORTED CQE processing.

Fixes: 53755903b935 ("scsi: ufs: mcq: Delete ufshcd_release_scsi_cmd() in ufshcd_mcq_abort()")
Signed-off-by: Jeuk Kim <jeuk20.kim@samsung.com>
---
 drivers/ufs/core/ufs-mcq.c     | 31 ++++++++++++++++++++++++++++++-
 drivers/ufs/core/ufshcd-priv.h |  2 ++
 drivers/ufs/core/ufshcd.c      |  9 +++++++++
 include/ufs/ufshcd.h           |  2 ++
 4 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index df293b66350d..efbe4d1487b4 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -692,11 +692,20 @@ static bool ufshcd_mcq_sqe_search(struct ufs_hba *hba,
 	return ret;
 }
 
+static bool ufshcd_mcq_cmd_reaped(struct ufs_hba *hba, struct ufs_hw_queue *hwq,
+				  struct scsi_cmnd *cmd)
+{
+	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
+
+	ufshcd_mcq_poll_cqe_lock(hba, hwq);
+	return test_bit(UFSHCD_LRB_RELEASED, &lrbp->flags);
+}
+
 /**
  * ufshcd_mcq_abort - Abort the command in MCQ.
  * @cmd: The command to be aborted.
  *
- * Return: SUCCESS or FAILED error codes
+ * Return: SUCCESS, FAILED, or -ETIMEDOUT if the CQE was not reaped.
  */
 int ufshcd_mcq_abort(struct scsi_cmnd *cmd)
 {
@@ -705,6 +714,7 @@ int ufshcd_mcq_abort(struct scsi_cmnd *cmd)
 	int tag = scsi_cmd_to_rq(cmd)->tag;
 	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
 	struct ufs_hw_queue *hwq;
+	bool reaped;
 	int err;
 
 	/* Skip task abort in case previous aborts failed and report failure */
@@ -743,5 +753,24 @@ int ufshcd_mcq_abort(struct scsi_cmnd *cmd)
 		return FAILED;
 	}
 
+	/* Reserved commands do not set UFSHCD_LRB_RELEASED. */
+	if (blk_mq_is_reserved_rq(scsi_cmd_to_rq(cmd)))
+		return SUCCESS;
+
+	/* Reap the CQE before the SCSI core can reuse the command. */
+	err = read_poll_timeout(ufshcd_mcq_cmd_reaped, reaped, reaped, 20,
+				MCQ_POLL_US, false, hba, hwq, cmd);
+
+	/* Wait for any CQ handler still releasing the command. */
+	scoped_guard(spinlock_irqsave, &hwq->cq_lock)
+		ufshcd_release_scsi_cmd(hba, cmd);
+
+	if (err) {
+		dev_err(hba->dev, "%s: no completion for aborted tag %d\n",
+			__func__, tag);
+		lrbp->req_abort_skip = true;
+		return -ETIMEDOUT;
+	}
+
 	return SUCCESS;
 }
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index e55c2a02c1f5..b899014ea70e 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -6,6 +6,8 @@
 #include <linux/pm_runtime.h>
 #include <ufs/ufshcd.h>
 
+#define UFSHCD_LRB_RELEASED	0
+
 void ufshcd_enable_intr(struct ufs_hba *hba, u32 intrs);
 
 static inline bool ufshcd_is_user_access_allowed(struct ufs_hba *hba)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index cc75384c6502..38148d8081f8 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3029,6 +3029,7 @@ static void __ufshcd_setup_cmd(struct ufs_hba *hba, struct scsi_cmnd *cmd,
 	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
 
 	ufshcd_init_lrb(hba, cmd);
+	lrbp->flags = 0;
 
 	memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr));
 
@@ -5968,6 +5969,11 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 /* Release the resources allocated for processing a SCSI command. */
 void ufshcd_release_scsi_cmd(struct ufs_hba *hba, struct scsi_cmnd *cmd)
 {
+	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
+
+	if (test_and_set_bit(UFSHCD_LRB_RELEASED, &lrbp->flags))
+		return;
+
 	scsi_dma_unmap(cmd);
 	ufshcd_crypto_clear_prdt(hba, cmd);
 	ufshcd_release(hba);
@@ -8183,6 +8189,9 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 	if (hba->mcq_enabled) {
 		/* MCQ mode. Branch off to handle abort for mcq mode */
 		err = ufshcd_mcq_abort(cmd);
+		/* A LU reset cannot invalidate a late CQE. */
+		if (err == -ETIMEDOUT)
+			err = ufshcd_eh_host_reset_handler(cmd);
 		goto release;
 	}
 
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index be62165b785e..1892821c5054 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -166,6 +166,7 @@ struct ufs_pm_lvl_states {
  * @lun: LUN of the command
  * @intr_cmd: Interrupt command (doesn't participate in interrupt aggregation)
  * @req_abort_skip: skip request abort task flag
+ * @flags: per-command flags
  * @issue_time_stamp: time stamp for debug purposes (CLOCK_MONOTONIC)
  * @issue_time_stamp_local_clock: time stamp for debug purposes (local_clock)
  * @compl_time_stamp: time stamp for statistics (CLOCK_MONOTONIC)
@@ -190,6 +191,7 @@ struct ufshcd_lrb {
 	u8 lun; /* UPIU LUN id field is only 8-bit wide */
 	bool intr_cmd;
 	bool req_abort_skip;
+	unsigned long flags;
 	ktime_t issue_time_stamp;
 	u64 issue_time_stamp_local_clock;
 	ktime_t compl_time_stamp;
-- 
2.43.0

Re: [PATCH] scsi: ufs: mcq: Reap aborted CQEs before returning from abort

[Bart Van Assche]

On 10/26/26 2:17 AM, Jeuk Kim wrote:
 > ufshcd_mcq_abort() can return before the OCS_ABORTED CQE is reaped.
 > If the command is retried or reused, the late CQE can unmap its new
 > DMA buffers or access a freed SG table, triggering a KASAN
 > null-ptr-deref in dma_direct_unmap_sg().

This also needs a second sentence. If the late CQE arrives after the
command has been resubmitted, ufshcd_compl_one_cqe() also overwrites
cmd->result and calls scsi_done() for the new instance of the command,
because SCMD_STATE_COMPLETE was cleared when the command was
resubmitted. Hence, making ufshcd_release_scsi_cmd() idempotent is not
enough on its own. What matters is that the CQE is reaped before the
abort handler returns.

 > +	/* Wait for any CQ handler still releasing the command. */
 > +	scoped_guard(spinlock_irqsave, &hwq->cq_lock)
 > +		ufshcd_release_scsi_cmd(hba, cmd);
 > +
 > +	if (err) {
 > +		dev_err(hba->dev, "%s: no completion for aborted tag %d\n",
 > +			__func__, tag);
 > +		lrbp->req_abort_skip = true;
 > +		return -ETIMEDOUT;
 > +	}

If the poll times out, this unmaps the data buffer (and with swiotlb, 
frees the bounce buffer) while the controller may still own the command. 
The host reset only runs later, in ufshcd_abort(). That is the same 
use-after-unmap this patch tries to fix, just moved to a different 
place. Please release the command only after the host reset has 
completed in the timeout case. Note that ufshcd_mcq_force_compl_one() 
skips commands with SCMD_STATE_COMPLETE set, so the abort path does 
still have to release timed-out commands itself.

 > +	err = read_poll_timeout(ufshcd_mcq_cmd_reaped, reaped, reaped, 20,
 > +				MCQ_POLL_US, false, hba, hwq, cmd);

UFSHCD_LRB_RELEASED is used for two things here: "resources released" 
and "CQE reaped". That is why reserved commands need a special case. 
Have you considered setting a separate "completed" bit in 
ufshcd_compl_one_cqe() for every command type? Then the reserved-command 
special case goes away, and ufshcd_release_scsi_cmd() keeps its current 
semantics, so double releases elsewhere would no longer be silently hidden.

Also, ufshcd_clear_lu_cmds() polls the CQ only once after 
ufshcd_clear_cmd(). Does UFSHCI guarantee that the OCS_ABORTED CQE has 
been posted by the time SQRTSy.CUS is set? If so, please say so in the 
commit message and explain why polling is needed here. If not, 
ufshcd_clear_lu_cmds() has the same race and should be fixed too.

 > - * Return: SUCCESS or FAILED error codes
 > + * Return: SUCCESS, FAILED, or -ETIMEDOUT if the CQE was not reaped.

Please don't mix SCSI EH return codes with negative errno values in one 
return value. It would be cleaner to do the host reset (followed by the 
release) inside ufshcd_mcq_abort(), or to report the timeout some other way.

 > +		/* A LU reset cannot invalidate a late CQE. */
 > +		if (err == -ETIMEDOUT)
 > +			err = ufshcd_eh_host_reset_handler(cmd);

Calling ufshcd_eh_host_reset_handler() from ufshcd_abort() is completely
unacceptable. Only the SCSI core is allowed to call
ufshcd_eh_host_reset_handler(). Additionally, calls of
ufshcd_eh_host_reset_handler() must only happen after all commands
either have timed out or have been aborted.

 > +	lrbp->flags = 0;

The kernel-doc for @flags should say which bits it holds (UFSHCD_LRB_*).
I'd also define the bit next to struct ufshcd_lrb rather than in
ufshcd-priv.h.

The email was sent From: jeuk20.kim@gmail.com, but the Signed-off-by is 
@samsung.com, and the body has no `From:` line. `git am` will record the 
gmail address as the author, so checkpatch will complain about the 
mismatch. Please add an in-body `From: Jeuk Kim 
<jeuk20.kim@samsung.com>` line.

Please add `Cc: stable@vger.kernel.org`.

Thanks,

Bart.

Re: [PATCH] scsi: ufs: mcq: Reap aborted CQEs before returning from abort

[Jeuk Kim]

Hi Bart,

Thanks for the review.

> If the late CQE arrives after the command has been resubmitted,
> ufshcd_compl_one_cqe() also overwrites cmd->result and calls scsi_done()
> for the new instance of the command

Fixed in v2.

> Please release the command only after the host reset has completed in
> the timeout case.

Fixed in v2. On timeout, abort returns FAILED without releasing DMA.
Recovery is left to the SCSI core.

> Note that ufshcd_mcq_force_compl_one() skips commands with
> SCMD_STATE_COMPLETE set

Fixed in v2. It now releases timed-out commands' resources without
changing their result or calling scsi_done().

> Have you considered setting a separate "completed" bit in
> ufshcd_compl_one_cqe() for every command type?

Done in v2. UFSHCD_LRB_COMPLETED covers all command types, and
ufshcd_release_scsi_cmd() retains its original semantics.

> Does UFSHCI guarantee that the OCS_ABORTED CQE has been posted by the
> time SQRTSy.CUS is set?

CUS alone does not establish that software has consumed the CQE.
v2 waits for driver completion in ufshcd_mcq_sq_cleanup(), covering
both abort and LU reset, and propagates LU cleanup failures.

> Please don't mix SCSI EH return codes with negative errno values in
> one return value.

Fixed in v2.

> Calling ufshcd_eh_host_reset_handler() from ufshcd_abort() is
> completely unacceptable.

Removed in v2.

> The kernel-doc for @flags should say which bits it holds
> (UFSHCD_LRB_*). I'd also define the bit next to struct ufshcd_lrb
> rather than in ufshcd-priv.h.

Fixed in v2.

> Please add an in-body From: Jeuk Kim <jeuk20.kim@samsung.com> line.
> Please add Cc: stable@vger.kernel.org.

Added in v2.

Thanks,
Jeuk