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

[sashiko-bot@kernel.org]

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