Re: [PATCH 1/1] scsi: ufs: core: Fix task management completion timeout race

[Adrian Hunter <adrian.hunter@intel.com>]

On 14/10/2021 21:50, Adrian Hunter wrote:
> On 14/10/2021 19:47, Bart Van Assche wrote:
>> On 10/13/21 11:02 PM, Adrian Hunter wrote:
>>> On 14/10/2021 07:14, Bart Van Assche wrote:
>>>> Wouldn't it be better to keep the code that clears req->end_io_data
>>>> and to change complete(c) into if(c) complete(c) in
>>>> ufshcd_tmc_handler()?
>>>
>>> If that were needed, it would imply the synchronization was broken
>>> i.e. why are we referencing a request that has already been through
>>> blk_put_request()?
>>
>> The scenario I'm worried about is as follows:
>> * __ufshcd_issue_tm_cmd() issues a task management function.
>> * No completion is received before TM_CMD_TIMEOUT has expired (100 ms).
>> * ufshcd_clear_tm_cmd() fails.
>> * The TMF completes, ufshcd_tmc_handler() is called and that function calls complete(req->end_io_data).
>>
>> Can this happen?
> 
> No because the tag's bit is cleared from outstanding_tasks before blk_put_request() and
> access to outstanding_tasks is protected by host_lock in both __ufshcd_issue_tm_cmd()
> and ufshcd_clear_tm_cmd().

Although I just noticed a different issue.

In ufshcd_tmc_handler() the task doorbell register needs to be read
in conjunction with outstanding_tasks i.e. under the spinlock

I will send a V2.

> 
>>
>> I agree that this scenario involves completion of a request that has already been through blk_put_request().
>>
>> Thanks,
>>
>> Bart.
>