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

[Bart Van Assche <bvanassche@acm.org>]

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.