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

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- 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 --]