On 11/12/21 2:56 AM, Adrian Hunter wrote:
On 10/11/2021 20:56, Bart Van Assche wrote:
On 11/10/21 12:57 AM, Adrian Hunter wrote:
Seems like something ufshcd_clear_cmd() should be doing instead?
I'm concerned that would break ufshcd_eh_device_reset_handler()
>> since that reset handler retries SCSI commands by calling
>> __ufshcd_transfer_req_compl() after having called ufshcd_clear_cmd().
Whenever an outstanding_reqs bit transitions 1 -> 0, then
__ufshcd_transfer_req_compl() must be called.
I will look further into this.
As a separate issue, in ufshcd_abort() there is:
/* If command is already aborted/completed, return FAILED. */
if (!(test_bit(tag, &hba->outstanding_reqs))) {
dev_err(hba->dev,
"%s: cmd at tag %d already completed, outstanding=0x%lx, doorbell=0x%x\n",
__func__, tag, hba->outstanding_reqs, reg);
goto release;
}
which seems wrong. FAILED should only be returned to escalate the
error handling, so if the slot has already successfully been
cleared, that is SUCCESS. scsi_times_out() has already blocked
the scsi_done() path (by setting SCMD_STATE_COMPLETE), so any use
after free must be being caused by SCSI not the ufs driver.
scmd_eh_abort_handler() would trigger a use-after-free if ufshcd_abort()
would return SUCCESS for completed commands. Hence the choice for the
return value FAILED for completed commands.
BTW, can this code path ever be reached since scsi_done() sets the
SCMD_STATE_COMPLETE bit before it calls blk_mq_complete_request() and
since scsi_times_out() tests that bit before it schedules a call of
ufshcd_abort()?
Thanks,
Bart.
