Re: [bug report] RDMA/qedr: Add doorbell overflow recovery support

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Nov 06, 2019 at 10:52:59AM +0300, Dan Carpenter wrote:
> Hello Michal Kalderon,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch 9bcc6597f47b: "RDMA/qedr: Add doorbell overflow recovery 
> support" from Oct 30, 2019, leads to the following Smatch complaint:
> 
>     drivers/infiniband/hw/qedr/verbs.c:1046 qedr_create_cq()
>     warn: variable dereferenced before check 'ctx' (see line 970)
> 
> drivers/infiniband/hw/qedr/verbs.c
>    905  int qedr_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
>    906                     struct ib_udata *udata)
>    907  {
>    908          struct ib_device *ibdev = ibcq->device;
>    909          struct qedr_ucontext *ctx = rdma_udata_to_drv_context(
>                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>    910                  udata, struct qedr_ucontext, ibucontext);
>    911          struct qed_rdma_destroy_cq_out_params destroy_oparams;
>    912          struct qed_rdma_destroy_cq_in_params destroy_iparams;
>    913          struct qedr_dev *dev = get_qedr_dev(ibdev);
>    914          struct qed_rdma_create_cq_in_params params;
>    915          struct qedr_create_cq_ureq ureq = {};
>    916          int vector = attr->comp_vector;
>    917          int entries = attr->cqe;
>    918          struct qedr_cq *cq = get_qedr_cq(ibcq);
>    919          int chain_entries;
>    920          u32 db_offset;
>    921          int page_cnt;
>    922          u64 pbl_ptr;
>    923          u16 icid;
>    924          int rc;
>    925  
>    926          DP_DEBUG(dev, QEDR_MSG_INIT,
>    927                   "create_cq: called from %s. entries=%d, vector=%d\n",
>    928                   udata ? "User Lib" : "Kernel", entries, vector);
>    929  
>    930          if (entries > QEDR_MAX_CQES) {
>    931                  DP_ERR(dev,
>    932                         "create cq: the number of entries %d is too high. Must be equal or below %d.\n",
>    933                         entries, QEDR_MAX_CQES);
>    934                  return -EINVAL;
>    935          }
>    936  
>    937          chain_entries = qedr_align_cq_entries(entries);
>    938          chain_entries = min_t(int, chain_entries, QEDR_MAX_CQES);
>    939  
>    940          /* calc db offset. user will add DPI base, kernel will add db addr */
>    941          db_offset = DB_ADDR_SHIFT(DQ_PWM_OFFSET_UCM_RDMA_CQ_CONS_32BIT);
>    942  
>    943          if (udata) {
>    944                  if (ib_copy_from_udata(&ureq, udata, min(sizeof(ureq),
>    945                                                           udata->inlen))) {
>    946                          DP_ERR(dev,
>    947                                 "create cq: problem copying data from user space\n");
>    948                          goto err0;
>    949                  }
>    950  
>    951                  if (!ureq.len) {
>    952                          DP_ERR(dev,
>    953                                 "create cq: cannot create a cq with 0 entries\n");
>    954                          goto err0;
>    955                  }
>    956  
>    957                  cq->cq_type = QEDR_CQ_TYPE_USER;
>    958  
>    959                  rc = qedr_init_user_queue(udata, dev, &cq->q, ureq.addr,
>    960                                            ureq.len, true,
>    961                                            IB_ACCESS_LOCAL_WRITE,
>    962                                            1, 1);
>    963                  if (rc)
>    964                          goto err0;
>    965  
>    966                  pbl_ptr = cq->q.pbl_tbl->pa;
>    967                  page_cnt = cq->q.pbl_info.num_pbes;
>    968  
>    969			cq->ibcq.cqe = chain_entries;
>    970			cq->q.db_addr = ctx->dpi_addr + db_offset;
>                                         ^^^^^^^^^^^^^
> New unchecked dereference.

For rdma_udata_to_drv_context(), udata != NULL implies ctx != NULL

Generally I prefer to see the rdma_udata_to_drv_context() as a local
variable inside an 'if (udata)' but this is one of those places where
that doesn't work out.

Jason