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 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
>>
