Re: Kernel memory leak in bnx2x driver with vxlan tunnel
From: John <hidden>
Date: 2016-01-20 23:49:18
Subsystem:
networking [general], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
On 01/19/2016 06:31 PM, Thomas Graf wrote:
quoted hunk ↗ jump to hunk
On 01/19/16 at 04:51pm, Jesse Gross wrote:quoted
On Tue, Jan 19, 2016 at 4:17 PM, Eric Dumazet [off-list ref] wrote:quoted
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;
So I've tested the below patch (same as one above with minor modifications made to make it compile) and it worked - no memory leak. Should I submit this or...?
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 4355129..a8fac63 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h@@ -2829,6 +2829,7 @@ int skb_zerocopy(struct sk_buff *to, struct sk_buff *from, void skb_split(struct sk_buff *skb, struct sk_buff *skb1, const u32 len); int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen); void skb_scrub_packet(struct sk_buff *skb, bool xnet); +void skb_release_head_state(struct sk_buff *skb); unsigned int skb_gso_transport_seglen(const struct sk_buff *skb); struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features); struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
diff --git a/net/core/dev.c b/net/core/dev.c
index ae00b89..76e3623 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c@@ -4337,9 +4337,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;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b2df375..45f6f50 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c@@ -633,7 +633,7 @@ fastpath: kmem_cache_free(skbuff_fclone_cache, fclones); } -static void skb_release_head_state(struct sk_buff *skb) +void skb_release_head_state(struct sk_buff *skb) { skb_dst_drop(skb); #ifdef CONFIG_XFRM