Re: [PATCH for-next v2 1/3] RDMA/rxe: Move work queue code to subroutines

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

在 2023/6/20 23:02, Bob Pearson 写道:

On 6/20/23 09:49, Zhu Yanjun wrote:

On Tue, Jun 20, 2023 at 9:55 PM Bob Pearson <rpearsonhpe@xxxxxxxxx> wrote:


This patch:
         - Moves code to initialize a qp send work queue to a
           subroutine named rxe_init_sq.
         - Moves code to initialize a qp recv work queue to a
           subroutine named rxe_init_rq.


This is a use-before-initialization problem. It is better to
initialize the sq/rq queues before the queues are used.
These 3 commits are complicated. It is easy to introduce some risks
just like in the first version. A compact fix is preferred.
But these commits seems to fix the problem.

Acked-by: Zhu Yanjun <zyjzyj2000@xxxxxxxxx>


The fix to the reported problem is in patch 2/3 which is very simple.
Patch 1/3 mainly just cuts and pastes the code to init the queues into a
subroutine without any functional change. But it fixes another potential
use before setting issue with the packet queues simply by initializing
them first.

I am planning on spending today looking at the namespace patches.


Thanks, Bob

Zhu Yanjun



Thanks for looking - Bob





         - Moves initialization of qp request and response packet
           queues ahead of work queue initialization so that cleanup
           of a qp if it is not fully completed can successfully
           attempt to drain the packet queues without a seg fault.
         - Makes minor whitespace cleanups.

Fixes: 8700e3e7c485 ("Soft RoCE driver")
Signed-off-by: Bob Pearson <rpearsonhpe@xxxxxxxxx>
---
  drivers/infiniband/sw/rxe/rxe_qp.c | 163 +++++++++++++++++++----------
  1 file changed, 108 insertions(+), 55 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
index 95d4a6760c33..9dbb16699c36 100644
--- a/drivers/infiniband/sw/rxe/rxe_qp.c
+++ b/drivers/infiniband/sw/rxe/rxe_qp.c
@@ -180,13 +180,63 @@ static void rxe_qp_init_misc(struct rxe_dev *rxe, struct rxe_qp *qp,
         atomic_set(&qp->skb_out, 0);
  }

+static int rxe_init_sq(struct rxe_qp *qp, struct ib_qp_init_attr *init,
+                      struct ib_udata *udata,
+                      struct rxe_create_qp_resp __user *uresp)
+{
+       struct rxe_dev *rxe = to_rdev(qp->ibqp.device);
+       int wqe_size;
+       int err;
+
+       qp->sq.max_wr = init->cap.max_send_wr;
+       wqe_size = max_t(int, init->cap.max_send_sge * sizeof(struct ib_sge),
+                        init->cap.max_inline_data);
+       qp->sq.max_sge = wqe_size / sizeof(struct ib_sge);
+       qp->sq.max_inline = wqe_size;
+       wqe_size += sizeof(struct rxe_send_wqe);
+
+       qp->sq.queue = rxe_queue_init(rxe, &qp->sq.max_wr, wqe_size,
+                                     QUEUE_TYPE_FROM_CLIENT);
+       if (!qp->sq.queue) {
+               rxe_err_qp(qp, "Unable to allocate send queue");
+               err = -ENOMEM;
+               goto err_out;
+       }
+
+       /* prepare info for caller to mmap send queue if user space qp */
+       err = do_mmap_info(rxe, uresp ? &uresp->sq_mi : NULL, udata,
+                          qp->sq.queue->buf, qp->sq.queue->buf_size,
+                          &qp->sq.queue->ip);
+       if (err) {
+               rxe_err_qp(qp, "do_mmap_info failed, err = %d", err);
+               goto err_free;
+       }
+
+       /* return actual capabilities to caller which may be larger
+        * than requested
+        */
+       init->cap.max_send_wr = qp->sq.max_wr;
+       init->cap.max_send_sge = qp->sq.max_sge;
+       init->cap.max_inline_data = qp->sq.max_inline;
+
+       return 0;
+
+err_free:
+       vfree(qp->sq.queue->buf);
+       kfree(qp->sq.queue);
+       qp->sq.queue = NULL;
+err_out:
+       return err;
+}
+
  static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
                            struct ib_qp_init_attr *init, struct ib_udata *udata,
                            struct rxe_create_qp_resp __user *uresp)
  {
         int err;
-       int wqe_size;
-       enum queue_type type;
+
+       /* if we don't finish qp create make sure queue is valid */
+       skb_queue_head_init(&qp->req_pkts);

         err = sock_create_kern(&init_net, AF_INET, SOCK_DGRAM, 0, &qp->sk);
         if (err < 0)
@@ -201,32 +251,10 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
          * (0xc000 - 0xffff).
          */
         qp->src_port = RXE_ROCE_V2_SPORT + (hash_32(qp_num(qp), 14) & 0x3fff);
-       qp->sq.max_wr           = init->cap.max_send_wr;
-
-       /* These caps are limited by rxe_qp_chk_cap() done by the caller */
-       wqe_size = max_t(int, init->cap.max_send_sge * sizeof(struct ib_sge),
-                        init->cap.max_inline_data);
-       qp->sq.max_sge = init->cap.max_send_sge =
-               wqe_size / sizeof(struct ib_sge);
-       qp->sq.max_inline = init->cap.max_inline_data = wqe_size;
-       wqe_size += sizeof(struct rxe_send_wqe);
-
-       type = QUEUE_TYPE_FROM_CLIENT;
-       qp->sq.queue = rxe_queue_init(rxe, &qp->sq.max_wr,
-                               wqe_size, type);
-       if (!qp->sq.queue)
-               return -ENOMEM;

-       err = do_mmap_info(rxe, uresp ? &uresp->sq_mi : NULL, udata,
-                          qp->sq.queue->buf, qp->sq.queue->buf_size,
-                          &qp->sq.queue->ip);
-
-       if (err) {
-               vfree(qp->sq.queue->buf);
-               kfree(qp->sq.queue);
-               qp->sq.queue = NULL;
+       err = rxe_init_sq(qp, init, udata, uresp);
+       if (err)
                 return err;
-       }

         qp->req.wqe_index = queue_get_producer(qp->sq.queue,
                                                QUEUE_TYPE_FROM_CLIENT);
@@ -234,8 +262,6 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
         qp->req.opcode          = -1;
         qp->comp.opcode         = -1;

-       skb_queue_head_init(&qp->req_pkts);
-
         rxe_init_task(&qp->req.task, qp, rxe_requester);
         rxe_init_task(&qp->comp.task, qp, rxe_completer);

@@ -247,40 +273,67 @@ static int rxe_qp_init_req(struct rxe_dev *rxe, struct rxe_qp *qp,
         return 0;
  }

+static int rxe_init_rq(struct rxe_qp *qp, struct ib_qp_init_attr *init,
+                      struct ib_udata *udata,
+                      struct rxe_create_qp_resp __user *uresp)
+{
+       struct rxe_dev *rxe = to_rdev(qp->ibqp.device);
+       int wqe_size;
+       int err;
+
+       qp->rq.max_wr = init->cap.max_recv_wr;
+       qp->rq.max_sge = init->cap.max_recv_sge;
+       wqe_size = sizeof(struct rxe_recv_wqe) +
+                               qp->rq.max_sge*sizeof(struct ib_sge);
+
+       qp->rq.queue = rxe_queue_init(rxe, &qp->rq.max_wr, wqe_size,
+                                     QUEUE_TYPE_FROM_CLIENT);
+       if (!qp->rq.queue) {
+               rxe_err_qp(qp, "Unable to allocate recv queue");
+               err = -ENOMEM;
+               goto err_out;
+       }
+
+       /* prepare info for caller to mmap recv queue if user space qp */
+       err = do_mmap_info(rxe, uresp ? &uresp->rq_mi : NULL, udata,
+                          qp->rq.queue->buf, qp->rq.queue->buf_size,
+                          &qp->rq.queue->ip);
+       if (err) {
+               rxe_err_qp(qp, "do_mmap_info failed, err = %d", err);
+               goto err_free;
+       }
+
+       /* return actual capabilities to caller which may be larger
+        * than requested
+        */
+       init->cap.max_recv_wr = qp->rq.max_wr;
+
+       return 0;
+
+err_free:
+       vfree(qp->rq.queue->buf);
+       kfree(qp->rq.queue);
+       qp->rq.queue = NULL;
+err_out:
+       return err;
+}
+
  static int rxe_qp_init_resp(struct rxe_dev *rxe, struct rxe_qp *qp,
                             struct ib_qp_init_attr *init,
                             struct ib_udata *udata,
                             struct rxe_create_qp_resp __user *uresp)
  {
         int err;
-       int wqe_size;
-       enum queue_type type;
+
+       /* if we don't finish qp create make sure queue is valid */
+       skb_queue_head_init(&qp->resp_pkts);

         if (!qp->srq) {
-               qp->rq.max_wr           = init->cap.max_recv_wr;
-               qp->rq.max_sge          = init->cap.max_recv_sge;
-
-               wqe_size = rcv_wqe_size(qp->rq.max_sge);
-
-               type = QUEUE_TYPE_FROM_CLIENT;
-               qp->rq.queue = rxe_queue_init(rxe, &qp->rq.max_wr,
-                                       wqe_size, type);
-               if (!qp->rq.queue)
-                       return -ENOMEM;
-
-               err = do_mmap_info(rxe, uresp ? &uresp->rq_mi : NULL, udata,
-                                  qp->rq.queue->buf, qp->rq.queue->buf_size,
-                                  &qp->rq.queue->ip);
-               if (err) {
-                       vfree(qp->rq.queue->buf);
-                       kfree(qp->rq.queue);
-                       qp->rq.queue = NULL;
+               err = rxe_init_rq(qp, init, udata, uresp);
+               if (err)
                         return err;
-               }
         }

-       skb_queue_head_init(&qp->resp_pkts);
-
         rxe_init_task(&qp->resp.task, qp, rxe_responder);

         qp->resp.opcode         = OPCODE_NONE;
@@ -307,10 +360,10 @@ int rxe_qp_from_init(struct rxe_dev *rxe, struct rxe_qp *qp, struct rxe_pd *pd,
         if (srq)
                 rxe_get(srq);

-       qp->pd                  = pd;
-       qp->rcq                 = rcq;
-       qp->scq                 = scq;
-       qp->srq                 = srq;
+       qp->pd = pd;
+       qp->rcq = rcq;
+       qp->scq = scq;
+       qp->srq = srq;

         atomic_inc(&rcq->num_wq);
         atomic_inc(&scq->num_wq);
--
2.39.2
[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux