[kbuild-all] Re: [EXT] Re: drivers/infiniband/hw/qedr/verbs.c:1925 qedr_create_user_qp() error: we previously assumed 'ctx' could be null (see line 1900)

Thursday, 18 February 2021

...
 From: Jason Gunthorpe <jgg(a)nvidia.com&gt;
 Sent: Tuesday, February 16, 2021 9:03 PM

 ----------------------------------------------------------------------
 On Mon, Feb 15, 2021 at 04:31:25PM +0300, Dan Carpenter wrote:
 > tree:   https://urldefense.proofpoint.com/v2/url?u=https-
 3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git&d=DwIBA
 g&c=nKjWec2b6R0mOyPaz7xtfQ&r=5_8rRZTDuAS-6X-
 cGRU9Fo4yjCnkS1t7T3-gjL4FQng&m=YhK42aIA63YEc6ZxxEMSWb1_-
 N4zC52vzPRN82lyUa8&s=ZwYelPnxs3xwj988Lf0pr8AJK-
 oWmLXMQyGqlv74dtI&e=  master
 > head:   dcc0b49040c70ad827a7f3d58a21b01fdb14e749
 > commit: 06e8d1df46ed52eca6915a2a76341ca65cc428b9 RDMA/qedr: Add
 > support for user mode XRC-SRQ's
 > config: x86_64-randconfig-m001-20210213 (attached as .config)
 > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
 >
 > If you fix the issue, kindly add following tag as appropriate
 > Reported-by: kernel test robot <lkp(a)intel.com&gt;
 > Reported-by: Dan Carpenter <dan.carpenter(a)oracle.com&gt;
 >
 > smatch warnings:
 > drivers/infiniband/hw/qedr/verbs.c:1925 qedr_create_user_qp() error:
 > we previously assumed 'ctx' could be null (see line 1900)
 >
 > vim +/ctx +1925 drivers/infiniband/hw/qedr/verbs.c
 >
 > df15856132bc83 Amrani, Ram       2016-12-22  1830  static int
 qedr_create_user_qp(struct qedr_dev *dev,
 > cecbcddf6461a1 Ram Amrani        2016-10-10  1831
 struct qedr_qp *qp,
 > df15856132bc83 Amrani, Ram       2016-12-22  1832
 struct ib_pd *ibpd,
 > df15856132bc83 Amrani, Ram       2016-12-22  1833
 struct ib_udata *udata,
 > df15856132bc83 Amrani, Ram       2016-12-22  1834
 struct ib_qp_init_attr *attrs)
 > cecbcddf6461a1 Ram Amrani        2016-10-10  1835  {
 > df15856132bc83 Amrani, Ram       2016-12-22  1836  	struct
 qed_rdma_create_qp_in_params in_params;
 > df15856132bc83 Amrani, Ram       2016-12-22  1837  	struct
 qed_rdma_create_qp_out_params out_params;
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1838  	struct
 qedr_create_qp_uresp uresp = {};
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1839  	struct
 qedr_create_qp_ureq ureq = {};
 > 69ad0e7fe8452a Kalderon, Michal  2017-07-26  1840  	int alloc_and_init =
 rdma_protocol_roce(&dev->ibdev, 1);
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1841  	struct qedr_ucontext
 *ctx = NULL;
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1842  	struct qedr_pd *pd =
 NULL;
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1843  	int rc = 0;
 > cecbcddf6461a1 Ram Amrani        2016-10-10  1844
 > 82af6d19d8d922 Michal Kalderon   2019-10-27  1845  	qp->create_type =
 QEDR_QP_CREATE_USER;
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1846
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1847  	if (ibpd) {
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1848  		pd =
 get_qedr_pd(ibpd);
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1849  		ctx = pd-
 >uctx;
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1850  	}
 >
 > "ctx" is NULL on the else path.

 Looks like ibpd can never be NULL, so this is dead code

 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1851
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1852  	if (udata) {

 udata also can never be null because this is called 'qedr_create_user_qp' ?

 It is confusingly called with a xrcd test too, but xrcd's are only currently
 possible if udata is set.

 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1853  		rc =
 ib_copy_from_udata(&ureq, udata, min(sizeof(ureq),
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1854
 		udata->inlen));
 > df15856132bc83 Amrani, Ram       2016-12-22  1855  		if (rc) {
 > df15856132bc83 Amrani, Ram       2016-12-22  1856
 	DP_ERR(dev, "Problem copying data from user space\n");
 > df15856132bc83 Amrani, Ram       2016-12-22  1857
 	return rc;
 > df15856132bc83 Amrani, Ram       2016-12-22  1858  		}
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1859  	}
 > cecbcddf6461a1 Ram Amrani        2016-10-10  1860
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1861  	if
 (qedr_qp_has_sq(qp)) {
 > 72b894b09a96b7 Christoph Hellwig 2019-11-13  1862  		/* SQ - read
 access only (0) */
 > b0ea0fa5435f9d Jason Gunthorpe   2019-01-09  1863  		rc =
 qedr_init_user_queue(udata, dev, &qp->usq, ureq.sq_addr,
 > 72b894b09a96b7 Christoph Hellwig 2019-11-13  1864
 		  ureq.sq_len, true, 0, alloc_and_init);
 > df15856132bc83 Amrani, Ram       2016-12-22  1865  		if (rc)
 > df15856132bc83 Amrani, Ram       2016-12-22  1866
 	return rc;
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1867  	}
 > cecbcddf6461a1 Ram Amrani        2016-10-10  1868
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1869  	if
 (qedr_qp_has_rq(qp)) {
 > 72b894b09a96b7 Christoph Hellwig 2019-11-13  1870  		/* RQ - read
 access only (0) */
 > b0ea0fa5435f9d Jason Gunthorpe   2019-01-09  1871  		rc =
 qedr_init_user_queue(udata, dev, &qp->urq, ureq.rq_addr,
 > 72b894b09a96b7 Christoph Hellwig 2019-11-13  1872
 		  ureq.rq_len, true, 0, alloc_and_init);
 > df15856132bc83 Amrani, Ram       2016-12-22  1873  		if (rc)
 > df15856132bc83 Amrani, Ram       2016-12-22  1874
 	return rc;
 > 40b173ddce0fc6 Yuval Bason       2018-08-09  1875  	}
 > df15856132bc83 Amrani, Ram       2016-12-22  1876
 > df15856132bc83 Amrani, Ram       2016-12-22  1877  	memset(&in_params,
 0, sizeof(in_params));
 > df15856132bc83 Amrani, Ram       2016-12-22  1878
 	qedr_init_common_qp_in_params(dev, pd, qp, attrs, false,
 &in_params);
 > df15856132bc83 Amrani, Ram       2016-12-22  1879
 	in_params.qp_handle_lo = ureq.qp_handle_lo;
 > df15856132bc83 Amrani, Ram       2016-12-22  1880
 	in_params.qp_handle_hi = ureq.qp_handle_hi;
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1881
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1882  	if (qp->qp_type ==
 IB_QPT_XRC_TGT) {
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1883  		struct
 qedr_xrcd *xrcd = get_qedr_xrcd(attrs->xrcd);
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1884
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1885
 	in_params.xrcd_id = xrcd->xrcd_id;
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1886
 	in_params.qp_handle_lo = qp->qp_id;
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1887
 	in_params.use_srq = 1;
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1888  	}
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1889
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1890  	if
 (qedr_qp_has_sq(qp)) {
 > df15856132bc83 Amrani, Ram       2016-12-22  1891
 	in_params.sq_num_pages = qp->usq.pbl_info.num_pbes;
 > df15856132bc83 Amrani, Ram       2016-12-22  1892
 	in_params.sq_pbl_ptr = qp->usq.pbl_tbl->pa;
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1893  	}
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1894
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1895  	if
 (qedr_qp_has_rq(qp)) {
 > df15856132bc83 Amrani, Ram       2016-12-22  1896
 	in_params.rq_num_pages = qp->urq.pbl_info.num_pbes;
 > df15856132bc83 Amrani, Ram       2016-12-22  1897
 	in_params.rq_pbl_ptr = qp->urq.pbl_tbl->pa;
 > 40b173ddce0fc6 Yuval Bason       2018-08-09  1898  	}
 > df15856132bc83 Amrani, Ram       2016-12-22  1899
 > bbe4f4245271bd Michal Kalderon   2020-07-07 @1900  	if (ctx)
 >                                                             ^^^ Check
 > for NULL
 >
 > bbe4f4245271bd Michal Kalderon   2020-07-07  1901
 	SET_FIELD(in_params.flags, QED_ROCE_EDPM_MODE, ctx-
 >edpm_mode);
 > bbe4f4245271bd Michal Kalderon   2020-07-07  1902
 > df15856132bc83 Amrani, Ram       2016-12-22  1903  	qp->qed_qp = dev-
 >ops->rdma_create_qp(dev->rdma_ctx,
 > df15856132bc83 Amrani, Ram       2016-12-22  1904
 		      &in_params, &out_params);
 > df15856132bc83 Amrani, Ram       2016-12-22  1905
 > df15856132bc83 Amrani, Ram       2016-12-22  1906  	if (!qp->qed_qp) {
 > df15856132bc83 Amrani, Ram       2016-12-22  1907  		rc = -
 ENOMEM;
 > df15856132bc83 Amrani, Ram       2016-12-22  1908  		goto err1;
 > df15856132bc83 Amrani, Ram       2016-12-22  1909  	}
 > df15856132bc83 Amrani, Ram       2016-12-22  1910
 > 69ad0e7fe8452a Kalderon, Michal  2017-07-26  1911  	if
 (rdma_protocol_iwarp(&dev->ibdev, 1))
 > 69ad0e7fe8452a Kalderon, Michal  2017-07-26  1912
 	qedr_iwarp_populate_user_qp(dev, qp, &out_params);
 > 69ad0e7fe8452a Kalderon, Michal  2017-07-26  1913
 > df15856132bc83 Amrani, Ram       2016-12-22  1914  	qp->qp_id =
 out_params.qp_id;
 > df15856132bc83 Amrani, Ram       2016-12-22  1915  	qp->icid =
 out_params.icid;
 > df15856132bc83 Amrani, Ram       2016-12-22  1916
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1917  	if (udata) {
 > 97f612509294aa Michal Kalderon   2019-10-30  1918  		rc =
 qedr_copy_qp_uresp(dev, qp, udata, &uresp);
 > 97f612509294aa Michal Kalderon   2019-10-30  1919  		if (rc)
 > 97f612509294aa Michal Kalderon   2019-10-30  1920  			goto
 err;
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1921  	}
 > 97f612509294aa Michal Kalderon   2019-10-30  1922
 > 97f612509294aa Michal Kalderon   2019-10-30  1923  	/* db offset was
 calculated in copy_qp_uresp, now set in the user q */
 > 06e8d1df46ed52 Yuval Basson      2020-07-22  1924  	if
 (qedr_qp_has_sq(qp)) {
 > 97f612509294aa Michal Kalderon   2019-10-30 @1925  		qp-
 >usq.db_addr = ctx->dpi_addr + uresp.sq_db_offset;
 >
 > ^^^^^^^^^^^^^ Can "ctx" be NULL here?

 This would also not work right if udata == NULL

 Whole function needs cleaning, but doesn't look buggy
  Thanks, agreed, and will take care of the function re-write.

> Jason

2022

2021

2020

2019

[kbuild-all] Re: [EXT] Re: drivers/infiniband/hw/qedr/verbs.c:1925 qedr_create_user_qp() error: we previously assumed 'ctx' could be null (see line 1900)