Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
* [bug report] RDMA/qedr: Add doorbell overflow recovery support
@ 2019-11-06  7:52 Dan Carpenter
  2019-11-06 15:38 ` Jason Gunthorpe
  0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2019-11-06  7:52 UTC (permalink / raw)
  To: michal.kalderon; +Cc: linux-rdma

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.

   971		} else {
   972			cq->cq_type = QEDR_CQ_TYPE_KERNEL;
   973	
   974			rc = dev->ops->common->chain_alloc(dev->cdev,
   975							   QED_CHAIN_USE_TO_CONSUME,
   976							   QED_CHAIN_MODE_PBL,
   977							   QED_CHAIN_CNT_TYPE_U32,
   978							   chain_entries,
   979							   sizeof(union rdma_cqe),
   980							   &cq->pbl, NULL);
   981			if (rc)
   982				goto err0;
   983	
   984			page_cnt = qed_chain_get_page_cnt(&cq->pbl);
   985			pbl_ptr = qed_chain_get_pbl_phys(&cq->pbl);
   986			cq->ibcq.cqe = cq->pbl.capacity;
   987		}
   988	
   989		qedr_init_cq_params(cq, ctx, dev, vector, chain_entries, page_cnt,
   990				    pbl_ptr, &params);
   991	
   992		rc = dev->ops->rdma_create_cq(dev->rdma_ctx, &params, &icid);
   993		if (rc)
   994			goto err1;
   995	
   996		cq->icid = icid;
   997		cq->sig = QEDR_CQ_MAGIC_NUMBER;
   998		spin_lock_init(&cq->cq_lock);
   999	
  1000		if (udata) {
  1001			rc = qedr_copy_cq_uresp(dev, cq, udata, db_offset);
  1002			if (rc)
  1003				goto err2;
  1004	
  1005			rc = qedr_db_recovery_add(dev, cq->q.db_addr,
  1006						  &cq->q.db_rec_data->db_data,
  1007						  DB_REC_WIDTH_64B,
  1008						  DB_REC_USER);
  1009			if (rc)
  1010				goto err2;
  1011	
  1012		} else {
  1013			/* Generate doorbell address. */
  1014			cq->db.data.icid = cq->icid;
  1015			cq->db_addr = dev->db_addr + db_offset;
  1016			cq->db.data.params = DB_AGG_CMD_SET <<
  1017			    RDMA_PWM_VAL32_DATA_AGG_CMD_SHIFT;
  1018	
  1019			/* point to the very last element, passing it we will toggle */
  1020			cq->toggle_cqe = qed_chain_get_last_elem(&cq->pbl);
  1021			cq->pbl_toggle = RDMA_CQE_REQUESTER_TOGGLE_BIT_MASK;
  1022			cq->latest_cqe = NULL;
  1023			consume_cqe(cq);
  1024			cq->cq_cons = qed_chain_get_cons_idx_u32(&cq->pbl);
  1025	
  1026			rc = qedr_db_recovery_add(dev, cq->db_addr, &cq->db.data,
  1027						  DB_REC_WIDTH_64B, DB_REC_KERNEL);
  1028			if (rc)
  1029				goto err2;
  1030		}
  1031	
  1032		DP_DEBUG(dev, QEDR_MSG_CQ,
  1033			 "create cq: icid=0x%0x, addr=%p, size(entries)=0x%0x\n",
  1034			 cq->icid, cq, params.cq_size);
  1035	
  1036		return 0;
  1037	
  1038	err2:
  1039		destroy_iparams.icid = cq->icid;
  1040		dev->ops->rdma_destroy_cq(dev->rdma_ctx, &destroy_iparams,
  1041					  &destroy_oparams);
  1042	err1:
  1043		if (udata) {
  1044			qedr_free_pbl(dev, &cq->q.pbl_info, cq->q.pbl_tbl);
  1045			ib_umem_release(cq->q.umem);
  1046			if (ctx)
                            ^^^
Too late.

  1047				rdma_user_mmap_entry_remove(&ctx->ibucontext,
  1048							    cq->q.db_mmap_entry);

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [bug report] RDMA/qedr: Add doorbell overflow recovery support
  2019-11-06  7:52 [bug report] RDMA/qedr: Add doorbell overflow recovery support Dan Carpenter
@ 2019-11-06 15:38 ` Jason Gunthorpe
  2019-11-07 11:40   ` Dan Carpenter
  0 siblings, 1 reply; 3+ messages in thread
From: Jason Gunthorpe @ 2019-11-06 15:38 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: michal.kalderon, linux-rdma

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [bug report] RDMA/qedr: Add doorbell overflow recovery support
  2019-11-06 15:38 ` Jason Gunthorpe
@ 2019-11-07 11:40   ` Dan Carpenter
  0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2019-11-07 11:40 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: michal.kalderon, linux-rdma

On Wed, Nov 06, 2019 at 11:38:57AM -0400, Jason Gunthorpe wrote:
> >    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
> 

In that case, the other check for NULL ctx is inside an if (udata)
condition so it could be removed.

  1036		return 0;
  1037	
  1038	err2:
  1039		destroy_iparams.icid = cq->icid;
  1040		dev->ops->rdma_destroy_cq(dev->rdma_ctx, &destroy_iparams,
  1041					  &destroy_oparams);
  1042	err1:
  1043		if (udata) {
                    ^^^^^
  1044			qedr_free_pbl(dev, &cq->q.pbl_info, cq->q.pbl_tbl);
  1045			ib_umem_release(cq->q.umem);
  1046			if (ctx)
                            ^^^
Too late.

  1047				rdma_user_mmap_entry_remove(&ctx->ibucontext,
  1048							    cq->q.db_mmap_entry);




regards,
dan carpenter



^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-06  7:52 [bug report] RDMA/qedr: Add doorbell overflow recovery support Dan Carpenter
2019-11-06 15:38 ` Jason Gunthorpe
2019-11-07 11:40   ` Dan Carpenter

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org
	public-inbox-index linux-rdma

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git