All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jarek Poplawski <jarkao2@gmail.com>
To: David Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org, Herbert Xu <herbert@gondor.apana.org.au>,
	Patrick McHardy <kaber@trash.net>
Subject: [PATCH] net: Fix vlan_gro_frags vs netpoll and bonding paths
Date: Fri, 27 Aug 2010 22:50:42 +0200	[thread overview]
Message-ID: <20100827205042.GA13844@del.dom.local> (raw)

After positive netpoll_rx_on() check in vlan_gro_receive() there is
skipped part of the "common" GRO_NORMAL path, especially "pull:" in
dev_gro_receive(), where at least eth header should be copied for
entirely paged skbs. So, eth_type_trans() can read zeroed header only.

Alas moving the netpoll_rx_on() check isn't enough here because a bit
later, in vlan_gro_common(), skb_bond_should_drop() is called, which
depends on skb->protocol and skb->pkt_type data, and can also change
eth_hdr h_dest address (which is overwritten later, btw).

To fix both problems this patch completes copying and verifying of eth
header from the fragment in vlan_gro_receive(). For that purpose the
"pull:" part of dev_gro_receive() was separated into an inline as
skb_gro_pull_in(), and there was added a "lighter" version of
napi_frags_finish(). No gro path except vlan_gro_frags() should be
affected by these changes.

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

 include/linux/netdevice.h |   33 ++++++++++++++++++++++++++++++
 net/8021q/vlan_core.c     |   16 ++++++++++----
 net/core/dev.c            |   48 +++++++++++++++++++++++++-------------------
 3 files changed, 71 insertions(+), 26 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 46c36ff..ad59f76 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1339,6 +1339,36 @@ static inline void skb_gro_pull(struct sk_buff *skb, unsigned int len)
 	NAPI_GRO_CB(skb)->data_offset += len;
 }
 
+/*
+ * Complete pulling of the header(s) from the fragment
+ */
+static inline void skb_gro_pull_in(struct sk_buff *skb)
+{
+	int grow;
+
+	if (skb_headlen(skb) >= skb_gro_offset(skb))
+		return;
+
+	grow = skb_gro_offset(skb) - skb_headlen(skb);
+
+	BUG_ON(skb->end - skb->tail < grow);
+
+	memcpy(skb_tail_pointer(skb), NAPI_GRO_CB(skb)->frag0, grow);
+
+	skb->tail += grow;
+	skb->data_len -= grow;
+
+	skb_shinfo(skb)->frags[0].page_offset += grow;
+	skb_shinfo(skb)->frags[0].size -= grow;
+
+	if (unlikely(!skb_shinfo(skb)->frags[0].size)) {
+		put_page(skb_shinfo(skb)->frags[0].page);
+		memmove(skb_shinfo(skb)->frags,
+			skb_shinfo(skb)->frags + 1,
+			--skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));
+	}
+}
+
 static inline void *skb_gro_header_fast(struct sk_buff *skb,
 					unsigned int offset)
 {
@@ -1701,6 +1731,9 @@ extern struct sk_buff *	napi_get_frags(struct napi_struct *napi);
 extern gro_result_t	napi_frags_finish(struct napi_struct *napi,
 					  struct sk_buff *skb,
 					  gro_result_t ret);
+extern gro_result_t	__napi_frags_finish(struct napi_struct *napi,
+					    struct sk_buff *skb,
+					    gro_result_t ret);
 extern struct sk_buff *	napi_frags_skb(struct napi_struct *napi);
 extern gro_result_t	napi_gro_frags(struct napi_struct *napi);
 
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 01ddb04..58289fe 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -139,13 +139,19 @@ gro_result_t vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp,
 	if (!skb)
 		return GRO_DROP;
 
-	if (netpoll_rx_on(skb)) {
-		skb->protocol = eth_type_trans(skb, skb->dev);
+	/*
+	 * Complete the eth header here, mainly for skb_bond_should_drop(),
+	 * and for netpoll_rx_on() btw.
+	 */
+	skb_gro_pull_in(skb);
+	skb->protocol = eth_type_trans(skb, skb->dev);
+	skb_gro_pull(skb, -ETH_HLEN);
+
+	if (netpoll_rx_on(skb))
 		return vlan_hwaccel_receive_skb(skb, grp, vlan_tci)
 			? GRO_DROP : GRO_NORMAL;
-	}
 
-	return napi_frags_finish(napi, skb,
-				 vlan_gro_common(napi, grp, vlan_tci, skb));
+	return __napi_frags_finish(napi, skb,
+				   vlan_gro_common(napi, grp, vlan_tci, skb));
 }
 EXPORT_SYMBOL(vlan_gro_frags);
diff --git a/net/core/dev.c b/net/core/dev.c
index 3721fbb..cdc0be5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3126,27 +3126,7 @@ enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff *skb)
 	ret = GRO_HELD;
 
 pull:
-	if (skb_headlen(skb) < skb_gro_offset(skb)) {
-		int grow = skb_gro_offset(skb) - skb_headlen(skb);
-
-		BUG_ON(skb->end - skb->tail < grow);
-
-		memcpy(skb_tail_pointer(skb), NAPI_GRO_CB(skb)->frag0, grow);
-
-		skb->tail += grow;
-		skb->data_len -= grow;
-
-		skb_shinfo(skb)->frags[0].page_offset += grow;
-		skb_shinfo(skb)->frags[0].size -= grow;
-
-		if (unlikely(!skb_shinfo(skb)->frags[0].size)) {
-			put_page(skb_shinfo(skb)->frags[0].page);
-			memmove(skb_shinfo(skb)->frags,
-				skb_shinfo(skb)->frags + 1,
-				--skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));
-		}
-	}
-
+	skb_gro_pull_in(skb);
 ok:
 	return ret;
 
@@ -3267,6 +3247,32 @@ gro_result_t napi_frags_finish(struct napi_struct *napi, struct sk_buff *skb,
 }
 EXPORT_SYMBOL(napi_frags_finish);
 
+/*
+ * The lighter version of napi_frags_finish() without eth_type_trans() etc.
+ */
+gro_result_t __napi_frags_finish(struct napi_struct *napi, struct sk_buff *skb,
+				 gro_result_t ret)
+{
+	switch (ret) {
+	case GRO_NORMAL:
+		if (netif_receive_skb(skb))
+			ret = GRO_DROP;
+		break;
+
+	case GRO_DROP:
+	case GRO_MERGED_FREE:
+		napi_reuse_skb(napi, skb);
+		break;
+
+	case GRO_HELD:
+	case GRO_MERGED:
+		break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(__napi_frags_finish);
+
 struct sk_buff *napi_frags_skb(struct napi_struct *napi)
 {
 	struct sk_buff *skb = napi->skb;

             reply	other threads:[~2010-08-27 20:50 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-08-27 20:50 Jarek Poplawski [this message]
2010-08-28  0:13 ` [PATCH] net: Fix vlan_gro_frags vs netpoll and bonding paths Herbert Xu
2010-08-28  9:44   ` Jarek Poplawski
2010-08-28 10:54     ` [RFC] gro: Is it ok to share a single napi from several devs ? Eric Dumazet
2010-08-28 14:31       ` Jarek Poplawski
2010-08-28 14:48         ` Eric Dumazet
2010-08-28 15:16           ` Jarek Poplawski
2010-08-28 17:14           ` Stephen Hemminger
2010-08-28 21:41             ` David Miller
2010-08-28 22:31               ` Stephen Hemminger
2010-08-28 22:33                 ` David Miller
2010-08-29  9:59               ` Jarek Poplawski
2010-08-29 17:06                 ` David Miller
2010-08-29 18:39                   ` Eric Dumazet
2010-08-30  6:42                     ` Jarek Poplawski
2010-08-30 15:57                       ` Stephen Hemminger
2010-08-30 16:50                         ` David Miller
2010-08-30 17:51                           ` [PATCH] sky2: don't do GRO on second port Stephen Hemminger
2010-08-30 19:09                             ` Jarek Poplawski
2010-09-01 21:51                               ` David Miller
2010-09-01 21:55                                 ` Stephen Hemminger
2010-09-02  9:18                                   ` Jarek Poplawski
2010-09-02 12:53                                     ` Jarek Poplawski
2010-09-02 16:30                                       ` David Miller
2010-09-02 16:48                                         ` Jarek Poplawski
2010-09-02  8:33                                 ` Jarek Poplawski
2010-09-02  9:31                                   ` Eric Dumazet
2010-09-02  9:55                                     ` Jarek Poplawski
2010-09-02 10:41                                       ` Eric Dumazet
2010-09-02 11:02                                         ` Jarek Poplawski
2010-09-02 12:09                                           ` Eric Dumazet
2010-09-02 12:28                                             ` Jarek Poplawski
2010-09-02 17:08                                         ` David Miller
2010-09-02 21:26                                         ` Herbert Xu
2010-09-03  5:23                                           ` Eric Dumazet
2010-09-02  9:32                                   ` Jarek Poplawski
2010-08-30 18:36                           ` [RFC] gro: Is it ok to share a single napi from several devs ? Jarek Poplawski
2010-08-30 19:59                             ` [RFC] netpoll: " Eric Dumazet
2010-08-30 20:12                               ` Stephen Hemminger
2010-08-30 20:19                                 ` Eric Dumazet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20100827205042.GA13844@del.dom.local \
    --to=jarkao2@gmail.com \
    --cc=davem@davemloft.net \
    --cc=herbert@gondor.apana.org.au \
    --cc=kaber@trash.net \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.