[PATCH] qedr: Fix possible memory leak in qedr_create_qp()

[PATCH] qedr: Fix possible memory leak in qedr_create_qp()

[Wei Yongjun]

From: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

'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-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] qedr: Fix possible memory leak in qedr_create_qp()

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2596 bytes --]

On Fri, Oct 28, 2016 at 04:33:47PM +0000, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>
> '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-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>


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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] qedr: Fix possible memory leak in qedr_create_qp()

[Amrani, 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.
>

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] qedr: Fix possible memory leak in qedr_create_qp()

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] qedr: Fix possible memory leak in qedr_create_qp()

[Amrani, Ram]

> '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-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  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-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
--
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: [PATCH] qedr: Fix possible memory leak in qedr_create_qp()

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 477 bytes --]

On 10/28/2016 12:33 PM, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> 
> '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-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Thanks, applied.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]