[PATCH v3] net: Do not include padding in TCP GRO checksum

[PATCH v3] net: Do not include padding in TCP GRO checksum

[Alexander Duyck]

In some recent tests I found the TCP checksum was being treated as valid
for certain frames with padding on them.  On closer inspection I found the
issue was that GRO was using the skb->len instead of the length recorded in
the IP/IPv6 header to determine the number of bytes to checksum.  As such
padded frames that actually had invalid checksums generated by adding the
padding to the checksum were being incorrectly tagged as valid.

This change corrects that by using the tot_len from IPv4 headers and the
payload_len from IPv6 headers to compute the total number of bytes to be
included in the checksum.

To address the fact that skb->csum is invalid when a padded frame is
received I have updated the code to fall though to the CHECKSUM_NONE path
for CHECKSUM_COMPLETE frames that contain padding.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

v2: Update byte ordering of tot_len and payload_len so it is in host order.
    Updated CHECKSUM_COMPLETE path so it falls back through CHECKSUM_NONE for
    padded frames since this is how it is handled in ip_rcv.

    I have tested and verified the CHECKSUM_NONE path works, but I don't have
    any adapters that generate CHECKSUM_COMPLETE to test with.
v3: Added check to handle case where length is greater than skb_gro_len.

 net/ipv4/tcp_offload.c   |   30 +++++++++++++++++++++---------
 net/ipv6/tcpv6_offload.c |   31 +++++++++++++++++++++----------
 2 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index a2b68a1..b32f6c3 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -273,26 +273,38 @@ 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)
 {
 	const struct iphdr *iph = skb_gro_network_header(skb);
+	int length = ntohs(iph->tot_len);
 	__wsum wsum;
 	__sum16 sum;
 
+	/* adjust for any offsets */
+	length += skb_network_offset(skb) - skb_gro_offset(skb);
+
+	/* verify the entire packet is here */
+	if (length > skb_gro_len(skb))
+		goto flush;
+
 	switch (skb->ip_summed) {
 	case CHECKSUM_COMPLETE:
-		if (!tcp_v4_check(skb_gro_len(skb), iph->saddr, iph->daddr,
-				  skb->csum)) {
-			skb->ip_summed = CHECKSUM_UNNECESSARY;
-			break;
-		}
+		if (length == skb_gro_len(skb)) {
+			if (!tcp_v4_check(length, iph->saddr, iph->daddr,
+					   skb->csum)) {
+				skb->ip_summed = CHECKSUM_UNNECESSARY;
+				break;
+			}
 flush:
-		NAPI_GRO_CB(skb)->flush = 1;
-		return NULL;
+			NAPI_GRO_CB(skb)->flush = 1;
+			return NULL;
+		}
 
+		/* skb->csum is invalid if frame is padded */
+		skb->ip_summed = CHECKSUM_NONE;
 	case CHECKSUM_NONE:
 		wsum = csum_tcpudp_nofold(iph->saddr, iph->daddr,
-					  skb_gro_len(skb), IPPROTO_TCP, 0);
+					  length, IPPROTO_TCP, 0);
 		sum = csum_fold(skb_checksum(skb,
 					     skb_gro_offset(skb),
-					     skb_gro_len(skb),
+					     length,
 					     wsum));
 		if (sum)
 			goto flush;
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index c1097c7..f6047cc 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -36,27 +36,38 @@ static struct sk_buff **tcp6_gro_receive(struct sk_buff **head,
 					 struct sk_buff *skb)
 {
 	const struct ipv6hdr *iph = skb_gro_network_header(skb);
+	int length = ntohs(iph->payload_len);
 	__wsum wsum;
 	__sum16 sum;
 
+	/* adjust for any offset due to extension headers */
+	length += skb_transport_offset(skb) - skb_gro_offset(skb);
+
+	/* verify the entire packet is here */
+	if (length > skb_gro_len(skb))
+		goto flush;
+
 	switch (skb->ip_summed) {
 	case CHECKSUM_COMPLETE:
-		if (!tcp_v6_check(skb_gro_len(skb), &iph->saddr, &iph->daddr,
-				  skb->csum)) {
-			skb->ip_summed = CHECKSUM_UNNECESSARY;
-			break;
-		}
+		if (length == skb_gro_len(skb)) {
+			if (!tcp_v6_check(length, &iph->saddr, &iph->daddr,
+					  skb->csum)) {
+				skb->ip_summed = CHECKSUM_UNNECESSARY;
+				break;
+			}
 flush:
-		NAPI_GRO_CB(skb)->flush = 1;
-		return NULL;
+			NAPI_GRO_CB(skb)->flush = 1;
+			return NULL;
+		}
 
+		/* skb->csum is invalid if frame is padded */
+		skb->ip_summed = CHECKSUM_NONE;
 	case CHECKSUM_NONE:
 		wsum = ~csum_unfold(csum_ipv6_magic(&iph->saddr, &iph->daddr,
-						    skb_gro_len(skb),
-						    IPPROTO_TCP, 0));
+						    length, IPPROTO_TCP, 0));
 		sum = csum_fold(skb_checksum(skb,
 					     skb_gro_offset(skb),
-					     skb_gro_len(skb),
+					     length,
 					     wsum));
 		if (sum)
 			goto flush;

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[Herbert Xu]

On Fri, Nov 15, 2013 at 03:00:34PM -0800, Alexander Duyck wrote:
> In some recent tests I found the TCP checksum was being treated as valid
> for certain frames with padding on them.  On closer inspection I found the
> issue was that GRO was using the skb->len instead of the length recorded in
> the IP/IPv6 header to determine the number of bytes to checksum.  As such
> padded frames that actually had invalid checksums generated by adding the
> padding to the checksum were being incorrectly tagged as valid.
> 
> This change corrects that by using the tot_len from IPv4 headers and the
> payload_len from IPv6 headers to compute the total number of bytes to be
> included in the checksum.
> 
> To address the fact that skb->csum is invalid when a padded frame is
> received I have updated the code to fall though to the CHECKSUM_NONE path
> for CHECKSUM_COMPLETE frames that contain padding.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>

Nack.  This is needlessly complex.  As I said before, please
just do a pskb_trim_rcsum in inet_gro_receive and its IPv6
counterpart and this should all just work.

Also, if the padding occurs on every single packet or a fairly
large amount of packets then you should consider improving
pskb_trim_rcsum to keep the CHECKSUM_COMPLETE value.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 16 Nov 2013 08:47:38 +0800

> Also, if the padding occurs on every single packet or a fairly
> large amount of packets then you should consider improving
> pskb_trim_rcsum to keep the CHECKSUM_COMPLETE value.

It should be as easy as this?  (this is actually just a 3 line change,
it looks like more because I'm moving pskb_trim_rcsum() later in the
file to where skb_checksum() is in scope).

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 215b5ea..bec1cc7 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2263,24 +2263,6 @@ static inline void skb_postpull_rcsum(struct sk_buff *skb,
 
 unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len);
 
-/**
- *	pskb_trim_rcsum - trim received skb and update checksum
- *	@skb: buffer to trim
- *	@len: new length
- *
- *	This is exactly the same as pskb_trim except that it ensures the
- *	checksum of received packets are still valid after the operation.
- */
-
-static inline int pskb_trim_rcsum(struct sk_buff *skb, unsigned int len)
-{
-	if (likely(len >= skb->len))
-		return 0;
-	if (skb->ip_summed == CHECKSUM_COMPLETE)
-		skb->ip_summed = CHECKSUM_NONE;
-	return __pskb_trim(skb, len);
-}
-
 #define skb_queue_walk(queue, skb) \
 		for (skb = (queue)->next;					\
 		     skb != (struct sk_buff *)(queue);				\
@@ -2378,6 +2360,27 @@ __wsum __skb_checksum(const struct sk_buff *skb, int offset, int len,
 __wsum skb_checksum(const struct sk_buff *skb, int offset, int len,
 		    __wsum csum);
 
+/**
+ *	pskb_trim_rcsum - trim received skb and update checksum
+ *	@skb: buffer to trim
+ *	@len: new length
+ *
+ *	This is exactly the same as pskb_trim except that it ensures the
+ *	checksum of received packets are still valid after the operation.
+ */
+
+static inline int pskb_trim_rcsum(struct sk_buff *skb, unsigned int len)
+{
+	if (likely(len >= skb->len))
+		return 0;
+	if (skb->ip_summed == CHECKSUM_COMPLETE) {
+		__wsum adj = skb_checksum(skb, len, skb->len - len, 0);
+
+		skb->csum = csum_sub(skb->csum, adj);
+	}
+	return __pskb_trim(skb, len);
+}
+
 static inline void *skb_header_pointer(const struct sk_buff *skb, int offset,
 				       int len, void *buffer)
 {

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[Herbert Xu]

On Fri, Nov 15, 2013 at 08:34:27PM -0500, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Sat, 16 Nov 2013 08:47:38 +0800
> 
> > Also, if the padding occurs on every single packet or a fairly
> > large amount of packets then you should consider improving
> > pskb_trim_rcsum to keep the CHECKSUM_COMPLETE value.
> 
> It should be as easy as this?  (this is actually just a 3 line change,
> it looks like more because I'm moving pskb_trim_rcsum() later in the
> file to where skb_checksum() is in scope).

Looks good to me.  Thanks!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[Herbert Xu]

On Sat, Nov 16, 2013 at 08:47:38AM +0800, Herbert Xu wrote:
> On Fri, Nov 15, 2013 at 03:00:34PM -0800, Alexander Duyck wrote:
> > In some recent tests I found the TCP checksum was being treated as valid
> > for certain frames with padding on them.  On closer inspection I found the
> > issue was that GRO was using the skb->len instead of the length recorded in
> > the IP/IPv6 header to determine the number of bytes to checksum.  As such
> > padded frames that actually had invalid checksums generated by adding the
> > padding to the checksum were being incorrectly tagged as valid.
> > 
> > This change corrects that by using the tot_len from IPv4 headers and the
> > payload_len from IPv6 headers to compute the total number of bytes to be
> > included in the checksum.
> > 
> > To address the fact that skb->csum is invalid when a padded frame is
> > received I have updated the code to fall though to the CHECKSUM_NONE path
> > for CHECKSUM_COMPLETE frames that contain padding.
> > 
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> Nack.  This is needlessly complex.  As I said before, please
> just do a pskb_trim_rcsum in inet_gro_receive and its IPv6
> counterpart and this should all just work.

Actually I take that back.  You are right that the preference is to
flush and not trim, since we want to preserve the incoming packet in
its exact form.

So can you tell me a bit more about the padding? Is it added by the
NIC or did it come from the remote side?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 16 Nov 2013 09:43:50 +0800

> On Fri, Nov 15, 2013 at 08:34:27PM -0500, David Miller wrote:
>> From: Herbert Xu <herbert@gondor.apana.org.au>
>> Date: Sat, 16 Nov 2013 08:47:38 +0800
>> 
>> > Also, if the padding occurs on every single packet or a fairly
>> > large amount of packets then you should consider improving
>> > pskb_trim_rcsum to keep the CHECKSUM_COMPLETE value.
>> 
>> It should be as easy as this?  (this is actually just a 3 line change,
>> it looks like more because I'm moving pskb_trim_rcsum() later in the
>> file to where skb_checksum() is in scope).
> 
> Looks good to me.  Thanks!

Cool, I committed it as follows:

====================
[PATCH] net: Handle CHECKSUM_COMPLETE more adequately in pskb_trim_rcsum().

Currently pskb_trim_rcsum() just balks on CHECKSUM_COMPLETE packets
and remarks them as CHECKSUM_NONE, forcing a software checksum
validation later.

We have all of the mechanics available to fixup the skb->csum value,
even for complicated fragmented packets, via the helpers
skb_checksum() and csum_sub().

So just use them.

Based upon a suggestion by Herbert Xu.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 include/linux/skbuff.h | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 215b5ea..bec1cc7 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2263,24 +2263,6 @@ static inline void skb_postpull_rcsum(struct sk_buff *skb,
 
 unsigned char *skb_pull_rcsum(struct sk_buff *skb, unsigned int len);
 
-/**
- *	pskb_trim_rcsum - trim received skb and update checksum
- *	@skb: buffer to trim
- *	@len: new length
- *
- *	This is exactly the same as pskb_trim except that it ensures the
- *	checksum of received packets are still valid after the operation.
- */
-
-static inline int pskb_trim_rcsum(struct sk_buff *skb, unsigned int len)
-{
-	if (likely(len >= skb->len))
-		return 0;
-	if (skb->ip_summed == CHECKSUM_COMPLETE)
-		skb->ip_summed = CHECKSUM_NONE;
-	return __pskb_trim(skb, len);
-}
-
 #define skb_queue_walk(queue, skb) \
 		for (skb = (queue)->next;					\
 		     skb != (struct sk_buff *)(queue);				\
@@ -2378,6 +2360,27 @@ __wsum __skb_checksum(const struct sk_buff *skb, int offset, int len,
 __wsum skb_checksum(const struct sk_buff *skb, int offset, int len,
 		    __wsum csum);
 
+/**
+ *	pskb_trim_rcsum - trim received skb and update checksum
+ *	@skb: buffer to trim
+ *	@len: new length
+ *
+ *	This is exactly the same as pskb_trim except that it ensures the
+ *	checksum of received packets are still valid after the operation.
+ */
+
+static inline int pskb_trim_rcsum(struct sk_buff *skb, unsigned int len)
+{
+	if (likely(len >= skb->len))
+		return 0;
+	if (skb->ip_summed == CHECKSUM_COMPLETE) {
+		__wsum adj = skb_checksum(skb, len, skb->len - len, 0);
+
+		skb->csum = csum_sub(skb->csum, adj);
+	}
+	return __pskb_trim(skb, len);
+}
+
 static inline void *skb_header_pointer(const struct sk_buff *skb, int offset,
 				       int len, void *buffer)
 {
-- 
1.7.11.7

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[Alexander Duyck]

On 11/15/2013 05:53 PM, Herbert Xu wrote:
> On Sat, Nov 16, 2013 at 08:47:38AM +0800, Herbert Xu wrote:
>> On Fri, Nov 15, 2013 at 03:00:34PM -0800, Alexander Duyck wrote:
>>> In some recent tests I found the TCP checksum was being treated as valid
>>> for certain frames with padding on them.  On closer inspection I found the
>>> issue was that GRO was using the skb->len instead of the length recorded in
>>> the IP/IPv6 header to determine the number of bytes to checksum.  As such
>>> padded frames that actually had invalid checksums generated by adding the
>>> padding to the checksum were being incorrectly tagged as valid.
>>>
>>> This change corrects that by using the tot_len from IPv4 headers and the
>>> payload_len from IPv6 headers to compute the total number of bytes to be
>>> included in the checksum.
>>>
>>> To address the fact that skb->csum is invalid when a padded frame is
>>> received I have updated the code to fall though to the CHECKSUM_NONE path
>>> for CHECKSUM_COMPLETE frames that contain padding.
>>>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> Nack.  This is needlessly complex.  As I said before, please
>> just do a pskb_trim_rcsum in inet_gro_receive and its IPv6
>> counterpart and this should all just work.
> Actually I take that back.  You are right that the preference is to
> flush and not trim, since we want to preserve the incoming packet in
> its exact form.
>
> So can you tell me a bit more about the padding? Is it added by the
> NIC or did it come from the remote side?
>
> Thanks,

The case I am addressing is padding added by the remote side. 
Specifically in my case I was seeing TCP frames without options that
were padded up to 60 bytes from a netperf TCP_RR test.  I messed up the
padding/checksum logic so it was making the same mistake in the Tx
checksum logic in the driver that I caught here in GRO.  As a result I
was seeing checksum errors errors in wireshark, but noticed they were
being accepted by the stack as valid.

The driver has been fixed, and isn't anything that has been pushed
upstream yet so no harm there.  I figured while I was at it I should
probably fix the GRO logic so that it doesn't mis-identify those types
of frames as being valid since that is something that may lead to
similar driver errors going undetected.

Thanks,

Alex

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[Herbert Xu]

On Fri, Nov 15, 2013 at 08:10:00PM -0800, Alexander Duyck wrote:
>
> The case I am addressing is padding added by the remote side. 
> Specifically in my case I was seeing TCP frames without options that
> were padded up to 60 bytes from a netperf TCP_RR test.  I messed up the
> padding/checksum logic so it was making the same mistake in the Tx
> checksum logic in the driver that I caught here in GRO.  As a result I
> was seeing checksum errors errors in wireshark, but noticed they were
> being accepted by the stack as valid.

OK great.  So this isn't normal data that we expect to aggregate.

In that case the simplest solution is to skip the checksum check
altogether.  We only require it if the packet is going to be merged.

So how about something like this?

gro: Only verify TCP checksums for candidates

In some cases we may receive IP packets that are longer than
their stated lengths.  Such packets are never merged in GRO.
However, we may end up computing their checksums incorrectly
and end up allowing packets with a bogus checksum enter our
stack with the checksum status set as verified.

Since such packets are rare and not performance-critical, this
patch simply skips the checksum verification for them.

Reported-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index a2b68a1..55aeec9 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -276,6 +276,10 @@ static struct sk_buff **tcp4_gro_receive(struct sk_buff **head, struct sk_buff *
 	__wsum wsum;
 	__sum16 sum;
 
+	/* Don't bother verifying checksum if we're going to flush anyway. */
+	if (NAPI_GRO_CB(skb)->flush)
+		goto skip_csum;
+
 	switch (skb->ip_summed) {
 	case CHECKSUM_COMPLETE:
 		if (!tcp_v4_check(skb_gro_len(skb), iph->saddr, iph->daddr,
@@ -301,6 +305,7 @@ flush:
 		break;
 	}
 
+skip_csum:
 	return tcp_gro_receive(head, skb);
 }
 
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index c1097c7..71923d1 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -39,6 +39,10 @@ static struct sk_buff **tcp6_gro_receive(struct sk_buff **head,
 	__wsum wsum;
 	__sum16 sum;
 
+	/* Don't bother verifying checksum if we're going to flush anyway. */
+	if (NAPI_GRO_CB(skb)->flush)
+		goto skip_csum;
+
 	switch (skb->ip_summed) {
 	case CHECKSUM_COMPLETE:
 		if (!tcp_v6_check(skb_gro_len(skb), &iph->saddr, &iph->daddr,
@@ -65,6 +69,7 @@ flush:
 		break;
 	}
 
+skip_csum:
 	return tcp_gro_receive(head, skb);
 }

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[Herbert Xu]

On Sat, Nov 16, 2013 at 02:46:11PM +0800, Herbert Xu wrote:
> 
> gro: Only verify TCP checksums for candidates

And here is a patch on top of it that simplifies the checksum
code.

gro: Clean up tcpX_gro_receive checksum verification
    
This patch simplifies the checksum verification in tcpX_gro_receive
by reusing the CHECKSUM_COMPLETE code for CHECKSUM_NONE.  All it
does for CHECKSUM_NONE is compute the partial checksum and then
treat it as if it came from the hardware (CHECKSUM_COMPLETE).
    
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index 55aeec9..0560635 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -274,35 +274,29 @@ static struct sk_buff **tcp4_gro_receive(struct sk_buff **head, struct sk_buff *
 {
 	const struct iphdr *iph = skb_gro_network_header(skb);
 	__wsum wsum;
-	__sum16 sum;
 
 	/* Don't bother verifying checksum if we're going to flush anyway. */
 	if (NAPI_GRO_CB(skb)->flush)
 		goto skip_csum;
 
+	wsum = skb->csum;
+
 	switch (skb->ip_summed) {
+	case CHECKSUM_NONE:
+		wsum = skb_checksum(skb, skb_gro_offset(skb), skb_gro_len(skb),
+				    0);
+
+		/* fall through */
+
 	case CHECKSUM_COMPLETE:
 		if (!tcp_v4_check(skb_gro_len(skb), iph->saddr, iph->daddr,
-				  skb->csum)) {
+				  wsum)) {
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 			break;
 		}
-flush:
+
 		NAPI_GRO_CB(skb)->flush = 1;
 		return NULL;
-
-	case CHECKSUM_NONE:
-		wsum = csum_tcpudp_nofold(iph->saddr, iph->daddr,
-					  skb_gro_len(skb), IPPROTO_TCP, 0);
-		sum = csum_fold(skb_checksum(skb,
-					     skb_gro_offset(skb),
-					     skb_gro_len(skb),
-					     wsum));
-		if (sum)
-			goto flush;
-
-		skb->ip_summed = CHECKSUM_UNNECESSARY;
-		break;
 	}
 
 skip_csum:
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index 71923d1..6d18157 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -37,36 +37,29 @@ static struct sk_buff **tcp6_gro_receive(struct sk_buff **head,
 {
 	const struct ipv6hdr *iph = skb_gro_network_header(skb);
 	__wsum wsum;
-	__sum16 sum;
 
 	/* Don't bother verifying checksum if we're going to flush anyway. */
 	if (NAPI_GRO_CB(skb)->flush)
 		goto skip_csum;
 
+	wsum = skb->csum;
+
 	switch (skb->ip_summed) {
+	case CHECKSUM_NONE:
+		wsum = skb_checksum(skb, skb_gro_offset(skb), skb_gro_len(skb),
+				    wsum);
+
+		/* fall through */
+
 	case CHECKSUM_COMPLETE:
 		if (!tcp_v6_check(skb_gro_len(skb), &iph->saddr, &iph->daddr,
-				  skb->csum)) {
+				  wsum)) {
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 			break;
 		}
-flush:
+
 		NAPI_GRO_CB(skb)->flush = 1;
 		return NULL;
-
-	case CHECKSUM_NONE:
-		wsum = ~csum_unfold(csum_ipv6_magic(&iph->saddr, &iph->daddr,
-						    skb_gro_len(skb),
-						    IPPROTO_TCP, 0));
-		sum = csum_fold(skb_checksum(skb,
-					     skb_gro_offset(skb),
-					     skb_gro_len(skb),
-					     wsum));
-		if (sum)
-			goto flush;
-
-		skb->ip_summed = CHECKSUM_UNNECESSARY;
-		break;
 	}
 
 skip_csum:

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[Alexander Duyck]

On 11/15/2013 10:46 PM, Herbert Xu wrote:
> On Fri, Nov 15, 2013 at 08:10:00PM -0800, Alexander Duyck wrote:
>> The case I am addressing is padding added by the remote side. 
>> Specifically in my case I was seeing TCP frames without options that
>> were padded up to 60 bytes from a netperf TCP_RR test.  I messed up the
>> padding/checksum logic so it was making the same mistake in the Tx
>> checksum logic in the driver that I caught here in GRO.  As a result I
>> was seeing checksum errors errors in wireshark, but noticed they were
>> being accepted by the stack as valid.
> OK great.  So this isn't normal data that we expect to aggregate.

Sorry, I thought that was obvious.  The check in the IPv4/IPv6 GRO
functions always make it so that we flush any frames that contain padding.

> In that case the simplest solution is to skip the checksum check
> altogether.  We only require it if the packet is going to be merged.
>
> So how about something like this?
>
> gro: Only verify TCP checksums for candidates
>
> In some cases we may receive IP packets that are longer than
> their stated lengths.  Such packets are never merged in GRO.
> However, we may end up computing their checksums incorrectly
> and end up allowing packets with a bogus checksum enter our
> stack with the checksum status set as verified.
>
> Since such packets are rare and not performance-critical, this
> patch simply skips the checksum verification for them.
>
> Reported-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index a2b68a1..55aeec9 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
> @@ -276,6 +276,10 @@ static struct sk_buff **tcp4_gro_receive(struct sk_buff **head, struct sk_buff *
>  	__wsum wsum;
>  	__sum16 sum;
>  
> +	/* Don't bother verifying checksum if we're going to flush anyway. */
> +	if (NAPI_GRO_CB(skb)->flush)
> +		goto skip_csum;
> +
>  	switch (skb->ip_summed) {
>  	case CHECKSUM_COMPLETE:
>  		if (!tcp_v4_check(skb_gro_len(skb), iph->saddr, iph->daddr,
> @@ -301,6 +305,7 @@ flush:
>  		break;
>  	}
>  
> +skip_csum:
>  	return tcp_gro_receive(head, skb);
>  }
>  
> diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
> index c1097c7..71923d1 100644
> --- a/net/ipv6/tcpv6_offload.c
> +++ b/net/ipv6/tcpv6_offload.c
> @@ -39,6 +39,10 @@ static struct sk_buff **tcp6_gro_receive(struct sk_buff **head,
>  	__wsum wsum;
>  	__sum16 sum;
>  
> +	/* Don't bother verifying checksum if we're going to flush anyway. */
> +	if (NAPI_GRO_CB(skb)->flush)
> +		goto skip_csum;
> +
>  	switch (skb->ip_summed) {
>  	case CHECKSUM_COMPLETE:
>  		if (!tcp_v6_check(skb_gro_len(skb), &iph->saddr, &iph->daddr,
> @@ -65,6 +69,7 @@ flush:
>  		break;
>  	}
>  
> +skip_csum:
>  	return tcp_gro_receive(head, skb);
>  }
>
> Thanks,

This should work.  I was just playing it safe in the patches I was
submitting by trying not to alter the behaviour.  As long as it is safe
to push something with a bad checksum and the flush bit I am fine with this.

That being the case though, why don't we set the flush flag on detecting
a bad checksum and hand it off to tcp_gro_receive instead of returning
NULL?  It seems like it would be in our interest to flush the flow and
then report the bad checksum instead of keeping the flow and handing off
the bad checksum to the stack.

Thanks,

Alex

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[Herbert Xu]

On Sat, Nov 16, 2013 at 09:02:34AM -0800, Alexander Duyck wrote:
>
> That being the case though, why don't we set the flush flag on detecting
> a bad checksum and hand it off to tcp_gro_receive instead of returning
> NULL?  It seems like it would be in our interest to flush the flow and
> then report the bad checksum instead of keeping the flow and handing off
> the bad checksum to the stack.

Because if the TCP checksum is wrong then it may belong to a
different flow.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[Alexander Duyck]

On 11/16/2013 07:17 PM, Herbert Xu wrote:
> On Sat, Nov 16, 2013 at 09:02:34AM -0800, Alexander Duyck wrote:
>> That being the case though, why don't we set the flush flag on detecting
>> a bad checksum and hand it off to tcp_gro_receive instead of returning
>> NULL?  It seems like it would be in our interest to flush the flow and
>> then report the bad checksum instead of keeping the flow and handing off
>> the bad checksum to the stack.
> Because if the TCP checksum is wrong then it may belong to a
> different flow.
>
> Cheers,

It seems like it would be much more likely that a checksum error
occuring with a padded frame would corrupt the flow identifying data
then one that isn't padded.

I suppose it doesn't really matter though since checksum errors are
probably not going to be all that common of a case to deal with anyway.

Thanks,

Alex

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[Herbert Xu]

On Sun, Nov 17, 2013 at 10:24:36AM -0800, Alexander Duyck wrote:
>
> It seems like it would be much more likely that a checksum error
> occuring with a padded frame would corrupt the flow identifying data
> then one that isn't padded.

Well it doesn't matter anyway because if a packet has an incorrect
checksum it'll be dropped by the receiving stack anyway so a little
reordering is not going to do any harm.  After all, to be 100%
correct, you'd have to flush every single flow between those IP
addresses which is just silly.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[Alexander Duyck]

On 11/15/2013 10:46 PM, Herbert Xu wrote:
> On Fri, Nov 15, 2013 at 08:10:00PM -0800, Alexander Duyck wrote:
>>
>> The case I am addressing is padding added by the remote side. 
>> Specifically in my case I was seeing TCP frames without options that
>> were padded up to 60 bytes from a netperf TCP_RR test.  I messed up the
>> padding/checksum logic so it was making the same mistake in the Tx
>> checksum logic in the driver that I caught here in GRO.  As a result I
>> was seeing checksum errors errors in wireshark, but noticed they were
>> being accepted by the stack as valid.
> 
> OK great.  So this isn't normal data that we expect to aggregate.
> 
> In that case the simplest solution is to skip the checksum check
> altogether.  We only require it if the packet is going to be merged.
> 
> So how about something like this?
> 
> gro: Only verify TCP checksums for candidates
> 
> In some cases we may receive IP packets that are longer than
> their stated lengths.  Such packets are never merged in GRO.
> However, we may end up computing their checksums incorrectly
> and end up allowing packets with a bogus checksum enter our
> stack with the checksum status set as verified.
> 
> Since such packets are rare and not performance-critical, this
> patch simply skips the checksum verification for them.
> 
> Reported-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index a2b68a1..55aeec9 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
> @@ -276,6 +276,10 @@ static struct sk_buff **tcp4_gro_receive(struct sk_buff **head, struct sk_buff *
>  	__wsum wsum;
>  	__sum16 sum;
>  
> +	/* Don't bother verifying checksum if we're going to flush anyway. */
> +	if (NAPI_GRO_CB(skb)->flush)
> +		goto skip_csum;
> +
>  	switch (skb->ip_summed) {
>  	case CHECKSUM_COMPLETE:
>  		if (!tcp_v4_check(skb_gro_len(skb), iph->saddr, iph->daddr,
> @@ -301,6 +305,7 @@ flush:
>  		break;
>  	}
>  
> +skip_csum:
>  	return tcp_gro_receive(head, skb);
>  }
>  
> diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
> index c1097c7..71923d1 100644
> --- a/net/ipv6/tcpv6_offload.c
> +++ b/net/ipv6/tcpv6_offload.c
> @@ -39,6 +39,10 @@ static struct sk_buff **tcp6_gro_receive(struct sk_buff **head,
>  	__wsum wsum;
>  	__sum16 sum;
>  
> +	/* Don't bother verifying checksum if we're going to flush anyway. */
> +	if (NAPI_GRO_CB(skb)->flush)
> +		goto skip_csum;
> +
>  	switch (skb->ip_summed) {
>  	case CHECKSUM_COMPLETE:
>  		if (!tcp_v6_check(skb_gro_len(skb), &iph->saddr, &iph->daddr,
> @@ -65,6 +69,7 @@ flush:
>  		break;
>  	}
>  
> +skip_csum:
>  	return tcp_gro_receive(head, skb);
>  }
> 
> Thanks,
> 

I'm not going to have a chance to test this today, but on review it
should fix the issue.

Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[David Miller]

From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Mon, 18 Nov 2013 09:43:18 -0800

> I'm not going to have a chance to test this today, but on review it
> should fix the issue.
> 
> Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>

I've lost track of this dicussion, Herbert could you post the patches
I should apply?  I think it was this one and a follow-on simplification
to the checksum handling?

Thanks.

Re: [PATCH v3] net: Do not include padding in TCP GRO checksum

[Herbert Xu]

On Thu, Nov 21, 2013 at 01:35:01PM -0500, David Miller wrote:
> 
> I've lost track of this dicussion, Herbert could you post the patches
> I should apply?  I think it was this one and a follow-on simplification
> to the checksum handling?

OK I'll respost.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[1/2] gro: Only verify TCP checksums for candidates

[Herbert Xu]

In some cases we may receive IP packets that are longer than
their stated lengths.  Such packets are never merged in GRO.
However, we may end up computing their checksums incorrectly
and end up allowing packets with a bogus checksum enter our
stack with the checksum status set as verified.

Since such packets are rare and not performance-critical, this
patch simply skips the checksum verification for them.

Reported-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>

diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index a2b68a1..55aeec9 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -276,6 +276,10 @@ static struct sk_buff **tcp4_gro_receive(struct sk_buff **head, struct sk_buff *
 	__wsum wsum;
 	__sum16 sum;
 
+	/* Don't bother verifying checksum if we're going to flush anyway. */
+	if (NAPI_GRO_CB(skb)->flush)
+		goto skip_csum;
+
 	switch (skb->ip_summed) {
 	case CHECKSUM_COMPLETE:
 		if (!tcp_v4_check(skb_gro_len(skb), iph->saddr, iph->daddr,
@@ -301,6 +305,7 @@ flush:
 		break;
 	}
 
+skip_csum:
 	return tcp_gro_receive(head, skb);
 }
 
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index c1097c7..71923d1 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -39,6 +39,10 @@ static struct sk_buff **tcp6_gro_receive(struct sk_buff **head,
 	__wsum wsum;
 	__sum16 sum;
 
+	/* Don't bother verifying checksum if we're going to flush anyway. */
+	if (NAPI_GRO_CB(skb)->flush)
+		goto skip_csum;
+
 	switch (skb->ip_summed) {
 	case CHECKSUM_COMPLETE:
 		if (!tcp_v6_check(skb_gro_len(skb), &iph->saddr, &iph->daddr,
@@ -65,6 +69,7 @@ flush:
 		break;
 	}
 
+skip_csum:
 	return tcp_gro_receive(head, skb);
 }

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[2/2] gro: Clean up tcpX_gro_receive checksum verification

[Herbert Xu]

This patch simplifies the checksum verification in tcpX_gro_receive
by reusing the CHECKSUM_COMPLETE code for CHECKSUM_NONE.  All it
does for CHECKSUM_NONE is compute the partial checksum and then
treat it as if it came from the hardware (CHECKSUM_COMPLETE).
    
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index 55aeec9..0560635 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -274,35 +274,29 @@ static struct sk_buff **tcp4_gro_receive(struct sk_buff **head, struct sk_buff *
 {
 	const struct iphdr *iph = skb_gro_network_header(skb);
 	__wsum wsum;
-	__sum16 sum;
 
 	/* Don't bother verifying checksum if we're going to flush anyway. */
 	if (NAPI_GRO_CB(skb)->flush)
 		goto skip_csum;
 
+	wsum = skb->csum;
+
 	switch (skb->ip_summed) {
+	case CHECKSUM_NONE:
+		wsum = skb_checksum(skb, skb_gro_offset(skb), skb_gro_len(skb),
+				    0);
+
+		/* fall through */
+
 	case CHECKSUM_COMPLETE:
 		if (!tcp_v4_check(skb_gro_len(skb), iph->saddr, iph->daddr,
-				  skb->csum)) {
+				  wsum)) {
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 			break;
 		}
-flush:
+
 		NAPI_GRO_CB(skb)->flush = 1;
 		return NULL;
-
-	case CHECKSUM_NONE:
-		wsum = csum_tcpudp_nofold(iph->saddr, iph->daddr,
-					  skb_gro_len(skb), IPPROTO_TCP, 0);
-		sum = csum_fold(skb_checksum(skb,
-					     skb_gro_offset(skb),
-					     skb_gro_len(skb),
-					     wsum));
-		if (sum)
-			goto flush;
-
-		skb->ip_summed = CHECKSUM_UNNECESSARY;
-		break;
 	}
 
 skip_csum:
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index 71923d1..6d18157 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -37,36 +37,29 @@ static struct sk_buff **tcp6_gro_receive(struct sk_buff **head,
 {
 	const struct ipv6hdr *iph = skb_gro_network_header(skb);
 	__wsum wsum;
-	__sum16 sum;
 
 	/* Don't bother verifying checksum if we're going to flush anyway. */
 	if (NAPI_GRO_CB(skb)->flush)
 		goto skip_csum;
 
+	wsum = skb->csum;
+
 	switch (skb->ip_summed) {
+	case CHECKSUM_NONE:
+		wsum = skb_checksum(skb, skb_gro_offset(skb), skb_gro_len(skb),
+				    wsum);
+
+		/* fall through */
+
 	case CHECKSUM_COMPLETE:
 		if (!tcp_v6_check(skb_gro_len(skb), &iph->saddr, &iph->daddr,
-				  skb->csum)) {
+				  wsum)) {
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 			break;
 		}
-flush:
+
 		NAPI_GRO_CB(skb)->flush = 1;
 		return NULL;
-
-	case CHECKSUM_NONE:
-		wsum = ~csum_unfold(csum_ipv6_magic(&iph->saddr, &iph->daddr,
-						    skb_gro_len(skb),
-						    IPPROTO_TCP, 0));
-		sum = csum_fold(skb_checksum(skb,
-					     skb_gro_offset(skb),
-					     skb_gro_len(skb),
-					     wsum));
-		if (sum)
-			goto flush;
-
-		skb->ip_summed = CHECKSUM_UNNECESSARY;
-		break;
 	}
 
 skip_csum:

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [1/2] gro: Only verify TCP checksums for candidates

[Eric Dumazet]

On Fri, 2013-11-22 at 10:31 +0800, Herbert Xu wrote:
> In some cases we may receive IP packets that are longer than
> their stated lengths.  Such packets are never merged in GRO.
> However, we may end up computing their checksums incorrectly
> and end up allowing packets with a bogus checksum enter our
> stack with the checksum status set as verified.
> 
> Since such packets are rare and not performance-critical, this
> patch simply skips the checksum verification for them.
> 
> Reported-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [2/2] gro: Clean up tcpX_gro_receive checksum verification

[Eric Dumazet]

On Fri, 2013-11-22 at 10:32 +0800, Herbert Xu wrote:
> This patch simplifies the checksum verification in tcpX_gro_receive
> by reusing the CHECKSUM_COMPLETE code for CHECKSUM_NONE.  All it
> does for CHECKSUM_NONE is compute the partial checksum and then
> treat it as if it came from the hardware (CHECKSUM_COMPLETE).
>     
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [1/2] gro: Only verify TCP checksums for candidates

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 21 Nov 2013 21:55:12 -0800

> On Fri, 2013-11-22 at 10:31 +0800, Herbert Xu wrote:
>> In some cases we may receive IP packets that are longer than
>> their stated lengths.  Such packets are never merged in GRO.
>> However, we may end up computing their checksums incorrectly
>> and end up allowing packets with a bogus checksum enter our
>> stack with the checksum status set as verified.
>> 
>> Since such packets are rare and not performance-critical, this
>> patch simply skips the checksum verification for them.
>> 
>> Reported-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>> Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> Acked-by: Eric Dumazet <edumazet@google.com>

Appplied.

Re: [2/2] gro: Clean up tcpX_gro_receive checksum verification

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 21 Nov 2013 21:58:18 -0800

> On Fri, 2013-11-22 at 10:32 +0800, Herbert Xu wrote:
>> This patch simplifies the checksum verification in tcpX_gro_receive
>> by reusing the CHECKSUM_COMPLETE code for CHECKSUM_NONE.  All it
>> does for CHECKSUM_NONE is compute the partial checksum and then
>> treat it as if it came from the hardware (CHECKSUM_COMPLETE).
>>     
>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Acked-by: Eric Dumazet <edumazet@google.com>

Applied.