[PATCH] rdma_rxe: Prevent access to wr->next ptr afrer wr is posted to send queue

[PATCH] rdma_rxe: Prevent access to wr->next ptr afrer wr is posted to send queue

[m.malygin]

From: Mikhail Malygin <m.malygin@yadro.com>

rxe_post_send_kernel() iterates over linked list of wr's, until the wr->next ptr is NULL.
However it we've got an interrupt after last wr is posted, control may be returned
to the code after send completion callback is executed and wr memory is freed.
As a result, wr->next pointer may contain incorrect value leading to panic.

Signed-off-by: Mikhail Malygin <m.malygin@yadro.com>
Signed-off-by: Sergey Kojushev <s.kojushev@yadro.com>
---
 drivers/infiniband/sw/rxe/rxe_verbs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index b8a22af724e8..a539b11b4f9b 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
@@ -684,6 +684,7 @@ static int rxe_post_send_kernel(struct rxe_qp *qp, const struct ib_send_wr *wr,
 	unsigned int mask;
 	unsigned int length = 0;
 	int i;
+	struct ib_send_wr *next;
 
 	while (wr) {
 		mask = wr_opcode_mask(wr->opcode, qp);
@@ -700,6 +701,8 @@ static int rxe_post_send_kernel(struct rxe_qp *qp, const struct ib_send_wr *wr,
 			break;
 		}
 
+		next = READ_ONCE(wr->next);
+
 		length = 0;
 		for (i = 0; i < wr->num_sge; i++)
 			length += wr->sg_list[i].length;
@@ -710,7 +713,7 @@ static int rxe_post_send_kernel(struct rxe_qp *qp, const struct ib_send_wr *wr,
 			*bad_wr = wr;
 			break;
 		}
-		wr = wr->next;
+		wr = next;
 	}
 
 	rxe_run_task(&qp->req.task, 1);
-- 
2.21.0

Re: [PATCH] rdma_rxe: Prevent access to wr->next ptr afrer wr is posted to send queue

[Jason Gunthorpe]

On Tue, Jun 09, 2020 at 03:54:12PM +0300, m.malygin@yadro.com wrote:
> From: Mikhail Malygin <m.malygin@yadro.com>
> 
> rxe_post_send_kernel() iterates over linked list of wr's, until the wr->next ptr is NULL.
> However it we've got an interrupt after last wr is posted, control may be returned
> to the code after send completion callback is executed and wr memory is freed.
> As a result, wr->next pointer may contain incorrect value leading to panic.
> 
> Signed-off-by: Mikhail Malygin <m.malygin@yadro.com>
> Signed-off-by: Sergey Kojushev <s.kojushev@yadro.com>
>  drivers/infiniband/sw/rxe/rxe_verbs.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
> index b8a22af724e8..a539b11b4f9b 100644
> +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
> @@ -684,6 +684,7 @@ static int rxe_post_send_kernel(struct rxe_qp *qp, const struct ib_send_wr *wr,
>  	unsigned int mask;
>  	unsigned int length = 0;
>  	int i;
> +	struct ib_send_wr *next;
>  
>  	while (wr) {
>  		mask = wr_opcode_mask(wr->opcode, qp);
> @@ -700,6 +701,8 @@ static int rxe_post_send_kernel(struct rxe_qp *qp, const struct ib_send_wr *wr,
>  			break;
>  		}
>  
> +		next = READ_ONCE(wr->next);

Why is this READ_ONCE? The wr list at this point cannot be allowed to
change

Jason

[PATCH v2] rdma_rxe: Prevent access to wr->next ptr afrer wr is posted to send queue

[m.malygin]

From: Mikhail Malygin <m.malygin@yadro.com>

rxe_post_send_kernel() iterates over linked list of wr's, until the wr->next ptr is NULL.
However it we've got an interrupt after last wr is posted, control may be returned
to the code after send completion callback is executed and wr memory is freed.
As a result, wr->next pointer may contain incorrect value leading to panic.

Signed-off-by: Mikhail Malygin <m.malygin@yadro.com>
Signed-off-by: Sergey Kojushev <s.kojushev@yadro.com>
---
 drivers/infiniband/sw/rxe/rxe_verbs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index b8a22af724e8..84fec5fd798d 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
@@ -684,6 +684,7 @@ static int rxe_post_send_kernel(struct rxe_qp *qp, const struct ib_send_wr *wr,
 	unsigned int mask;
 	unsigned int length = 0;
 	int i;
+	struct ib_send_wr *next;
 
 	while (wr) {
 		mask = wr_opcode_mask(wr->opcode, qp);
@@ -700,6 +701,8 @@ static int rxe_post_send_kernel(struct rxe_qp *qp, const struct ib_send_wr *wr,
 			break;
 		}
 
+		next = wr->next;
+
 		length = 0;
 		for (i = 0; i < wr->num_sge; i++)
 			length += wr->sg_list[i].length;
@@ -710,7 +713,7 @@ static int rxe_post_send_kernel(struct rxe_qp *qp, const struct ib_send_wr *wr,
 			*bad_wr = wr;
 			break;
 		}
-		wr = wr->next;
+		wr = next;
 	}
 
 	rxe_run_task(&qp->req.task, 1);
-- 
2.26.2

Re: [PATCH v2] rdma_rxe: Prevent access to wr->next ptr afrer wr is posted to send queue

[Jason Gunthorpe]

On Thu, Jul 16, 2020 at 10:03:41PM +0300, m.malygin@yadro.com wrote:
> From: Mikhail Malygin <m.malygin@yadro.com>
> 
> rxe_post_send_kernel() iterates over linked list of wr's, until the wr->next ptr is NULL.
> However it we've got an interrupt after last wr is posted, control may be returned
> to the code after send completion callback is executed and wr memory is freed.
> As a result, wr->next pointer may contain incorrect value leading to panic.
> 
> Signed-off-by: Mikhail Malygin <m.malygin@yadro.com>
> Signed-off-by: Sergey Kojushev <s.kojushev@yadro.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_verbs.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Applied to for-next, thanks

Jason

Re: [PATCH] rdma_rxe: Prevent access to wr->next ptr afrer wr is posted to send queue

[Mikhail Malygin]

> Why is this READ_ONCE? The wr list at this point cannot be allowed to
> change
> 
> Jason

The idea behind this READ_ONCE was to make sure read of wr->next happens before post_send_one(), as there is no data dependency between post_send_one and read of wr->next. However I’m not 100% sure this is necessary here.

Mikhail

Re: [PATCH] rdma_rxe: Prevent access to wr->next ptr afrer wr is posted to send queue

[Jason Gunthorpe]

On Wed, Jul 15, 2020 at 08:57:40AM +0000, Mikhail Malygin wrote:
> 
> > Why is this READ_ONCE? The wr list at this point cannot be allowed to
> > change
> > 
> > Jason
> 
> The idea behind this READ_ONCE was to make sure read of wr->next
> happens before post_send_one(), as there is no data dependency
> between post_send_one and read of wr->next. However I’m not 100%
> sure this is necessary here.

The spinock in post_one_send() guarantees no reordering across
post_one_send()

Jason

Re: [PATCH] rdma_rxe: Prevent access to wr->next ptr afrer wr is posted to send queue

[Mikhail Malygin]

Thanks, I’ll post an updated version.

Mikhail

> On 15 Jul 2020, at 14:37, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> The spinock in post_one_send() guarantees no reordering across
> post_one_send()
> 
> Jason

Re: [PATCH] rdma_rxe: Prevent access to wr->next ptr afrer wr is posted to send queue

[Zhu Yanjun]

On Wed, Jul 15, 2020 at 8:39 PM Mikhail Malygin <m.malygin@yadro.com> wrote:
>
> Thanks, I’ll post an updated version.
>
> Mikhail
>
> > On 15 Jul 2020, at 14:37, Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > The spinock in post_one_send() guarantees no reordering across

How about spin_lock_irqsave?

Zhu Yanjun

> > post_one_send()
> >
> > Jason
>

Re: [PATCH] rdma_rxe: Prevent access to wr->next ptr afrer wr is posted to send queue

[Jason Gunthorpe]

On Thu, Jul 16, 2020 at 11:17:09AM +0800, Zhu Yanjun wrote:
> On Wed, Jul 15, 2020 at 8:39 PM Mikhail Malygin <m.malygin@yadro.com> wrote:
> >
> > Thanks, I’ll post an updated version.
> >
> > Mikhail
> >
> > > On 15 Jul 2020, at 14:37, Jason Gunthorpe <jgg@nvidia.com> wrote:
> > >
> > > The spinock in post_one_send() guarantees no reordering across
> 
> How about spin_lock_irqsave?

The unlock of any lock prevents store reordering by definition

Jason