[PATCH v3 net-next] net-gro: Prepare GRO stack for the upcoming tunneling support

[PATCH v3 net-next] net-gro: Prepare GRO stack for the upcoming tunneling support

[H.K. Jerry Chu]

From: Jerry Chu <hkchu@google.com>

This patch modifies the GRO stack to avoid the use of "network_header"
and associated macros like ip_hdr() and ipv6_hdr() in order to allow
an arbitary number of IP hdrs (v4 or v6) to be used in the
encapsulation chain. This lays the foundation for various IP
tunneling support (IP-in-IP, GRE, VXLAN, SIT,...) to be added later.

With this patch, the GRO stack traversing now is mostly based on
skb_gro_offset rather than special hdr offsets saved in skb (e.g.,
skb->network_header). As a result all but the top layer (i.e., the
the transport layer) must have hdrs of the same length in order for
a pkt to be considered for aggregation. Therefore when adding a new
encap layer (e.g., for tunneling), one must check and skip flows
(e.g., by setting NAPI_GRO_CB(p)->same_flow to 0) that have a
different hdr length.

Note that unlike the network header, the transport header can and
will continue to be set by the GRO code since there will be at
most one "transport layer" in the encap chain.

Signed-off-by: H.K. Jerry Chu <hkchu@google.com>
Suggested-by: Eric Dumazet <edumazet@google.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/netdevice.h |  2 +-
 net/core/dev.c            | 76 ++++++++++++++++-------------------------------
 net/ipv4/af_inet.c        | 25 ++++++++++++----
 net/ipv4/tcp_offload.c    |  9 +++---
 net/ipv6/ip6_offload.c    | 54 ++++++++++++++++++++++++++-------
 net/ipv6/tcpv6_offload.c  |  6 ++--
 6 files changed, 97 insertions(+), 75 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9d55e51..ba321f1b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1673,7 +1673,7 @@ struct offload_callbacks {
 	int			(*gso_send_check)(struct sk_buff *skb);
 	struct sk_buff		**(*gro_receive)(struct sk_buff **head,
 					       struct sk_buff *skb);
-	int			(*gro_complete)(struct sk_buff *skb);
+	int			(*gro_complete)(struct sk_buff *skb, int nhoff);
 };
 
 struct packet_offload {
diff --git a/net/core/dev.c b/net/core/dev.c
index 6cc98dd..bd71bec 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3767,7 +3767,7 @@ static int napi_gro_complete(struct sk_buff *skb)
 		if (ptype->type != type || !ptype->callbacks.gro_complete)
 			continue;
 
-		err = ptype->callbacks.gro_complete(skb);
+		err = ptype->callbacks.gro_complete(skb, 0);
 		break;
 	}
 	rcu_read_unlock();
@@ -3833,6 +3833,23 @@ static void gro_list_prepare(struct napi_struct *napi, struct sk_buff *skb)
 	}
 }
 
+static void skb_gro_reset_offset(struct sk_buff *skb)
+{
+	const struct skb_shared_info *pinfo = skb_shinfo(skb);
+	const skb_frag_t *frag0 = &pinfo->frags[0];
+
+	NAPI_GRO_CB(skb)->data_offset = 0;
+	NAPI_GRO_CB(skb)->frag0 = NULL;
+	NAPI_GRO_CB(skb)->frag0_len = 0;
+
+	if (skb_mac_header(skb) == skb_tail_pointer(skb) &&
+	    pinfo->nr_frags &&
+	    !PageHighMem(skb_frag_page(frag0))) {
+		NAPI_GRO_CB(skb)->frag0 = skb_frag_address(frag0);
+		NAPI_GRO_CB(skb)->frag0_len = skb_frag_size(frag0);
+	}
+}
+
 static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff *skb)
 {
 	struct sk_buff **pp = NULL;
@@ -3848,6 +3865,7 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
 	if (skb_is_gso(skb) || skb_has_frag_list(skb))
 		goto normal;
 
+	skb_gro_reset_offset(skb);
 	gro_list_prepare(napi, skb);
 
 	rcu_read_lock();
@@ -3953,27 +3971,8 @@ static gro_result_t napi_skb_finish(gro_result_t ret, struct sk_buff *skb)
 	return ret;
 }
 
-static void skb_gro_reset_offset(struct sk_buff *skb)
-{
-	const struct skb_shared_info *pinfo = skb_shinfo(skb);
-	const skb_frag_t *frag0 = &pinfo->frags[0];
-
-	NAPI_GRO_CB(skb)->data_offset = 0;
-	NAPI_GRO_CB(skb)->frag0 = NULL;
-	NAPI_GRO_CB(skb)->frag0_len = 0;
-
-	if (skb_mac_header(skb) == skb_tail_pointer(skb) &&
-	    pinfo->nr_frags &&
-	    !PageHighMem(skb_frag_page(frag0))) {
-		NAPI_GRO_CB(skb)->frag0 = skb_frag_address(frag0);
-		NAPI_GRO_CB(skb)->frag0_len = skb_frag_size(frag0);
-	}
-}
-
 gro_result_t napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb)
 {
-	skb_gro_reset_offset(skb);
-
 	return napi_skb_finish(dev_gro_receive(napi, skb), skb);
 }
 EXPORT_SYMBOL(napi_gro_receive);
@@ -4007,12 +4006,7 @@ static gro_result_t napi_frags_finish(struct napi_struct *napi, struct sk_buff *
 {
 	switch (ret) {
 	case GRO_NORMAL:
-	case GRO_HELD:
-		skb->protocol = eth_type_trans(skb, skb->dev);
-
-		if (ret == GRO_HELD)
-			skb_gro_pull(skb, -ETH_HLEN);
-		else if (netif_receive_skb(skb))
+		if (netif_receive_skb(skb))
 			ret = GRO_DROP;
 		break;
 
@@ -4021,6 +4015,7 @@ static gro_result_t napi_frags_finish(struct napi_struct *napi, struct sk_buff *
 		napi_reuse_skb(napi, skb);
 		break;
 
+	case GRO_HELD:
 	case GRO_MERGED:
 		break;
 	}
@@ -4031,36 +4026,15 @@ static gro_result_t napi_frags_finish(struct napi_struct *napi, struct sk_buff *
 static struct sk_buff *napi_frags_skb(struct napi_struct *napi)
 {
 	struct sk_buff *skb = napi->skb;
-	struct ethhdr *eth;
-	unsigned int hlen;
-	unsigned int off;
 
 	napi->skb = NULL;
 
-	skb_reset_mac_header(skb);
-	skb_gro_reset_offset(skb);
-
-	off = skb_gro_offset(skb);
-	hlen = off + sizeof(*eth);
-	eth = skb_gro_header_fast(skb, off);
-	if (skb_gro_header_hard(skb, hlen)) {
-		eth = skb_gro_header_slow(skb, hlen, off);
-		if (unlikely(!eth)) {
-			napi_reuse_skb(napi, skb);
-			skb = NULL;
-			goto out;
-		}
+	if (unlikely(!pskb_may_pull(skb, sizeof(struct ethhdr)))) {
+		napi_reuse_skb(napi, skb);
+		return NULL;
 	}
+	skb->protocol = eth_type_trans(skb, skb->dev);
 
-	skb_gro_pull(skb, sizeof(*eth));
-
-	/*
-	 * This works because the only protocols we care about don't require
-	 * special handling.  We'll fix it up properly at the end.
-	 */
-	skb->protocol = eth->h_proto;
-
-out:
 	return skb;
 }
 
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 70011e0..ef4f9df 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1377,8 +1377,12 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head,
 		if (!NAPI_GRO_CB(p)->same_flow)
 			continue;
 
-		iph2 = ip_hdr(p);
-
+		iph2 = (struct iphdr *)(p->data + off);
+		/* The above works because, with the exception of the top
+		 * (inner most) layer, we only aggregate pkts with the same
+		 * hdr length so all the hdrs we'll need to verify will start
+		 * at the same offset.
+		 */
 		if ((iph->protocol ^ iph2->protocol) |
 		    ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) |
 		    ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {
@@ -1397,6 +1401,11 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head,
 	}
 
 	NAPI_GRO_CB(skb)->flush |= flush;
+	skb_set_network_header(skb, off);
+	/* The above will be needed by the transport layer if there is one
+	 * immediately following this IP hdr.
+	 */
+
 	skb_gro_pull(skb, sizeof(*iph));
 	skb_set_transport_header(skb, skb_gro_offset(skb));
 
@@ -1411,10 +1420,10 @@ out:
 	return pp;
 }
 
-static int inet_gro_complete(struct sk_buff *skb)
+static int inet_gro_complete(struct sk_buff *skb, int nhoff)
 {
-	__be16 newlen = htons(skb->len - skb_network_offset(skb));
-	struct iphdr *iph = ip_hdr(skb);
+	__be16 newlen = htons(skb->len - nhoff);
+	struct iphdr *iph = (struct iphdr *)(skb->data + nhoff);
 	const struct net_offload *ops;
 	int proto = iph->protocol;
 	int err = -ENOSYS;
@@ -1427,7 +1436,11 @@ static int inet_gro_complete(struct sk_buff *skb)
 	if (WARN_ON(!ops || !ops->callbacks.gro_complete))
 		goto out_unlock;
 
-	err = ops->callbacks.gro_complete(skb);
+	/* Only need to add sizeof(*iph) to get to the next hdr below
+	 * because any hdr with option will have been flushed in
+	 * inet_gro_receive().
+	 */
+	err = ops->callbacks.gro_complete(skb, nhoff + sizeof(*iph));
 
 out_unlock:
 	rcu_read_unlock();
diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index 0560635..2658a27 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -240,7 +240,7 @@ int tcp_gro_complete(struct sk_buff *skb)
 {
 	struct tcphdr *th = tcp_hdr(skb);
 
-	skb->csum_start = skb_transport_header(skb) - skb->head;
+	skb->csum_start = (unsigned char *)th - skb->head;
 	skb->csum_offset = offsetof(struct tcphdr, check);
 	skb->ip_summed = CHECKSUM_PARTIAL;
 
@@ -272,6 +272,7 @@ static int tcp_v4_gso_send_check(struct sk_buff *skb)
 
 static struct sk_buff **tcp4_gro_receive(struct sk_buff **head, struct sk_buff *skb)
 {
+	/* Use the IP hdr immediately proceeding for this transport */
 	const struct iphdr *iph = skb_gro_network_header(skb);
 	__wsum wsum;
 
@@ -303,13 +304,13 @@ skip_csum:
 	return tcp_gro_receive(head, skb);
 }
 
-static int tcp4_gro_complete(struct sk_buff *skb)
+static int tcp4_gro_complete(struct sk_buff *skb, int thoff)
 {
 	const struct iphdr *iph = ip_hdr(skb);
 	struct tcphdr *th = tcp_hdr(skb);
 
-	th->check = ~tcp_v4_check(skb->len - skb_transport_offset(skb),
-				  iph->saddr, iph->daddr, 0);
+	th->check = ~tcp_v4_check(skb->len - thoff, iph->saddr,
+				  iph->daddr, 0);
 	skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
 
 	return tcp_gro_complete(skb);
diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index 4b85169..7540a0e 100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -154,6 +154,35 @@ out:
 	return segs;
 }
 
+/* Return the total length of all the extension hdrs, following the same
+ * logic in ipv6_gso_pull_exthdrs() when parsing ext-hdrs.
+ */
+static int ipv6_exthdrs_len(struct ipv6hdr *iph,
+			    const struct net_offload **opps)
+{
+	struct ipv6_opt_hdr *opth = NULL;
+	int len = 0, proto, optlen;
+
+	proto = iph->nexthdr;
+	for (;;) {
+		if (proto != NEXTHDR_HOP) {
+			*opps = rcu_dereference(inet6_offloads[proto]);
+			if (unlikely(!(*opps)))
+				break;
+			if (!((*opps)->flags & INET6_PROTO_GSO_EXTHDR))
+				break;
+		}
+		if (opth == NULL)
+			opth = (void *)(iph+1);
+		else
+			opth = (void *)opth + optlen;
+		optlen = ipv6_optlen(opth);
+		len += optlen;
+		proto = opth->nexthdr;
+	}
+	return len;
+}
+
 static struct sk_buff **ipv6_gro_receive(struct sk_buff **head,
 					 struct sk_buff *skb)
 {
@@ -177,6 +206,7 @@ static struct sk_buff **ipv6_gro_receive(struct sk_buff **head,
 			goto out;
 	}
 
+	skb_set_network_header(skb, off);
 	skb_gro_pull(skb, sizeof(*iph));
 	skb_set_transport_header(skb, skb_gro_offset(skb));
 
@@ -211,12 +241,16 @@ static struct sk_buff **ipv6_gro_receive(struct sk_buff **head,
 		if (!NAPI_GRO_CB(p)->same_flow)
 			continue;
 
-		iph2 = ipv6_hdr(p);
+		iph2 = (struct ipv6hdr *)(p->data + off);
 		first_word = *(__be32 *)iph ^ *(__be32 *)iph2 ;
 
-		/* All fields must match except length and Traffic Class. */
-		if (nlen != skb_network_header_len(p) ||
-		    (first_word & htonl(0xF00FFFFF)) ||
+		/* All fields must match except length and Traffic Class.
+		 * XXX skbs on the gro_list have all been parsed and pulled
+		 * already so we don't need to compare nlen
+		 * (nlen != (sizeof(*iph2) + ipv6_exthdrs_len(iph2, &ops)))
+		 * memcmp() alone below is suffcient, right?
+		 */
+		 if ((first_word & htonl(0xF00FFFFF)) ||
 		    memcmp(&iph->nexthdr, &iph2->nexthdr,
 			   nlen - offsetof(struct ipv6hdr, nexthdr))) {
 			NAPI_GRO_CB(p)->same_flow = 0;
@@ -245,21 +279,21 @@ out:
 	return pp;
 }
 
-static int ipv6_gro_complete(struct sk_buff *skb)
+static int ipv6_gro_complete(struct sk_buff *skb, int nhoff)
 {
 	const struct net_offload *ops;
-	struct ipv6hdr *iph = ipv6_hdr(skb);
+	struct ipv6hdr *iph = (struct ipv6hdr *)(skb->data + nhoff);
 	int err = -ENOSYS;
 
-	iph->payload_len = htons(skb->len - skb_network_offset(skb) -
-				 sizeof(*iph));
+	iph->payload_len = htons(skb->len - nhoff - sizeof(*iph));
 
 	rcu_read_lock();
-	ops = rcu_dereference(inet6_offloads[NAPI_GRO_CB(skb)->proto]);
+
+	nhoff += sizeof(*iph) + ipv6_exthdrs_len(iph, &ops);
 	if (WARN_ON(!ops || !ops->callbacks.gro_complete))
 		goto out_unlock;
 
-	err = ops->callbacks.gro_complete(skb);
+	err = ops->callbacks.gro_complete(skb, nhoff);
 
 out_unlock:
 	rcu_read_unlock();
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index 6d18157..0d78132 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -66,13 +66,13 @@ skip_csum:
 	return tcp_gro_receive(head, skb);
 }
 
-static int tcp6_gro_complete(struct sk_buff *skb)
+static int tcp6_gro_complete(struct sk_buff *skb, int thoff)
 {
 	const struct ipv6hdr *iph = ipv6_hdr(skb);
 	struct tcphdr *th = tcp_hdr(skb);
 
-	th->check = ~tcp_v6_check(skb->len - skb_transport_offset(skb),
-				  &iph->saddr, &iph->daddr, 0);
+	th->check = ~tcp_v6_check(skb->len - thoff, &iph->saddr,
+				  &iph->daddr, 0);
 	skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
 
 	return tcp_gro_complete(skb);
-- 
1.8.5.1

Re: [PATCH v3 net-next] net-gro: Prepare GRO stack for the upcoming tunneling support

[David Miller]

From: "H.K. Jerry Chu" <hkchu@google.com>
Date: Wed, 11 Dec 2013 20:53:45 -0800

> From: Jerry Chu <hkchu@google.com>
> 
> This patch modifies the GRO stack to avoid the use of "network_header"
> and associated macros like ip_hdr() and ipv6_hdr() in order to allow
> an arbitary number of IP hdrs (v4 or v6) to be used in the
> encapsulation chain. This lays the foundation for various IP
> tunneling support (IP-in-IP, GRE, VXLAN, SIT,...) to be added later.
> 
> With this patch, the GRO stack traversing now is mostly based on
> skb_gro_offset rather than special hdr offsets saved in skb (e.g.,
> skb->network_header). As a result all but the top layer (i.e., the
> the transport layer) must have hdrs of the same length in order for
> a pkt to be considered for aggregation. Therefore when adding a new
> encap layer (e.g., for tunneling), one must check and skip flows
> (e.g., by setting NAPI_GRO_CB(p)->same_flow to 0) that have a
> different hdr length.
> 
> Note that unlike the network header, the transport header can and
> will continue to be set by the GRO code since there will be at
> most one "transport layer" in the encap chain.
> 
> Signed-off-by: H.K. Jerry Chu <hkchu@google.com>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Applied, thanks.

Re: [PATCH v3 net-next] net-gro: Prepare GRO stack for the upcoming tunneling support

[Jerry Chu]

On Fri, Dec 13, 2013 at 2:48 AM, David Miller <davem@davemloft.net> wrote:
> From: "H.K. Jerry Chu" <hkchu@google.com>
> Date: Wed, 11 Dec 2013 20:53:45 -0800
>
>> From: Jerry Chu <hkchu@google.com>
>>
>> This patch modifies the GRO stack to avoid the use of "network_header"
>> and associated macros like ip_hdr() and ipv6_hdr() in order to allow
>> an arbitary number of IP hdrs (v4 or v6) to be used in the
>> encapsulation chain. This lays the foundation for various IP
>> tunneling support (IP-in-IP, GRE, VXLAN, SIT,...) to be added later.
>>
>> With this patch, the GRO stack traversing now is mostly based on
>> skb_gro_offset rather than special hdr offsets saved in skb (e.g.,
>> skb->network_header). As a result all but the top layer (i.e., the
>> the transport layer) must have hdrs of the same length in order for
>> a pkt to be considered for aggregation. Therefore when adding a new
>> encap layer (e.g., for tunneling), one must check and skip flows
>> (e.g., by setting NAPI_GRO_CB(p)->same_flow to 0) that have a
>> different hdr length.
>>
>> Note that unlike the network header, the transport header can and
>> will continue to be set by the GRO code since there will be at
>> most one "transport layer" in the encap chain.
>>
>> Signed-off-by: H.K. Jerry Chu <hkchu@google.com>
>> Suggested-by: Eric Dumazet <edumazet@google.com>
>> Reviewed-by: Eric Dumazet <edumazet@google.com>
>
> Applied, thanks.

Thanks, David. I will submit a patch for GRE soon to demonstrate
just how easy it is to add tunneling support to GRO after the assumption
of only one IP hdr in the encap chain is removed. (I already have the
code but it doesn't support the CSUM flag. I'm thinking adding it
plus csum offload support before submitting the patch.)

Best,

Jerry

[PATCH net-next] ipv6: fix compiler warning in ipv6_exthdrs_len

[Hannes Frederic Sowa]

Commit 299603e8370a93dd5d8e8d800f0dff1ce2c53d36 ("net-gro: Prepare GRO
stack for the upcoming tunneling support") used an uninitialized variable
which leads to the following compiler warning:

net/ipv6/ip6_offload.c: In function ‘ipv6_gro_complete’:
net/ipv6/ip6_offload.c:178:24: warning: ‘optlen’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    opth = (void *)opth + optlen;
                        ^
net/ipv6/ip6_offload.c:164:22: note: ‘optlen’ was declared here
  int len = 0, proto, optlen;
                      ^
Fix it up.

Cc: Jerry Chu <hkchu@google.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---

I just noticed this and tried to fix it. Jerry, could you please review?
I hope I got it right. ;)

 net/ipv6/ip6_offload.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index 7540a0e..08861f1 100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -161,7 +161,7 @@ static int ipv6_exthdrs_len(struct ipv6hdr *iph,
 			    const struct net_offload **opps)
 {
 	struct ipv6_opt_hdr *opth = NULL;
-	int len = 0, proto, optlen;
+	int len = 0, optlen = 0, proto;
 
 	proto = iph->nexthdr;
 	for (;;) {
@@ -172,11 +172,12 @@ static int ipv6_exthdrs_len(struct ipv6hdr *iph,
 			if (!((*opps)->flags & INET6_PROTO_GSO_EXTHDR))
 				break;
 		}
-		if (opth == NULL)
+		if (opth == NULL) {
 			opth = (void *)(iph+1);
-		else
+		} else {
+			optlen = ipv6_optlen(opth);
 			opth = (void *)opth + optlen;
-		optlen = ipv6_optlen(opth);
+		}
 		len += optlen;
 		proto = opth->nexthdr;
 	}
-- 
1.8.3.1

Re: [PATCH net-next] ipv6: fix compiler warning in ipv6_exthdrs_len

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Sat, 14 Dec 2013 07:29:29 +0100

> Commit 299603e8370a93dd5d8e8d800f0dff1ce2c53d36 ("net-gro: Prepare GRO
> stack for the upcoming tunneling support") used an uninitialized variable
> which leads to the following compiler warning:
> 
> net/ipv6/ip6_offload.c: In function ‘ipv6_gro_complete’:
> net/ipv6/ip6_offload.c:178:24: warning: ‘optlen’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     opth = (void *)opth + optlen;
>                         ^
> net/ipv6/ip6_offload.c:164:22: note: ‘optlen’ was declared here
>   int len = 0, proto, optlen;
>                       ^
> Fix it up.
> 
> Cc: Jerry Chu <hkchu@google.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Applied.

Re: [PATCH net-next] ipv6: fix compiler warning in ipv6_exthdrs_len

[Jerry Chu]

On Sat, Dec 14, 2013 at 3:02 PM, David Miller <davem@davemloft.net> wrote:
> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Date: Sat, 14 Dec 2013 07:29:29 +0100
>
>> Commit 299603e8370a93dd5d8e8d800f0dff1ce2c53d36 ("net-gro: Prepare GRO
>> stack for the upcoming tunneling support") used an uninitialized variable
>> which leads to the following compiler warning:
>>
>> net/ipv6/ip6_offload.c: In function ‘ipv6_gro_complete’:
>> net/ipv6/ip6_offload.c:178:24: warning: ‘optlen’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>>     opth = (void *)opth + optlen;
>>                         ^
>> net/ipv6/ip6_offload.c:164:22: note: ‘optlen’ was declared here
>>   int len = 0, proto, optlen;
>>                       ^
>> Fix it up.
>>
>> Cc: Jerry Chu <hkchu@google.com>
>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>
> Applied.

Please back out the above "fix" - it actually breaks the original code because
the fix will cause exthdrs_len to be one optlen short.

There was no bug in the original code, just the compiler warning (and I do not
get any warning, although I admit I did not run with the -W flag).

In any case I will submit a revision shortly that will silent any
warning if exists.

Jerry

[PATCH net-next] ipv6: revert misguided compiler warning fix on ipv6_exthdrs_len

[Hannes Frederic Sowa]

This essentlally reverts commit f52d81dc27c3456c702e83183035142c222acdc7
("ipv6: fix compiler warning in ipv6_exthdrs_len") and silences the
warning with uninitialized_var.

Cc: Jerry Chu <hkchu@google.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---

On Mon, Dec 16, 2013 at 12:20:22AM +0800, Jerry Chu wrote:
> On Sat, Dec 14, 2013 at 3:02 PM, David Miller <davem@davemloft.net> wrote:
> > From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > Date: Sat, 14 Dec 2013 07:29:29 +0100
> >
> >> Commit 299603e8370a93dd5d8e8d800f0dff1ce2c53d36 ("net-gro: Prepare GRO
> >> stack for the upcoming tunneling support") used an uninitialized variable
> >> which leads to the following compiler warning:
> >>
> >> net/ipv6/ip6_offload.c: In function ‘ipv6_gro_complete’:
> >> net/ipv6/ip6_offload.c:178:24: warning: ‘optlen’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >>     opth = (void *)opth + optlen;
> >>                         ^
> >> net/ipv6/ip6_offload.c:164:22: note: ‘optlen’ was declared here
> >>   int len = 0, proto, optlen;
> >>                       ^
> >> Fix it up.
> >>
> >> Cc: Jerry Chu <hkchu@google.com>
> >> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> >
> > Applied.
> 
> Please back out the above "fix" - it actually breaks the original code because
> the fix will cause exthdrs_len to be one optlen short.
>
> There was no bug in the original code, just the compiler warning (and I do not
> get any warning, although I admit I did not run with the -W flag).

Sorry, I see now, of course. Btw. I didn't use any special flags, just
a normal kernel build with gcc 4.8.2.

> In any case I will submit a revision shortly that will silent any
> warning if exists.

I hope you are ok with this.

Sorry,

  Hannes

 net/ipv6/ip6_offload.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
index 08861f1..0a578fc 100644
--- a/net/ipv6/ip6_offload.c
+++ b/net/ipv6/ip6_offload.c
@@ -161,7 +161,7 @@ static int ipv6_exthdrs_len(struct ipv6hdr *iph,
 			    const struct net_offload **opps)
 {
 	struct ipv6_opt_hdr *opth = NULL;
-	int len = 0, optlen = 0, proto;
+	int len = 0, proto, uninitialized_var(optlen);
 
 	proto = iph->nexthdr;
 	for (;;) {
@@ -172,12 +172,11 @@ static int ipv6_exthdrs_len(struct ipv6hdr *iph,
 			if (!((*opps)->flags & INET6_PROTO_GSO_EXTHDR))
 				break;
 		}
-		if (opth == NULL) {
+		if (opth == NULL)
 			opth = (void *)(iph+1);
-		} else {
-			optlen = ipv6_optlen(opth);
+		else
 			opth = (void *)opth + optlen;
-		}
+		optlen = ipv6_optlen(opth);
 		len += optlen;
 		proto = opth->nexthdr;
 	}
-- 
1.8.3.1

Re: [PATCH net-next] ipv6: revert misguided compiler warning fix on ipv6_exthdrs_len

[Hannes Frederic Sowa]

On Sun, Dec 15, 2013 at 06:18:06PM +0100, Hannes Frederic Sowa wrote:
> This essentlally reverts commit f52d81dc27c3456c702e83183035142c222acdc7
> ("ipv6: fix compiler warning in ipv6_exthdrs_len") and silences the
> warning with uninitialized_var.

David, please discard this. Jerry already posted a nicer cleanup.

Sorry,

  Hannes

Re: [PATCH net-next] ipv6: revert misguided compiler warning fix on ipv6_exthdrs_len

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Sun, 15 Dec 2013 18:23:22 +0100

> On Sun, Dec 15, 2013 at 06:18:06PM +0100, Hannes Frederic Sowa wrote:
>> This essentlally reverts commit f52d81dc27c3456c702e83183035142c222acdc7
>> ("ipv6: fix compiler warning in ipv6_exthdrs_len") and silences the
>> warning with uninitialized_var.
> 
> David, please discard this. Jerry already posted a nicer cleanup.

I can't resolve these two patches in any reasonable way.

Specifically, whether I revert your patch or not, I cannot get Jerry's
patch to apply cleanly.

I can't figure out what Jerry's patch is against at all.