Re: Kernel memory leak in bnx2x driver with vxlan tunnel

[Thomas Graf <tgraf@suug.ch>]

On 01/19/16 at 04:51pm, Jesse Gross wrote:
> On Tue, Jan 19, 2016 at 4:17 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > So what is the purpose of having a dst if we need to drop it ?
> >
> > Adding code in GRO would be fine if someone explains me the purpose of
> > doing apparently useless work.
> >
> > (refcounting on dst is not exactly free)
> 
> In the GRO case, the dst is only dropped on the packets which have
> been merged and therefore need to be freed (the GRO_MERGED_FREE case).
> It's not being thrown away for the overall frame, just metadata that
> has been duplicated on each individual frame, similar to the metadata
> in struct sk_buff itself. And while it is not used by the IP stack
> there are other consumers (eBPF/OVS/etc.). This entire process is
> controlled by the COLLECT_METADATA flag on tunnels, so there is no
> cost in situations where it is not actually used.

Right. There were thoughts around leveraging a per CPU scratch
buffer without a refcount and turn it into a full reference when
the packet gets enqueued somewhere but the need hasn't really come
up yet.

Jesse, is this what you have in mind:

diff --git a/net/core/dev.c b/net/core/dev.c
index cc9e365..3a5e96d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4548,9 +4548,10 @@ static gro_result_t napi_skb_finish(gro_result_t ret, struct sk_buff *skb)
                break;
 
        case GRO_MERGED_FREE:
-               if (NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD)
+               if (NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD) {
+                       skb_release_head_state(skb);
                        kmem_cache_free(skbuff_head_cache, skb);
-               else
+               } else
                        __kfree_skb(skb);
                break;