* Re: [PATCH] scsi: ufs: core: Let the sq_lock protect sq_tail_slot access
2023-12-18 15:32 [PATCH] scsi: ufs: core: Let the sq_lock protect sq_tail_slot access Can Guo
@ 2023-12-18 21:41 ` Bart Van Assche
2023-12-20 14:50 ` Manivannan Sadhasivam
2024-01-02 20:33 ` Bao D. Nguyen
2 siblings, 0 replies; 6+ messages in thread
From: Bart Van Assche @ 2023-12-18 21:41 UTC (permalink / raw)
To: Can Guo, mani, adrian.hunter, beanhuo, avri.altman, junwoo80.lee,
martin.petersen
Cc: linux-scsi, linux-arm-msm, Alim Akhtar, James E.J. Bottomley,
Stanley Chu, Asutosh Das, Peter Wang, Bao D. Nguyen,
Arthur Simchaev, open list
On 12/18/23 07:32, Can Guo wrote:
> If access sq_tail_slot without the protection from the sq_lock, race
> condition can have multiple SQEs copied to duplicate SQE slot(s), which can
> lead to multiple incredible stability issues. Fix it by moving the *dest
> initialization, in ufshcd_send_command(), back under protection from the
> sq_lock.
>
> Fixes: 3c85f087faec ("scsi: ufs: mcq: Use pointer arithmetic in ufshcd_send_command()")
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ae9936f..2994aac 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2274,9 +2274,10 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag,
> if (is_mcq_enabled(hba)) {
> int utrd_size = sizeof(struct utp_transfer_req_desc);
> struct utp_transfer_req_desc *src = lrbp->utr_descriptor_ptr;
> - struct utp_transfer_req_desc *dest = hwq->sqe_base_addr + hwq->sq_tail_slot;
> + struct utp_transfer_req_desc *dest;
>
> spin_lock(&hwq->sq_lock);
> + dest = hwq->sqe_base_addr + hwq->sq_tail_slot;
> memcpy(dest, src, utrd_size);
> ufshcd_inc_sq_tail(hwq);
> spin_unlock(&hwq->sq_lock);
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: ufs: core: Let the sq_lock protect sq_tail_slot access
2023-12-18 15:32 [PATCH] scsi: ufs: core: Let the sq_lock protect sq_tail_slot access Can Guo
2023-12-18 21:41 ` Bart Van Assche
@ 2023-12-20 14:50 ` Manivannan Sadhasivam
2023-12-20 16:35 ` Bart Van Assche
2024-01-02 20:33 ` Bao D. Nguyen
2 siblings, 1 reply; 6+ messages in thread
From: Manivannan Sadhasivam @ 2023-12-20 14:50 UTC (permalink / raw)
To: Can Guo
Cc: bvanassche, adrian.hunter, beanhuo, avri.altman, junwoo80.lee,
martin.petersen, linux-scsi, linux-arm-msm, Alim Akhtar,
James E.J. Bottomley, Stanley Chu, Asutosh Das, Peter Wang,
Bao D. Nguyen, Arthur Simchaev, open list
On Mon, Dec 18, 2023 at 07:32:17AM -0800, Can Guo wrote:
> If access sq_tail_slot without the protection from the sq_lock, race
> condition can have multiple SQEs copied to duplicate SQE slot(s), which can
> lead to multiple incredible stability issues. Fix it by moving the *dest
> initialization, in ufshcd_send_command(), back under protection from the
> sq_lock.
>
> Fixes: 3c85f087faec ("scsi: ufs: mcq: Use pointer arithmetic in ufshcd_send_command()")
Cc: stable@vger.kernel.org
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
- Mani
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ae9936f..2994aac 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2274,9 +2274,10 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag,
> if (is_mcq_enabled(hba)) {
> int utrd_size = sizeof(struct utp_transfer_req_desc);
> struct utp_transfer_req_desc *src = lrbp->utr_descriptor_ptr;
> - struct utp_transfer_req_desc *dest = hwq->sqe_base_addr + hwq->sq_tail_slot;
> + struct utp_transfer_req_desc *dest;
>
> spin_lock(&hwq->sq_lock);
> + dest = hwq->sqe_base_addr + hwq->sq_tail_slot;
> memcpy(dest, src, utrd_size);
> ufshcd_inc_sq_tail(hwq);
> spin_unlock(&hwq->sq_lock);
> --
> 2.7.4
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: ufs: core: Let the sq_lock protect sq_tail_slot access
2023-12-20 14:50 ` Manivannan Sadhasivam
@ 2023-12-20 16:35 ` Bart Van Assche
2023-12-20 17:00 ` Manivannan Sadhasivam
0 siblings, 1 reply; 6+ messages in thread
From: Bart Van Assche @ 2023-12-20 16:35 UTC (permalink / raw)
To: Manivannan Sadhasivam, Can Guo
Cc: adrian.hunter, beanhuo, avri.altman, junwoo80.lee,
martin.petersen, linux-scsi, linux-arm-msm, Alim Akhtar,
James E.J. Bottomley, Stanley Chu, Asutosh Das, Peter Wang,
Bao D. Nguyen, Arthur Simchaev, open list
On 12/20/23 06:50, Manivannan Sadhasivam wrote:
> On Mon, Dec 18, 2023 at 07:32:17AM -0800, Can Guo wrote:
>> If access sq_tail_slot without the protection from the sq_lock, race
>> condition can have multiple SQEs copied to duplicate SQE slot(s), which can
>> lead to multiple incredible stability issues. Fix it by moving the *dest
>> initialization, in ufshcd_send_command(), back under protection from the
>> sq_lock.
>>
>> Fixes: 3c85f087faec ("scsi: ufs: mcq: Use pointer arithmetic in ufshcd_send_command()")
>
> Cc: stable@vger.kernel.org
Hmm ... is the "Cc: stable" tag really required if a "Fixes:" tag is present?
Bart.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: ufs: core: Let the sq_lock protect sq_tail_slot access
2023-12-20 16:35 ` Bart Van Assche
@ 2023-12-20 17:00 ` Manivannan Sadhasivam
0 siblings, 0 replies; 6+ messages in thread
From: Manivannan Sadhasivam @ 2023-12-20 17:00 UTC (permalink / raw)
To: Bart Van Assche
Cc: Can Guo, adrian.hunter, beanhuo, avri.altman, junwoo80.lee,
martin.petersen, linux-scsi, linux-arm-msm, Alim Akhtar,
James E.J. Bottomley, Stanley Chu, Asutosh Das, Peter Wang,
Bao D. Nguyen, Arthur Simchaev, open list
On Wed, Dec 20, 2023 at 08:35:18AM -0800, Bart Van Assche wrote:
> On 12/20/23 06:50, Manivannan Sadhasivam wrote:
> > On Mon, Dec 18, 2023 at 07:32:17AM -0800, Can Guo wrote:
> > > If access sq_tail_slot without the protection from the sq_lock, race
> > > condition can have multiple SQEs copied to duplicate SQE slot(s), which can
> > > lead to multiple incredible stability issues. Fix it by moving the *dest
> > > initialization, in ufshcd_send_command(), back under protection from the
> > > sq_lock.
> > >
> > > Fixes: 3c85f087faec ("scsi: ufs: mcq: Use pointer arithmetic in ufshcd_send_command()")
> >
> > Cc: stable@vger.kernel.org
>
> Hmm ... is the "Cc: stable" tag really required if a "Fixes:" tag is present?
>
Yes it is required as I pointed out in the other thread.
- Mani
> Bart.
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: ufs: core: Let the sq_lock protect sq_tail_slot access
2023-12-18 15:32 [PATCH] scsi: ufs: core: Let the sq_lock protect sq_tail_slot access Can Guo
2023-12-18 21:41 ` Bart Van Assche
2023-12-20 14:50 ` Manivannan Sadhasivam
@ 2024-01-02 20:33 ` Bao D. Nguyen
2 siblings, 0 replies; 6+ messages in thread
From: Bao D. Nguyen @ 2024-01-02 20:33 UTC (permalink / raw)
To: Can Guo, bvanassche, mani, adrian.hunter, beanhuo, avri.altman,
junwoo80.lee, martin.petersen
Cc: linux-scsi, linux-arm-msm, Alim Akhtar, James E.J. Bottomley,
Stanley Chu, Asutosh Das, Peter Wang, Arthur Simchaev, open list
On 12/18/2023 7:32 AM, Can Guo wrote:
> If access sq_tail_slot without the protection from the sq_lock, race
> condition can have multiple SQEs copied to duplicate SQE slot(s), which can
> lead to multiple incredible stability issues. Fix it by moving the *dest
> initialization, in ufshcd_send_command(), back under protection from the
> sq_lock.
>
> Fixes: 3c85f087faec ("scsi: ufs: mcq: Use pointer arithmetic in ufshcd_send_command()")
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ae9936f..2994aac 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2274,9 +2274,10 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag,
> if (is_mcq_enabled(hba)) {
> int utrd_size = sizeof(struct utp_transfer_req_desc);
> struct utp_transfer_req_desc *src = lrbp->utr_descriptor_ptr;
> - struct utp_transfer_req_desc *dest = hwq->sqe_base_addr + hwq->sq_tail_slot;
> + struct utp_transfer_req_desc *dest;
>
> spin_lock(&hwq->sq_lock);
> + dest = hwq->sqe_base_addr + hwq->sq_tail_slot;
> memcpy(dest, src, utrd_size);
> ufshcd_inc_sq_tail(hwq);
> spin_unlock(&hwq->sq_lock);
Reviewed and Tested-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
^ permalink raw reply [flat|nested] 6+ messages in thread