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

[Zhu Yanjun <yanjun.zhu@linux.dev>]

在 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@gmail.com> 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@gmail.com>
> 
> 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@gmail.com>
>>> ---
>>>   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
>>>
>