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

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

[Alexander Duyck]

In some recent tests where I was generating invalid frames I found that
the checksum was being treated as valid for certain frames that computed
the checksum with padding included.  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.

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

I haven't had a chance to test this much yet and I am not that familiar
some of this GRO code so any review of this would be greatly appreciated.
I will try to get this tested by end-of-day tomorrow to verify it resolves
the issues I saw with invalid padded frames being marked as valid and
doesn't introduce any new issues.

 net/ipv4/tcp_offload.c   |   13 ++++++++-----
 net/ipv6/tcpv6_offload.c |   11 +++++++----
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index a2b68a1..3dabb76 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -273,13 +273,16 @@ 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 = iph->tot_len;
 	__wsum wsum;
 	__sum16 sum;
 
+	/* adjust for any offsets */
+	length += skb_network_offset(skb) - skb_gro_offset(skb);
+
 	switch (skb->ip_summed) {
 	case CHECKSUM_COMPLETE:
-		if (!tcp_v4_check(skb_gro_len(skb), iph->saddr, iph->daddr,
-				  skb->csum)) {
+		if (!tcp_v4_check(length, iph->saddr, iph->daddr, skb->csum)) {
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 			break;
 		}
@@ -288,11 +291,11 @@ flush:
 		return NULL;
 
 	case CHECKSUM_NONE:
-		wsum = csum_tcpudp_nofold(iph->saddr, iph->daddr,
-					  skb_gro_len(skb), IPPROTO_TCP, 0);
+		wsum = csum_tcpudp_nofold(iph->saddr, iph->daddr, 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..53fc71d 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -36,12 +36,16 @@ 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 = iph->payload_len;
 	__wsum wsum;
 	__sum16 sum;
 
+	/* adjust for any offset due to extension headers */
+	length += skb_transport_offset(skb) - skb_gro_offset(skb);
+
 	switch (skb->ip_summed) {
 	case CHECKSUM_COMPLETE:
-		if (!tcp_v6_check(skb_gro_len(skb), &iph->saddr, &iph->daddr,
+		if (!tcp_v6_check(length, &iph->saddr, &iph->daddr,
 				  skb->csum)) {
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 			break;
@@ -52,11 +56,10 @@ flush:
 
 	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] net: Do not include padding in TCP GRO checksum

[Eric Dumazet]

On Thu, 2013-11-14 at 17:18 -0800, Alexander Duyck wrote:
> In some recent tests where I was generating invalid frames I found that
> the checksum was being treated as valid for certain frames that computed
> the checksum with padding included.  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.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
> 
> I haven't had a chance to test this much yet and I am not that familiar
> some of this GRO code so any review of this would be greatly appreciated.
> I will try to get this tested by end-of-day tomorrow to verify it resolves
> the issues I saw with invalid padded frames being marked as valid and
> doesn't introduce any new issues.
> 
>  net/ipv4/tcp_offload.c   |   13 ++++++++-----
>  net/ipv6/tcpv6_offload.c |   11 +++++++----
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index a2b68a1..3dabb76 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
> @@ -273,13 +273,16 @@ 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 = iph->tot_len;

Well, you probably should test this on x86 for example ;)

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

[David Miller]

From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Thu, 14 Nov 2013 17:18:18 -0800

> I haven't had a chance to test this much yet and I am not that familiar

As Eric mentioned, unless you tested on big endian, chances are you
didn't test this at all. :-)

You're accessing the IPv4/IPv6 length fields without converting it
to CPU endian.

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

[Herbert Xu]

On Thu, Nov 14, 2013 at 05:18:18PM -0800, Alexander Duyck wrote:
> In some recent tests where I was generating invalid frames I found that
> the checksum was being treated as valid for certain frames that computed
> the checksum with padding included.  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.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>

Good catch.

> +	/* adjust for any offsets */
> +	length += skb_network_offset(skb) - skb_gro_offset(skb);

Since skb->csum includes your padding, you'll need to adjust that
as well.  Also this is not the only place where we use skb_gro_len
to measure the packet length.  So rather than changing each one
of them, I think we could just do a pskb_trim_rcsum at the point
where we obtain the network packet length, i.e., in ipv4/ipv6.

We should then fix pskb_trim_rcsum to adjust CHECKSUM_COMPLETE
checksums as otherwise your NIC's RX checksum offload feature
will be useless.

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] net: Do not include padding in TCP GRO checksum

[Alexander Duyck]

On 11/14/2013 08:08 PM, David Miller wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> Date: Thu, 14 Nov 2013 17:18:18 -0800
> 
>> I haven't had a chance to test this much yet and I am not that familiar
> 
> As Eric mentioned, unless you tested on big endian, chances are you
> didn't test this at all. :-)
> 

Actually I had tested it very breifly, but with Rx checksum offloads
still enabled.

> You're accessing the IPv4/IPv6 length fields without converting it
> to CPU endian.

Yeah, I realized that after I hit the BUG_ON in skb_checksum.

I'll be submitting a v2 in the morning with some tested code.  The only
difficult part is that I don't have any devices that generate
CHECKSUM_COMPLETE.  I think if there is padding I will probably just
fall back to the CHECKSUM_NONE case since that seems to be what
IPv4/IPv6 does in their receive calls when they call pskb_trim_rcsum on
padded frames.

Thanks,

Alex

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

[Alexander Duyck]

On 11/14/2013 08:20 PM, Herbert Xu wrote:
> On Thu, Nov 14, 2013 at 05:18:18PM -0800, Alexander Duyck wrote:
>> In some recent tests where I was generating invalid frames I found that
>> the checksum was being treated as valid for certain frames that computed
>> the checksum with padding included.  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.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> Good catch.

I wouldn't have caught it except for I had introduced just the right
packet to see this when I was debugging some FPGA hardware.

> 
>> +	/* adjust for any offsets */
>> +	length += skb_network_offset(skb) - skb_gro_offset(skb);
> 
> Since skb->csum includes your padding, you'll need to adjust that
> as well.  Also this is not the only place where we use skb_gro_len
> to measure the packet length.  So rather than changing each one
> of them, I think we could just do a pskb_trim_rcsum at the point
> where we obtain the network packet length, i.e., in ipv4/ipv6.

I thought about doing it there, but it seemed like the preference was to
flush instead of trim.  Both the inet_gro_receive call and the
ipv6_gro_receive perform a check for length, and if it differs they set
the flush bit.

> We should then fix pskb_trim_rcsum to adjust CHECKSUM_COMPLETE
> checksums as otherwise your NIC's RX checksum offload feature
> will be useless.
> 
> Thanks,

If that is the case why aren't we doing this for ipv4 right now?  I
think what I will do for now is just detect the case where we are padded
for CHECKSUM_COMPLETE and simply fall back to the CHECKSUM_NONE case.

Thanks,

Alex

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

[Ben Hutchings]

On Thu, 2013-11-14 at 17:18 -0800, Alexander Duyck wrote:
> In some recent tests where I was generating invalid frames I found that
> the checksum was being treated as valid for certain frames that computed
> the checksum with padding included.  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.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
> 
> I haven't had a chance to test this much yet and I am not that familiar
> some of this GRO code so any review of this would be greatly appreciated.
> I will try to get this tested by end-of-day tomorrow to verify it resolves
> the issues I saw with invalid padded frames being marked as valid and
> doesn't introduce any new issues.
> 
>  net/ipv4/tcp_offload.c   |   13 ++++++++-----
>  net/ipv6/tcpv6_offload.c |   11 +++++++----
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> index a2b68a1..3dabb76 100644
> --- a/net/ipv4/tcp_offload.c
> +++ b/net/ipv4/tcp_offload.c
[...]
> @@ -288,11 +291,11 @@ flush:
>  		return NULL;
>  
>  	case CHECKSUM_NONE:
> -		wsum = csum_tcpudp_nofold(iph->saddr, iph->daddr,
> -					  skb_gro_len(skb), IPPROTO_TCP, 0);
> +		wsum = csum_tcpudp_nofold(iph->saddr, iph->daddr, length,
> +					  IPPROTO_TCP, 0);
>  		sum = csum_fold(skb_checksum(skb,
>  					     skb_gro_offset(skb),
> -					     skb_gro_len(skb),
> +					     length,

length may also be > skb_gro_len() which makes this unsafe.

>  					     wsum));
>  		if (sum)
>  			goto flush;
> diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
> index c1097c7..53fc71d 100644
> --- a/net/ipv6/tcpv6_offload.c
> +++ b/net/ipv6/tcpv6_offload.c
[...]
> @@ -52,11 +56,10 @@ flush:
>  
>  	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;

Same problem here.

inet_gro_receive() and ipv6_gro_receive() already check whether the IP
length field is consistent with skb_gro_len(), and set
NAPI_GRO_CB(skb)->flush if it is not.  I wonder whether it would make
sense to skip calling the transport-layer gro_receive function in that
case.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.