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

On Tue, Nov 01, 2016 at 02:42:27PM +0000, Amrani, Ram wrote:

quoted

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.

quoted

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