Re: [PATCH 08/11] scsi: ufs: Improve SCSI abort handling further

Bart Van Assche
2021-11-15T23:11:33Z

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.