lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 20 Jan 2016 16:07:05 -0800 From: Eric Dumazet <eric.dumazet@...il.com> To: John <john.phillips5@....com> Cc: Thomas Graf <tgraf@...g.ch>, Jesse Gross <jesse@...nel.org>, Linux Kernel Network Developers <netdev@...r.kernel.org>, Tom Herbert <tom@...bertland.com>, david.roth@....com, Pravin B Shelar <pshelar@...ira.com> Subject: Re: Kernel memory leak in bnx2x driver with vxlan tunnel On Wed, 2016-01-20 at 16:43 -0700, John wrote: > > On 01/19/2016 06:31 PM, Thomas Graf wrote: > > On 01/19/16 at 04:51pm, Jesse Gross wrote: > >> On Tue, Jan 19, 2016 at 4:17 PM, Eric Dumazet <eric.dumazet@...il.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; > 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...? Unfortunately fix is not complete. As someone mentioned, GRO should not aggregate packets having different dst. This part is hard to achieve, as a pointer comparison wont be enough : Each skb has its own meta dst allocation. Quite frankly, I would rather disable GRO for packets with a dst, instead of making GRO dog slow. diff --git a/include/net/gro_cells.h b/include/net/gro_cells.h index cf6c74550baa..124b8a5537e3 100644 --- a/include/net/gro_cells.h +++ b/include/net/gro_cells.h @@ -19,7 +19,10 @@ static inline void gro_cells_receive(struct gro_cells *gcells, struct sk_buff *s struct gro_cell *cell; struct net_device *dev = skb->dev; - if (!gcells->cells || skb_cloned(skb) || !(dev->features & NETIF_F_GRO)) { + if (!gcells->cells || + skb->_skb_refdst || + skb_cloned(skb) || + !(dev->features & NETIF_F_GRO)) { netif_rx(skb); return; }
Powered by blists - more mailing lists