| Message ID | 1702913550-20631-1-git-send-email-quic_cang@quicinc.com |
|---|---|
| State | Accepted |
| Commit | 04c116e2bdfc3969f9819d2cebfdf678353c354c |
| Headers | show |
| Series | scsi: ufs: core: Let the sq_lock protect sq_tail_slot access | expand |
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>
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 >
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.
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. >
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>
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);
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>