[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