Re: [PATCH v2] ufs: core: fix ufshcd_abort_all racing issue

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

On 6/25/24 1:29 AM, Peter Wang (王信友) wrote:
> On Mon, 2024-06-24 at 11:01 -0700, Bart Van Assche wrote:
>>   On 6/24/24 5:11 AM, peter.wang@mediatek.com wrote:
>>> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-
>> mcq.c
>>> index 8944548c30fa..3b2e5bcb08a7 100644
>>> --- a/drivers/ufs/core/ufs-mcq.c
>>> +++ b/drivers/ufs/core/ufs-mcq.c
>>> @@ -512,8 +512,9 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba,
>> int task_tag)
>>>    return -ETIMEDOUT;
>>>    
>>>    if (task_tag != hba->nutrs - UFSHCD_NUM_RESERVED) {
>>> -if (!cmd)
>>> -return -EINVAL;
>>> +/* Should return 0 if cmd is already complete by irq */
>>> +if (!cmd || !ufshcd_cmd_inflight(cmd))
>>> +return 0;
>>>    hwq = ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(cmd));
>>>    } else {
>>>    hwq = hba->dev_cmd_queue;
>>
>> Does the call trace show that blk_mq_unique_tag() tries to
>> dereference
>> address 0x194? If so, how is this possible? There are
>> only two lrbp->cmd assignments in the UFS driver. These assignments
>> either assign a valid SCSI command pointer or NULL. Even after a SCSI
>> command has been completed, the SCSI command pointer remains valid.
>> So
>> how can an invalid pointer be passed to blk_mq_unique_tag()? Please
>> root-cause this issue instead of posting a code change that reduces a
>> race window without closing the race window completely.
> 
> blk_mq_unique_tag() tries to dereference address 0x194, and it is null.
> Beacuse ISR end this IO by scsi_done, free request will be called and
> set mq_hctx null.
> The call path is
> scsi_done -> scsi_done_internal -> blk_mq_complete_request ->
> scsi_complete ->
> scsi_finish_command -> scsi_io_completion -> scsi_end_request ->
> __blk_mq_end_request ->
> blk_mq_free_request -> __blk_mq_free_request
> 
> And blk_mq_unique_tag will access mq_hctx then get null pointer error.
> Please reference
> https://elixir.bootlin.com/linux/latest/source/block/blk-mq.c#L713
> https://elixir.bootlin.com/linux/latest/source/block/blk-mq-tag.c#L680
> 
> So, the root-casue is very simple, free request then get hwq.
> This patch only check if reqesut not free(inflight) then get hwq.
> Thought it still have racing winodw, but it is better then do nothing,
> right?
> Or, maybe we get all cq_lock before get hwq to close the racing window.
> But the code may ugly, how do you think?

Please include a full root cause analysis when reposting fixes for the
reported crashes. It is not clear to me how it is possible that an
invalid pointer is passed to blk_mq_unique_tag() (0x194). As I mentioned
in my previous email, freeing a request does not modify the request
pointer and does not modify the SCSI command pointer either. As one can
derive from the blk_mq_alloc_rqs() call stack, memory for struct request
and struct scsi_cmnd is allocated at request queue allocation time and
is not freed until the request queue is freed. Hence, for a given tag,
neither the request pointer nor the SCSI command pointer changes as long
as a request queue exists. Hence my request for an explanation how it is
possible that an invalid pointer was passed to blk_mq_unique_tag().

Thanks,

Bart.