[bug report] qedr: Add support for QP verbs

[bug report] qedr: Add support for QP verbs

[Dan Carpenter]

Hello Ram Amrani,

The patch cecbcddf6461: "qedr: Add support for QP verbs" from Oct 10,
2016, leads to the following static checker warning:

	drivers/infiniband/hw/qedr/verbs.c:1494 qedr_create_qp()
	warn: possible memory leak of 'qp'

drivers/infiniband/hw/qedr/verbs.c
  1484  
  1485          rc = qedr_check_qp_attrs(ibpd, dev, attrs);
  1486          if (rc)
  1487                  return ERR_PTR(rc);
  1488  
  1489          qp = kzalloc(sizeof(*qp), GFP_KERNEL);
  1490          if (!qp)
  1491                  return ERR_PTR(-ENOMEM);
  1492  
  1493          if (attrs->srq)
  1494                  return ERR_PTR(-EINVAL);

You should move this in front of the allocation to avoid the memory
leak.

  1495  
  1496          DP_DEBUG(dev, QEDR_MSG_QP,
  1497                   "create qp: sq_cq=%p, sq_icid=%d, rq_cq=%p, rq_icid=%d\n",
  1498                   get_qedr_cq(attrs->send_cq),
  1499                   get_qedr_cq(attrs->send_cq)->icid,
  1500                   get_qedr_cq(attrs->recv_cq),
  1501                   get_qedr_cq(attrs->recv_cq)->icid);
  1502  
  1503          qedr_set_qp_init_params(dev, qp, pd, attrs);
  1504  
  1505          if (attrs->qp_type == IB_QPT_GSI) {
  1506                  if (udata) {
  1507                          DP_ERR(dev,
  1508                                 "create qp: unexpected udata when creating GSI QP\n");
  1509                          goto err0;

Ugh...  GW-BASIC style numbered labels...  What does goto err0 do???
Imagine if instead of function names we should use numbers like:

	one();
	two();
	five();

Use a meaningful label names like "goto free_qp;"

  1510                  }
  1511                  return qedr_create_gsi_qp(dev, attrs, qp);

We should free qp if qedr_create_gsi_qp() fails as well.

  1512          }
  1513  
  1514          memset(&in_params, 0, sizeof(in_params));
  1515  
  1516          if (udata) {
  1517                  if (!(udata && ibpd->uobject && ibpd->uobject->context))
  1518                          goto err0;
  1519  

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [bug report] qedr: Add support for QP verbs

[Amrani, Ram]

> Hello Ram Amrani,
> 
> The patch cecbcddf6461: "qedr: Add support for QP verbs" from Oct 10, 2016,
> leads to the following static checker warning:
> 
> 	drivers/infiniband/hw/qedr/verbs.c:2067 qedr_destroy_qp()
> 	'0x5 | 0x1' has '0x1' set on both sides
> 
> drivers/infiniband/hw/qedr/verbs.c
>   2056  int qedr_destroy_qp(struct ib_qp *ibqp)
>   2057  {
>   2058          struct qedr_qp *qp = get_qedr_qp(ibqp);
>   2059          struct qedr_dev *dev = qp->dev;
>   2060          struct ib_qp_attr attr;
>   2061          int attr_mask = 0;
>   2062          int rc = 0;
>   2063
>   2064          DP_DEBUG(dev, QEDR_MSG_QP, "destroy qp: destroying %p, qp
> type=%d\n",
>   2065                   qp, qp->qp_type);
>   2066
>   2067          if (qp->state != (QED_ROCE_QP_STATE_RESET |
> QED_ROCE_QP_STATE_ERR |
>   2068                            QED_ROCE_QP_STATE_INIT)) {
> 
> These aren't bitfields, they're just numbers.  This code is pretty suspect.  Not
> sure what was intended.
> 
>   2069                  attr.qp_state = IB_QPS_ERR;
>   2070                  attr_mask |= IB_QP_STATE;
>   2071
>   2072                  /* Change the QP state to ERROR */
>   2073                  qedr_modify_qp(ibqp, &attr, attr_mask, NULL);
>   2074          }
> 
> regards,
> dan carpenter

That's a real bug. Thanks!
Will fix.

Ram

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[bug report] qedr: Add support for QP verbs

[Dan Carpenter]

Hello Ram Amrani,

The patch cecbcddf6461: "qedr: Add support for QP verbs" from Oct 10,
2016, leads to the following static checker warning:

	drivers/infiniband/hw/qedr/verbs.c:2067 qedr_destroy_qp()
	'0x5 | 0x1' has '0x1' set on both sides

drivers/infiniband/hw/qedr/verbs.c
  2056  int qedr_destroy_qp(struct ib_qp *ibqp)
  2057  {
  2058          struct qedr_qp *qp = get_qedr_qp(ibqp);
  2059          struct qedr_dev *dev = qp->dev;
  2060          struct ib_qp_attr attr;
  2061          int attr_mask = 0;
  2062          int rc = 0;
  2063  
  2064          DP_DEBUG(dev, QEDR_MSG_QP, "destroy qp: destroying %p, qp type=%d\n",
  2065                   qp, qp->qp_type);
  2066  
  2067          if (qp->state != (QED_ROCE_QP_STATE_RESET | QED_ROCE_QP_STATE_ERR |
  2068                            QED_ROCE_QP_STATE_INIT)) {

These aren't bitfields, they're just numbers.  This code is pretty
suspect.  Not sure what was intended.

  2069                  attr.qp_state = IB_QPS_ERR;
  2070                  attr_mask |= IB_QP_STATE;
  2071  
  2072                  /* Change the QP state to ERROR */
  2073                  qedr_modify_qp(ibqp, &attr, attr_mask, NULL);
  2074          }

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html