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

[Peter Wang (王信友)]

On Mon, 2024-06-24 at 11:01 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 6/24/24 5:11 AM, peter.wang at mediatek.com 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.
> 

Hi Bart,

Will split this patch into two patches and add changelog next version,
thanks.

> > 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.
> 
> Thanks,
> 
> Bart.
> 

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?

Thanks.
Peter