Re: [PATCH for-next v3] RDMA/rxe: Fix ib_device reference counting (again)

[Bob Pearson <rpearsonhpe@gmail.com>]

> 
> Where do these not-freed skb come from? Except these skb, are other
> resources freed correctly?
> 
> IMHO, the root cause should be found and other resources should be also handled.
> WARN_ON_ONCE() should be kept on exit and done.
> 
> So if some skb are not freed incorrectly, we can find these skb in the loop end.
> 
> Zhu Yanjun
> 

In the original code (before the patch that this patch is trying to fix) there was one path
through rxe_completer() that *could* have leaked an skb. We haven't been seeing that one though.
Every other path through the state machine exited after calling kfree_skb(skb). You can check
but you have to look at an older kernel.

The earlier "Fix FIXME" patch, which is upstream, collected the code related to freeing the skbs
into a subroutine called free_pkt(). This was an improvement but even though it calls
kfree_skb(skb) it didn't set the local variable skb to NULL. This allowed some bogus warnings
to show up as you have been demonstrating.

This patch "fix reference counting (again)" fixes both of these issues by getting rid of the
warning and moving the free_pkt() to the end of rxe_completer(). Now, after applying the
patch, the end of the subroutine reads

...
done:
	if (pkt)
		free_pkt(pkt);
	rxe_drop_ref(qp);

	return ret;
}

If you check, you can see there are no other return statements in the routine. (skb and pkt are
pointers to the same packet. the macro SKB_TO_PKT sets pkt = &skb->cb and PKT_TO_SKB sets
skb = container_of(...) so passing pkt is equivalent to passing skb.)

The only way out, therefore, frees a packet if there is one and we cannot be leaking skbs. The
free_pkt() routine handles all the other stuff that needs to be done when the skb is freed.
(The rxe_drop_ref(qp) drops the reference to QP taken at the top of the rxe_completer() routine.)

The default behavior when receiving an ack packet that isn't expected or perfectly correct is to
drop it. This includes ack packets received when the sender is retrying a request. The comment
after case COMPST_ERROR_RETRY: is misleading because you do have an ack packet when you get there
but it could be unexpected if no wqe is waiting for it. This was the path that before could have
lead to leaking an skb but that is now fixed.

As of now there are no remaining paths that can leak a packet and no reason to check that
skb == NULL. If it makes you happy you can clear it like this

	if (pkt)
		free_pkt(pkt);
	skb = NULL;
	WARN_ON_ONCE(skb);

but that would be a pointless waste of time.

Bob