ufs: core: fix ufshcd_abort_all racing issue

[ ... ]

drivers/ufs/core/ufs-mcq.c +0 -0

index 8944548c30fa..3b2e5bcb08a7 100644
@@ -512,8 +512,9 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag)
  		return -ETIMEDOUT;

External email : Please do not click links or open attachments until you have verified the sender or the content.

Quoted 1 line · On 6/24/24 5:11 AM, [email protected] wrote:

[ ... ]

In this patch there are two call traces, two fixes tags and two code changes. Please split this patch into two patches with each one call trace, one Fixes: tag and one code change. Additionally, please include
a changelog when posting a second or later version.

drivers/ufs/core/ufs-

Quoted 1 line

drivers/ufs/core/ufs- renamed from drivers/ufs/core/ufs-mcq.c +0 -0

mcq.c

drivers/ufs/core/ufs-mcq.c int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba,

Quoted 4 lines

index 8944548c30fa..3b2e5bcb08a7 100644

drivers/ufs/core/ufs-mcq.c +0 -0

@@ -512,8 +512,9 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba,

int task_tag)

drivers/ufs/core/ufs-mcq.c int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag) · line 520

Quoted 11 lines

drivers/ufs/core/ufs-mcq.c +3 -2

  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.

Thanks,

Bart.

Quoted 22 lines · Bart Van Assche

drivers/ufs/core/ufs-

Quoted 1 line · On 6/24/24 5:11 AM, [email protected] wrote:

drivers/ufs/core/ufs- renamed from drivers/ufs/core/ufs-mcq.c +0 -0

mcq.c

drivers/ufs/core/ufs-mcq.c int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba,

Quoted 4 lines

index 8944548c30fa..3b2e5bcb08a7 100644

drivers/ufs/core/ufs-mcq.c +0 -0

@@ -512,8 +512,9 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba,

int task_tag)

drivers/ufs/core/ufs-mcq.c int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag) · line 520

Quoted 11 lines

drivers/ufs/core/ufs-mcq.c +3 -2

   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

Quoted 8 lines

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.

Sorry I have not explain root-cause clearly. I will add more clear root-cause analyze next version.

And it is not an invalid pointer is passed to blk_mq_unique_tag(), I means blk_mq_unique_tag function try access null pointer. It is differnt and cause misunderstanding.

The null pinter blk_mq_unique_tag try access is:
rq->mq_hctx(NULL)->queue_num.

The racing flow is:

Thread A
ufshcd_err_handler step 1

	ufshcd_cmd_inflight(true)			step 3
	ufshcd_mcq_req_to_hwq
		blk_mq_unique_tag
			rq->mq_hctx->queue_num		step 5

Thread B
ufs_mtk_mcq_intr(cq complete ISR) step 2

	scsi_done						
		...
		__blk_mq_free_request
			rq->mq_hctx = NULL;		step 4

Quoted 19 lines · Peter Wang (王信友)

Sorry I have not explain root-cause clearly. I will add more clear root-cause analyze next version.

And it is not an invalid pointer is passed to blk_mq_unique_tag(), I means blk_mq_unique_tag function try access null pointer. It is differnt and cause misunderstanding.

The null pinter blk_mq_unique_tag try access is:
rq->mq_hctx(NULL)->queue_num.

The racing flow is:

Thread A
ufshcd_err_handler                    step 1
    ufshcd_cmd_inflight(true)            step 3
    ufshcd_mcq_req_to_hwq
        blk_mq_unique_tag
            rq->mq_hctx->queue_num        step 5

Quoted 6 lines

Thread B
ufs_mtk_mcq_intr(cq complete ISR)            step 2 scsi_done
        ...
        __blk_mq_free_request
            rq->mq_hctx = NULL;        step 4

How about surrounding the blk_mq_unique_tag() call with atomic_inc_not_zero(&req->ref) / atomic_dec(&req->ref)?

Thanks,

Bart.

Hi Bart,

A small wonder, then should we append __blk_mq_free_request() if req->ref decreased to 0 like following?

        if (req_ref_put_and_test(rq))
                __blk_mq_free_request(rq);

Quoted 4 lines

Thanks,

Bart.

This is a chicken-and-egg problem. We need to acquire a lock to know which hwq it is, but we need to know which hwq it is to acquire the lock. Therefore, to resolve this dilemma, perhaps we should just take
all the hwq locks indiscriminately?

  } else {
  hwq = hba->dev_cmd_queue;
  }

Quoted 22 lines · Bart Van Assche

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().

Quoted 4 lines

Thanks,

Bart.

Hi Bart,

Sorry I have not explain root-cause clearly. I will add more clear root-cause analyze next version.

And it is not an invalid pointer is passed to blk_mq_unique_tag(), I means blk_mq_unique_tag function try access null pointer. It is differnt and cause misunderstanding.

The null pinter blk_mq_unique_tag try access is:
rq->mq_hctx(NULL)->queue_num.

The racing flow is:

Thread A
ufshcd_err_handler step 1

	ufshcd_cmd_inflight(true)			step 3
	ufshcd_mcq_req_to_hwq
		blk_mq_unique_tag
			rq->mq_hctx->queue_num		step 5

Thread B
ufs_mtk_mcq_intr(cq complete ISR) step 2

	scsi_done						
		...
		__blk_mq_free_request
			rq->mq_hctx = NULL;		step 4

Thanks.
Peter

Hi Peter,

What is queue_num's offset of blk_mq_hw_ctx in your machine?

gdb vmlinux

(gdb) print /x (int)&((struct blk_mq_hw_ctx *)0)->queue_num $5 = 0x164

I read your descriptions and wondered a same race flow as you described
following. But I found the offset mismatch, if the racing flow is correct,
then the address accessed in blk_mq_unique_tag() should be 0x164, not 0x194.
Maybe the offset is different between our machine?

What's more, if the racing flow is correct, I did not get how your changes
can address this racing flow.

Quoted 18 lines · Wenchao Hao

Hi Peter,

What is queue_num's offset of blk_mq_hw_ctx in your machine?

gdb vmlinux

(gdb) print /x (int)&((struct blk_mq_hw_ctx *)0)->queue_num $5 = 0x164

I read your descriptions and wondered a same race flow as you described
following. But I found the offset mismatch, if the racing flow is correct,
then the address accessed in blk_mq_unique_tag() should be 0x164, not 0x194.
Maybe the offset is different between our machine?

Quoted 5 lines

What's more, if the racing flow is correct, I did not get how your changes
can address this racing flow.

Hi Wenchao Hao,

Yes, our queue_num's offset of blk_mq_hw_ctx is 0x194. Our kernel version is: Linux version 6.1.68
I think the offset is different by kernel version.

(gdb) print /x (int)&((struct blk_mq_hw_ctx *)0)->queue_num $1 = 0x194

And yes, it only shorten the race window of step3 and step5. Reduce the probability of step 4 appearing between step 3 and step 5.

Thanks.
Peter