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: <CAKgT0UdeVvJe6k5OitkKgujSw3SQSOvtQ84KrPch8xdd=5ub1A@mail.gmail.com> Date: Mon, 30 Apr 2012 22:33:11 -0700 From: Alexander Duyck <alexander.duyck@...il.com> To: Eric Dumazet <eric.dumazet@...il.com> Cc: Alexander Duyck <alexander.h.duyck@...el.com>, 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, Apr 30, 2012 at 6:27 PM, Eric Dumazet <eric.dumazet@...il.com> wrote: > 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" >> > [...] >> > 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. The question I had was more specific to GRO. As long as we have skb->users == 1 and the skb isn't cloned we should be fine. It just hadn't occurred to me before that napi_gro_receive had the extra requirement that the skb couldn't be cloned. Thanks, Alex -- 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