[bug report] RDMA/erdma: Add verbs implementation

[bug report] RDMA/erdma: Add verbs implementation

[Dan Carpenter]

Hello Cheng Xu,

The patch c4612e83c14b: "RDMA/erdma: Add verbs implementation" from
May 23, 2022, leads to the following Smatch static checker warning:

	drivers/infiniband/hw/erdma/erdma_verbs.c:111 create_qp_cmd()
	error: uninitialized symbol 'resp0'.

drivers/infiniband/hw/erdma/erdma_verbs.c
    100                 req.sq_buf_addr = user_qp->sq_mtt.mtt_entry[0];
    101                 req.rq_buf_addr = user_qp->rq_mtt.mtt_entry[0];
    102 
    103                 req.sq_db_info_dma_addr = user_qp->sq_db_info_dma_addr;
    104                 req.rq_db_info_dma_addr = user_qp->rq_db_info_dma_addr;
    105         }
    106 
    107         err = erdma_post_cmd_wait(&dev->cmdq, (u64 *)&req, sizeof(req), &resp0,
    108                                   &resp1);

The erdma_post_cmd_wait() function does not initialize erdma_post_cmd_wait()
on the error paths.

Also it returns comp_wait->comp_status which is a u8.  I guess that's
some kind of custom error code.  Mixing kernel and custom error codes
is dangerous.  I think it's a bug in this case.

    109 
    110         qp->attrs.cookie =
--> 111                 FIELD_GET(ERDMA_CMDQ_CREATE_QP_RESP_COOKIE_MASK, resp0);
    112 
    113         return err;
    114 }

regards,
dan carpenter

Re: [bug report] RDMA/erdma: Add verbs implementation

[Cheng Xu]

On 6/8/22 8:56 PM, Dan Carpenter wrote:
> Hello Cheng Xu,
> 
> The patch c4612e83c14b: "RDMA/erdma: Add verbs implementation" from
> May 23, 2022, leads to the following Smatch static checker warning:
> 
> 	drivers/infiniband/hw/erdma/erdma_verbs.c:111 create_qp_cmd()
> 	error: uninitialized symbol 'resp0'.
> 
> drivers/infiniband/hw/erdma/erdma_verbs.c
>      100                 req.sq_buf_addr = user_qp->sq_mtt.mtt_entry[0];
>      101                 req.rq_buf_addr = user_qp->rq_mtt.mtt_entry[0];
>      102
>      103                 req.sq_db_info_dma_addr = user_qp->sq_db_info_dma_addr;
>      104                 req.rq_db_info_dma_addr = user_qp->rq_db_info_dma_addr;
>      105         }
>      106
>      107         err = erdma_post_cmd_wait(&dev->cmdq, (u64 *)&req, sizeof(req), &resp0,
>      108                                   &resp1);
> 
> The erdma_post_cmd_wait() function does not initialize erdma_post_cmd_wait()
> on the error paths.

Oh, I knew this before: since erdma_post_cmd_wait returns error, the qp
's resource will destroy soon, the uninitialized value of resp0 will
influence nothing. So here I reduce a condition judgement.

Anyway, I will fix it to eliminate the static checker warning.


> Also it returns comp_wait->comp_status which is a u8.  I guess that's
> some kind of custom error code.  Mixing kernel and custom error codes
> is dangerous.  I think it's a bug in this case.
> 

I will fix it in the next version of our patches.

Thanks,
Cheng Xu

Re: [bug report] RDMA/erdma: Add verbs implementation

[Dan Carpenter]

On Thu, Jun 09, 2022 at 10:32:18AM +0800, Cheng Xu wrote:
> 
> 
> On 6/8/22 8:56 PM, Dan Carpenter wrote:
> > Hello Cheng Xu,
> > 
> > The patch c4612e83c14b: "RDMA/erdma: Add verbs implementation" from
> > May 23, 2022, leads to the following Smatch static checker warning:
> > 
> > 	drivers/infiniband/hw/erdma/erdma_verbs.c:111 create_qp_cmd()
> > 	error: uninitialized symbol 'resp0'.
> > 
> > drivers/infiniband/hw/erdma/erdma_verbs.c
> >      100                 req.sq_buf_addr = user_qp->sq_mtt.mtt_entry[0];
> >      101                 req.rq_buf_addr = user_qp->rq_mtt.mtt_entry[0];
> >      102
> >      103                 req.sq_db_info_dma_addr = user_qp->sq_db_info_dma_addr;
> >      104                 req.rq_db_info_dma_addr = user_qp->rq_db_info_dma_addr;
> >      105         }
> >      106
> >      107         err = erdma_post_cmd_wait(&dev->cmdq, (u64 *)&req, sizeof(req), &resp0,
> >      108                                   &resp1);
> > 
> > The erdma_post_cmd_wait() function does not initialize erdma_post_cmd_wait()
> > on the error paths.
> 
> Oh, I knew this before: since erdma_post_cmd_wait returns error, the qp
> 's resource will destroy soon, the uninitialized value of resp0 will
> influence nothing. So here I reduce a condition judgement.
> 
> Anyway, I will fix it to eliminate the static checker warning.

Thanks!

I am pretty sure it will trigger a KMEMsan warning at runtime once
someone upgrae sysbot to test RDMA.

Another idea would be to initialize resp0 at the top of the function.
Soon the compiler will start initializing to zero anyway so doing it
manually is a zero cost approach.

regards,
dan carpenter

Re: [bug report] RDMA/erdma: Add verbs implementation

[Cheng Xu]

On 9/6/23 7:27 PM, Dan Carpenter wrote:
> Hello Cheng Xu,
> 
> The patch 155055771704: "RDMA/erdma: Add verbs implementation" from
> Jul 27, 2022 (linux-next), leads to the following Smatch static
> checker warning:
> 
> 	drivers/infiniband/hw/erdma/erdma_verbs.c:1044 erdma_get_dma_mr()
> 	error: potential zalloc NULL dereference: 'mr->mem.mtt'
> 

<...>

Hi, Dan,

I can reproduce it, it was introduced by commit:

 7244b4aa4221 ("RDMA/erdma: Refactor the storage structure of MTT entries")

Also I submited a patch to fix it just now.

Thanks very much.

Cheng Xu

[bug report] RDMA/erdma: Add verbs implementation

[Dan Carpenter]

Hello Cheng Xu,

The patch 155055771704: "RDMA/erdma: Add verbs implementation" from
Jul 27, 2022 (linux-next), leads to the following Smatch static
checker warning:

	drivers/infiniband/hw/erdma/erdma_verbs.c:1044 erdma_get_dma_mr()
	error: potential zalloc NULL dereference: 'mr->mem.mtt'

drivers/infiniband/hw/erdma/erdma_verbs.c
    1023 struct ib_mr *erdma_get_dma_mr(struct ib_pd *ibpd, int acc)
    1024 {
    1025         struct erdma_dev *dev = to_edev(ibpd->device);
    1026         struct erdma_mr *mr;
    1027         u32 stag;
    1028         int ret;
    1029 
    1030         mr = kzalloc(sizeof(*mr), GFP_KERNEL);
    1031         if (!mr)
    1032                 return ERR_PTR(-ENOMEM);
    1033 
    1034         ret = erdma_create_stag(dev, &stag);
    1035         if (ret)
    1036                 goto out_free;
    1037 
    1038         mr->type = ERDMA_MR_TYPE_DMA;
    1039 
    1040         mr->ibmr.lkey = stag;
    1041         mr->ibmr.rkey = stag;
    1042         mr->ibmr.pd = ibpd;
    1043         mr->access = ERDMA_MR_ACC_LR | to_erdma_access_flags(acc);
--> 1044         ret = regmr_cmd(dev, mr);

The "mr->mem.mtt" pointer is NULL here so regmr_cmd() will crash.  There
are three callers and the other two are correct.

    1045         if (ret)
    1046                 goto out_remove_stag;
    1047 
    1048         return &mr->ibmr;
    1049 
    1050 out_remove_stag:
    1051         erdma_free_idx(&dev->res_cb[ERDMA_RES_TYPE_STAG_IDX],
    1052                        mr->ibmr.lkey >> 8);
    1053 
    1054 out_free:
    1055         kfree(mr);
    1056 
    1057         return ERR_PTR(ret);
    1058 }

regards,
dan carpenter