| Message ID | 1477672427-31575-1-git-send-email-weiyj.lk@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Oct 28, 2016 at 04:33:47PM +0000, Wei Yongjun wrote: > From: Wei Yongjun <weiyongjun1@huawei.com> > > 'qp' is malloced in qedr_create_qp() and should be freed before leaving > from the error handling cases, otherwise it will cause memory leak. > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> Hi Ram, While looking on this patch and associated code to it, I noticed the following code stack: qedr_create_qp --> dev->ops->rdma_destroy_qp(dev->rdma_ctx, qp->qed_qp); --> qed_rdma_destroy_qp --> qed_roce_destroy_qp This function will check the QP state and return -INVAL and comment that this QP needs to be prepared before destroying it. However immediately after returning, you are calling to kfree(qp) without any checks. It looks like an error and it is worth to take a look on it. And did I miss the fix to memory leak posted during code review? Thanks > --- > drivers/infiniband/hw/qedr/verbs.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c > index a615142..b60f145 100644 > --- a/drivers/infiniband/hw/qedr/verbs.c > +++ b/drivers/infiniband/hw/qedr/verbs.c > @@ -1477,6 +1477,7 @@ struct ib_qp *qedr_create_qp(struct ib_pd *ibpd, > struct qedr_ucontext *ctx = NULL; > struct qedr_create_qp_ureq ureq; > struct qedr_qp *qp; > + struct ib_qp *ibqp; > int rc = 0; > > DP_DEBUG(dev, QEDR_MSG_QP, "create qp: called from %s, pd=%p\n", > @@ -1486,13 +1487,13 @@ struct ib_qp *qedr_create_qp(struct ib_pd *ibpd, > if (rc) > return ERR_PTR(rc); > > + if (attrs->srq) > + return ERR_PTR(-EINVAL); > + > qp = kzalloc(sizeof(*qp), GFP_KERNEL); > if (!qp) > return ERR_PTR(-ENOMEM); > > - if (attrs->srq) > - return ERR_PTR(-EINVAL); > - > DP_DEBUG(dev, QEDR_MSG_QP, > "create qp: sq_cq=%p, sq_icid=%d, rq_cq=%p, rq_icid=%d\n", > get_qedr_cq(attrs->send_cq), > @@ -1508,7 +1509,10 @@ struct ib_qp *qedr_create_qp(struct ib_pd *ibpd, > "create qp: unexpected udata when creating GSI QP\n"); > goto err0; > } > - return qedr_create_gsi_qp(dev, attrs, qp); > + ibqp = qedr_create_gsi_qp(dev, attrs, qp); > + if (IS_ERR(ibqp)) > + kfree(qp); > + return ibqp; > } > > memset(&in_params, 0, sizeof(in_params)); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> 'qp' is malloced in qedr_create_qp() and should be freed before leaving from the > error handling cases, otherwise it will cause memory leak. > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > --- > drivers/infiniband/hw/qedr/verbs.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/hw/qedr/verbs.c > b/drivers/infiniband/hw/qedr/verbs.c > index a615142..b60f145 100644 > --- a/drivers/infiniband/hw/qedr/verbs.c > +++ b/drivers/infiniband/hw/qedr/verbs.c > @@ -1477,6 +1477,7 @@ struct ib_qp *qedr_create_qp(struct ib_pd *ibpd, > struct qedr_ucontext *ctx = NULL; > struct qedr_create_qp_ureq ureq; > struct qedr_qp *qp; > + struct ib_qp *ibqp; > int rc = 0; > > DP_DEBUG(dev, QEDR_MSG_QP, "create qp: called from %s, pd=%p\n", > @@ -1486,13 +1487,13 @@ struct ib_qp *qedr_create_qp(struct ib_pd *ibpd, > if (rc) > return ERR_PTR(rc); > > + if (attrs->srq) > + return ERR_PTR(-EINVAL); > + > qp = kzalloc(sizeof(*qp), GFP_KERNEL); > if (!qp) > return ERR_PTR(-ENOMEM); > > - if (attrs->srq) > - return ERR_PTR(-EINVAL); > - > DP_DEBUG(dev, QEDR_MSG_QP, > "create qp: sq_cq=%p, sq_icid=%d, rq_cq=%p, rq_icid=%d\n", > get_qedr_cq(attrs->send_cq), > @@ -1508,7 +1509,10 @@ struct ib_qp *qedr_create_qp(struct ib_pd *ibpd, > "create qp: unexpected udata when creating GSI > QP\n"); > goto err0; > } > - return qedr_create_gsi_qp(dev, attrs, qp); > + ibqp = qedr_create_gsi_qp(dev, attrs, qp); > + if (IS_ERR(ibqp)) > + kfree(qp); > + return ibqp; > } > > memset(&in_params, 0, sizeof(in_params)); Thanks again Acked-by: Ram Amrani <Ram.Amrani@cavium.com> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> While looking on this patch and associated code to it, I noticed the > following code stack: > > qedr_create_qp > --> > dev->ops->rdma_destroy_qp(dev->rdma_ctx, qp->qed_qp); > --> > qed_rdma_destroy_qp > --> > qed_roce_destroy_qp > This function will check the QP state and return -INVAL and comment > that this QP needs to be prepared before destroying it. > > However immediately after returning, you are calling to kfree(qp) > without any checks. > > It looks like an error and it is worth to take a look on it. > That's a deep level of reading... thanks. When the QP is created its state is set in ecore_rdma_create_qp(): qp->cur_state = ECORE_ROCE_QP_STATE_RESET; When it is ecore_roce_destroy_qp() is invoked, the function *must* be in either RESET or two other states: if ((qp->cur_state != QED_ROCE_QP_STATE_RESET) && (qp->cur_state != QED_ROCE_QP_STATE_ERR) && (qp->cur_state != QED_ROCE_QP_STATE_INIT)) { DP_NOTICE(p_hwfn, "QP must be in error, reset or init state before destroying it\n"); return -EINVAL; } So actually, we won't return -INVAL here. The bug I see is that I see in our upstream code is that for RESET the normal "destroy" operations continue. But they shouldn't. We need here something like this: if (qp->cur_state == ECORE_ROCE_QP_STATE_RESET) return 0; Flow will return to qed_rdma_destroy_qp() that will release the qp resource in the qed_roce scope (our real purpose). And then return to qedr_create_qp() where the qp resource will be released in the qedr scope. And as a side issue - an improvement that can be added is to return the error code of the QP create and not of the QP destroy. I'll the first later on and probably the second too. > And did I miss the fix to memory leak posted during code review? > As far as I know, I have supplied patches for all memory leaks. Can you direct me to a specific e-mail? Thanks, Ram -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 01, 2016 at 02:42:27PM +0000, Amrani, Ram wrote: > > While looking on this patch and associated code to it, I noticed the > > following code stack: > > > > qedr_create_qp > > --> > > dev->ops->rdma_destroy_qp(dev->rdma_ctx, qp->qed_qp); > > --> > > qed_rdma_destroy_qp > > --> > > qed_roce_destroy_qp > > This function will check the QP state and return -EINVAL and comment > > that this QP needs to be prepared before destroying it. > > > > However immediately after returning, you are calling to kfree(qp) > > without any checks. > > > > It looks like an error and it is worth to take a look on it. > > > > That's a deep level of reading... thanks. You are welcome. > > When the QP is created its state is set in ecore_rdma_create_qp(): > qp->cur_state = ECORE_ROCE_QP_STATE_RESET; > > When it is ecore_roce_destroy_qp() is invoked, the function *must* be in either RESET or two other states: > if ((qp->cur_state != QED_ROCE_QP_STATE_RESET) && > (qp->cur_state != QED_ROCE_QP_STATE_ERR) && > (qp->cur_state != QED_ROCE_QP_STATE_INIT)) { > DP_NOTICE(p_hwfn, > "QP must be in error, reset or init state before destroying it\n"); > return -EINVAL; > } > So actually, we won't return -EINVAL here. > > The bug I see is that I see in our upstream code is that for RESET the normal "destroy" operations continue. But they shouldn't. > We need here something like this: > if (qp->cur_state == ECORE_ROCE_QP_STATE_RESET) > return 0; > > Flow will return to qed_rdma_destroy_qp() that will release the qp resource in the qed_roce scope (our real purpose). > And then return to qedr_create_qp() where the qp resource will be released in the qedr scope. > > And as a side issue - an improvement that can be added is to return the error code of the QP create and not of the QP destroy. I'm happy to hear that it helped. > > I'll the first later on and probably the second too. > > > > > And did I miss the fix to memory leak posted during code review? > > > As far as I know, I have supplied patches for all memory leaks. Can you direct me to a specific e-mail? I failed to find the relevant thread now, so forget it, probably it is my fault. > > Thanks, > Ram > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/28/2016 12:33 PM, Wei Yongjun wrote: > From: Wei Yongjun <weiyongjun1@huawei.com> > > 'qp' is malloced in qedr_create_qp() and should be freed before leaving > from the error handling cases, otherwise it will cause memory leak. > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> Thanks, applied.
diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c index a615142..b60f145 100644 --- a/drivers/infiniband/hw/qedr/verbs.c +++ b/drivers/infiniband/hw/qedr/verbs.c @@ -1477,6 +1477,7 @@ struct ib_qp *qedr_create_qp(struct ib_pd *ibpd, struct qedr_ucontext *ctx = NULL; struct qedr_create_qp_ureq ureq; struct qedr_qp *qp; + struct ib_qp *ibqp; int rc = 0; DP_DEBUG(dev, QEDR_MSG_QP, "create qp: called from %s, pd=%p\n", @@ -1486,13 +1487,13 @@ struct ib_qp *qedr_create_qp(struct ib_pd *ibpd, if (rc) return ERR_PTR(rc); + if (attrs->srq) + return ERR_PTR(-EINVAL); + qp = kzalloc(sizeof(*qp), GFP_KERNEL); if (!qp) return ERR_PTR(-ENOMEM); - if (attrs->srq) - return ERR_PTR(-EINVAL); - DP_DEBUG(dev, QEDR_MSG_QP, "create qp: sq_cq=%p, sq_icid=%d, rq_cq=%p, rq_icid=%d\n", get_qedr_cq(attrs->send_cq), @@ -1508,7 +1509,10 @@ struct ib_qp *qedr_create_qp(struct ib_pd *ibpd, "create qp: unexpected udata when creating GSI QP\n"); goto err0; } - return qedr_create_gsi_qp(dev, attrs, qp); + ibqp = qedr_create_gsi_qp(dev, attrs, qp); + if (IS_ERR(ibqp)) + kfree(qp); + return ibqp; } memset(&in_params, 0, sizeof(in_params));