| 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
| ||
|
Message-ID: <1335835677.11396.5.camel@edumazet-glaptop>
Date: Tue, 01 May 2012 03:27:57 +0200
From: Eric Dumazet <eric.dumazet@...il.com>
To: Alexander Duyck <alexander.h.duyck@...el.com>
Cc: David Miller <davem@...emloft.net>,
netdev <netdev@...r.kernel.org>,
Neal Cardwell <ncardwell@...gle.com>,
Tom Herbert <therbert@...gle.com>,
Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
Michael Chan <mchan@...adcom.com>,
Matt Carlson <mcarlson@...adcom.com>,
Herbert Xu <herbert@...dor.apana.org.au>,
Ben Hutchings <bhutchings@...arflare.com>,
Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>,
Maciej Żenczykowski <maze@...gle.com>
Subject: Re: [PATCH 3/4 v2 net-next] net: make GRO aware of skb->head_frag
On Mon, 2012-04-30 at 16:36 -0700, Alexander Duyck wrote:
> On 04/30/2012 11:10 AM, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@...gle.com>
> >
> > GRO can check if skb to be merged has its skb->head mapped to a page
> > fragment, instead of a kmalloc() area.
> >
> > We 'upgrade' skb->head as a fragment in itself
> >
> > This avoids the frag_list fallback, and permits to build true GRO skb
> > (one sk_buff and up to 16 fragments), using less memory.
> >
> > This reduces number of cache misses when user makes its copy, since a
> > single sk_buff is fetched.
> >
> > This is a followup of patch "net: allow skb->head to be a page fragment"
> >
> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > Cc: Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
> > Cc: Herbert Xu <herbert@...dor.apana.org.au>
> > Cc: Maciej Żenczykowski <maze@...gle.com>
> > Cc: Neal Cardwell <ncardwell@...gle.com>
> > Cc: Tom Herbert <therbert@...gle.com>
> > Cc: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> > Cc: Ben Hutchings <bhutchings@...arflare.com>
> > Cc: Matt Carlson <mcarlson@...adcom.com>
> > Cc: Michael Chan <mchan@...adcom.com>
> > ---
> >
> > v2: change skb->head by skb->data to compute correct first_offset in
> > frag
> >
> > include/linux/netdevice.h | 2 ++
> > include/linux/skbuff.h | 1 +
> > net/core/dev.c | 5 ++++-
> > net/core/skbuff.c | 27 ++++++++++++++++++++++++++-
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index e0b70e9..7f377fb 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1509,6 +1509,8 @@ struct napi_gro_cb {
> >
> > /* Free the skb? */
> > int free;
> > +#define NAPI_GRO_FREE 1
> > +#define NAPI_GRO_FREE_STOLEN_HEAD 2
> > };
> >
> > #define NAPI_GRO_CB(skb) ((struct napi_gro_cb *)(skb)->cb)
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 9d28a22..2c75e98 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -561,6 +561,7 @@ static inline struct rtable *skb_rtable(const struct sk_buff *skb)
> > extern void kfree_skb(struct sk_buff *skb);
> > extern void consume_skb(struct sk_buff *skb);
> > extern void __kfree_skb(struct sk_buff *skb);
> > +extern struct kmem_cache *skbuff_head_cache;
> > extern struct sk_buff *__alloc_skb(unsigned int size,
> > gfp_t priority, int fclone, int node);
> > extern struct sk_buff *build_skb(void *data, unsigned int frag_size);
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 501f3cc..a2be59f 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -3546,7 +3546,10 @@ gro_result_t napi_skb_finish(gro_result_t ret, struct sk_buff *skb)
> > break;
> >
> > case GRO_MERGED_FREE:
> > - consume_skb(skb);
> > + if (NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD)
> > + kmem_cache_free(skbuff_head_cache, skb);
> > + else
> > + __kfree_skb(skb);
> > break;
> >
> > case GRO_HELD:
> How can you go from consume_skb() which checks for skb->users to a case
> that doesn't, or are you guaranteed that there is no way to get here
> with skb->users greater than 1?
>
GRO runs with skbs delivered from NIC rx handlers, and they MUST have
skb->users == 1.
Thinks GRO is the first layer above drivers. We own each skb.
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index effa75d..2ad1ee7 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -69,7 +69,7 @@
> > #include <trace/events/skb.h>
> > #include <linux/highmem.h>
> >
> > -static struct kmem_cache *skbuff_head_cache __read_mostly;
> > +struct kmem_cache *skbuff_head_cache __read_mostly;
> > static struct kmem_cache *skbuff_fclone_cache __read_mostly;
> >
> > static void sock_pipe_buf_release(struct pipe_inode_info *pipe,
> > @@ -2901,6 +2901,31 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb)
> >
> > NAPI_GRO_CB(skb)->free = 1;
> > goto done;
> > + } else if (skb->head_frag) {
> > + int nr_frags = pinfo->nr_frags;
> > + skb_frag_t *frag = pinfo->frags + nr_frags;
> > + struct page *page = virt_to_head_page(skb->head);
> > + unsigned int first_size = headlen - offset;
> > + unsigned int first_offset;
> > +
> > + if (nr_frags + 1 + skbinfo->nr_frags > MAX_SKB_FRAGS)
> > + return -E2BIG;
> > +
> > + first_offset = skb->data -
> > + (unsigned char *)page_address(page) +
> > + offset;
> > +
> > + pinfo->nr_frags = nr_frags + 1 + skbinfo->nr_frags;
> > +
> > + frag->page.p = page;
> > + frag->page_offset = first_offset;
> > + skb_frag_size_set(frag, first_size);
> > +
> > + memcpy(frag + 1, skbinfo->frags, sizeof(*frag) * skbinfo->nr_frags);
> > + /* We dont need to clear skbinfo->nr_frags here */
> > +
> > + NAPI_GRO_CB(skb)->free = NAPI_GRO_FREE_STOLEN_HEAD;
> > + goto done;
> > } else if (skb_gro_len(p) != pinfo->gso_size)
> > return -E2BIG;
> >
> >
> >
> Maybe I missed something, but shouldn't you be checking skb->cloned, and
> skb_shinfo()->dataref before you can consider just dropping the
> sk_buff? It seems like if you are sharing the frag with a clone you
> would have to retain the skb->head so that you can track the dataref.
> Otherwise you will likely cause issues because the stack could end up
> freeing the sk_buff, or the GRO frame will be capable of calling
> put_page and freeing the page out from under the clone.
>
Is it a general question, or specific to this patch ?
If its a general problem, we already check dataref where appropriate.
Fact that skb->head is a kmalloc() or frag doesnt matter.
If specific to GRO, see my first answer. GRO owns each skb.
adding a BUG() would make no sense here.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists