[PATCH v1] ufs: core: fix racing issue between ufshcd_mcq_abort and ISR

[PATCH v1] ufs: core: fix racing issue between ufshcd_mcq_abort and ISR

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

If command timeout happen and cq complete irq raise at the same time,
ufshcd_mcq_abort null the lprb->cmd and NULL poiner KE in ISR.
Below is error log.

ufshcd_abort: Device abort task at tag 18
Unable to handle kernel NULL pointer dereference at virtual address
0000000000000108
pc : [0xffffffe27ef867ac] scsi_dma_unmap+0xc/0x44
lr : [0xffffffe27f1b898c] ufshcd_release_scsi_cmd+0x24/0x114

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufs-mcq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 2ba8ec254dce..6ea96406f2bf 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -630,6 +630,7 @@ int ufshcd_mcq_abort(struct scsi_cmnd *cmd)
 	int tag = scsi_cmd_to_rq(cmd)->tag;
 	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
 	struct ufs_hw_queue *hwq;
+	unsigned long flags;
 	int err = FAILED;
 
 	if (!ufshcd_cmd_inflight(lrbp->cmd)) {
@@ -670,8 +671,10 @@ int ufshcd_mcq_abort(struct scsi_cmnd *cmd)
 	}
 
 	err = SUCCESS;
+	spin_lock_irqsave(&hwq->cq_lock, flags);
 	if (ufshcd_cmd_inflight(lrbp->cmd))
 		ufshcd_release_scsi_cmd(hba, lrbp);
+	spin_unlock_irqrestore(&hwq->cq_lock, flags);
 
 out:
 	return err;
-- 
2.18.0

Re: [PATCH v1] ufs: core: fix racing issue between ufshcd_mcq_abort and ISR

[Peter Wang (王信友)]

Hi all,

Gentle ping for this bug fix review.

Thanks.



On Fri, 2023-10-27 at 16:43 +0800, peter.wang@mediatek.com wrote:
> From: Peter Wang <peter.wang@mediatek.com>
> 
> If command timeout happen and cq complete irq raise at the same time,
> ufshcd_mcq_abort null the lprb->cmd and NULL poiner KE in ISR.
> Below is error log.
> 
> ufshcd_abort: Device abort task at tag 18
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000108
> pc : [0xffffffe27ef867ac] scsi_dma_unmap+0xc/0x44
> lr : [0xffffffe27f1b898c] ufshcd_release_scsi_cmd+0x24/0x114
> 
> Signed-off-by: Peter Wang <peter.wang@mediatek.com>
> ---
>  drivers/ufs/core/ufs-mcq.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 2ba8ec254dce..6ea96406f2bf 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -630,6 +630,7 @@ int ufshcd_mcq_abort(struct scsi_cmnd *cmd)
>  	int tag = scsi_cmd_to_rq(cmd)->tag;
>  	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
>  	struct ufs_hw_queue *hwq;
> +	unsigned long flags;
>  	int err = FAILED;
>  
>  	if (!ufshcd_cmd_inflight(lrbp->cmd)) {
> @@ -670,8 +671,10 @@ int ufshcd_mcq_abort(struct scsi_cmnd *cmd)
>  	}
>  
>  	err = SUCCESS;
> +	spin_lock_irqsave(&hwq->cq_lock, flags);
>  	if (ufshcd_cmd_inflight(lrbp->cmd))
>  		ufshcd_release_scsi_cmd(hba, lrbp);
> +	spin_unlock_irqrestore(&hwq->cq_lock, flags);
>  
>  out:
>  	return err;

Re: [PATCH v1] ufs: core: fix racing issue between ufshcd_mcq_abort and ISR

[Bart Van Assche]

On 10/27/23 01:43, peter.wang@mediatek.com wrote:
> If command timeout happen and cq complete irq raise at the same time,
> ufshcd_mcq_abort null the lprb->cmd and NULL poiner KE in ISR.

Please add a Fixes: tag. Otherwise this patch looks good to me.

Thanks,

Bart.

Re: [PATCH v1] ufs: core: fix racing issue between ufshcd_mcq_abort and ISR

[Peter Wang (王信友)]

On Fri, 2023-11-03 at 07:57 -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 10/27/23 01:43, peter.wang@mediatek.com wrote:
> > If command timeout happen and cq complete irq raise at the same
> time,
> > ufshcd_mcq_abort null the lprb->cmd and NULL poiner KE in ISR.
> 
> Please add a Fixes: tag. Otherwise this patch looks good to me.
> 
> Thanks,
> 
> Bart.

Hi Bart,

Thank you for review, will add fixes tag next version.

Thanks.
Peter