ipv6: Fix tcp_v6_send_response checksum

ipv6: Fix tcp_v6_send_response checksum

[Herbert Xu]

Hi:

ipv6: Fix tcp_v6_send_response checksum

My recent patch to remove the open-coded checksum sequence in
tcp_v6_send_response broke it as we did not set the transport
header pointer on the new packet.

Instead we had set the transport header on the original packet,
which is unnecessary and unexpected.

So this patch removes that and instead sets the transport header
on the new packet.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index c92ebe8..075f540 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1015,7 +1015,7 @@ static void tcp_v6_send_response(struct sk_buff *skb, u32 seq, u32 ack, u32 win,
 	skb_reserve(buff, MAX_HEADER + sizeof(struct ipv6hdr) + tot_len);
 
 	t1 = (struct tcphdr *) skb_push(buff, tot_len);
-	skb_reset_transport_header(skb);
+	skb_reset_transport_header(buff);
 
 	/* Swap the send and the receive. */
 	memset(t1, 0, sizeof(*t1));

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: ipv6: Fix tcp_v6_send_response checksum

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 21 Apr 2010 15:07:37 +0800

> ipv6: Fix tcp_v6_send_response checksum

I put this into net-2.6 and modified the commit message since, as we
found, this incorrect transport header reset was added there to fix
IPSEC.

I'm convinced that Cosmin didn't test the patch he actually sent out
:-)

Thanks!

--------------------
ipv6: Fix tcp_v6_send_response transport header setting.

My recent patch to remove the open-coded checksum sequence in
tcp_v6_send_response broke it as we did not set the transport
header pointer on the new packet.

Actually, there is code there trying to set the transport
header properly, but it sets it for the wrong skb ('skb'
instead of 'buff').

This bug was introduced by commit
a8fdf2b331b38d61fb5f11f3aec4a4f9fb2dedcb ("ipv6: Fix
tcp_v6_send_response(): it didn't set skb transport header")

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ipv6/tcp_ipv6.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index c92ebe8..075f540 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1015,7 +1015,7 @@ static void tcp_v6_send_response(struct sk_buff *skb, u32 seq, u32 ack, u32 win,
 	skb_reserve(buff, MAX_HEADER + sizeof(struct ipv6hdr) + tot_len);
 
 	t1 = (struct tcphdr *) skb_push(buff, tot_len);
-	skb_reset_transport_header(skb);
+	skb_reset_transport_header(buff);
 
 	/* Swap the send and the receive. */
 	memset(t1, 0, sizeof(*t1));
-- 
1.7.0.4

Re: ipv6: Fix tcp_v6_send_response checksum

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 21 Apr 2010 00:49:22 -0700 (PDT)

> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Wed, 21 Apr 2010 15:07:37 +0800
> 
>> ipv6: Fix tcp_v6_send_response checksum
> 
> I put this into net-2.6 and modified the commit message since, as we
> found, this incorrect transport header reset was added there to fix
> IPSEC.

Ok, even with this pulled into net-next-2.6 the ipv6 tcp response
checksums are still bad.  The following fix is necessary as
well:

--------------------
tcp: Fix ipv6 checksumming on response packets for real.

Commit 6651ffc8e8bdd5fb4b7d1867c6cfebb4f309512c
("ipv6: Fix tcp_v6_send_response transport header setting.")
fixed one half of why ipv6 tcp response checksums were
invalid, but it's not the whole story.

If we're going to use CHECKSUM_PARTIAL for these things (which we are
since commit 2e8e18ef52e7dd1af0a3bd1f7d990a1d0b249586 "tcp: Set
CHECKSUM_UNNECESSARY in tcp_init_nondata_skb"), we can't be setting
buff->csum as we always have been here in tcp_v6_send_response.  We
need to leave it at zero.

Kill that line and checksums are good again.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ipv6/tcp_ipv6.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 78480f4..5d2e430 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1050,8 +1050,6 @@ static void tcp_v6_send_response(struct sk_buff *skb, u32 seq, u32 ack, u32 win,
 	}
 #endif
 
-	buff->csum = csum_partial(t1, tot_len, 0);
-
 	memset(&fl, 0, sizeof(fl));
 	ipv6_addr_copy(&fl.fl6_dst, &ipv6_hdr(skb)->saddr);
 	ipv6_addr_copy(&fl.fl6_src, &ipv6_hdr(skb)->daddr);
-- 
1.7.0.4

Re: ipv6: Fix tcp_v6_send_response checksum

[Cosmin Ratiu]

On Wednesday 21 April 2010 10:49:22 David Miller wrote:
> I put this into net-2.6 and modified the commit message since, as we
> found, this incorrect transport header reset was added there to fix
> IPSEC.
> 
> I'm convinced that Cosmin didn't test the patch he actually sent out
> 
> :-)

I apologize. I've tested the patch on 2.6.7 (tcp_v6_send_ack then) and then 
redid the patch half-asleep against net-next.

Cosmin.

Re: ipv6: Fix tcp_v6_send_response checksum

[Herbert Xu]

On Wed, Apr 21, 2010 at 01:58:23AM -0700, David Miller wrote:
> 
> If we're going to use CHECKSUM_PARTIAL for these things (which we are
> since commit 2e8e18ef52e7dd1af0a3bd1f7d990a1d0b249586 "tcp: Set
> CHECKSUM_UNNECESSARY in tcp_init_nondata_skb"), we can't be setting
> buff->csum as we always have been here in tcp_v6_send_response.  We
> need to leave it at zero.
> 
> Kill that line and checksums are good again.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>

That line is needed for the CHECKSUM_NONE case.  In fact, if we're
in the CHECKSUM_PARTIAL case, then that buff->csum calculation
should be a noop because it immediately gets overwritten when we
subsequently set csum_start/csum_offset.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: ipv6: Fix tcp_v6_send_response checksum

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 21 Apr 2010 21:09:20 +0800

> On Wed, Apr 21, 2010 at 01:58:23AM -0700, David Miller wrote:
>> 
>> If we're going to use CHECKSUM_PARTIAL for these things (which we are
>> since commit 2e8e18ef52e7dd1af0a3bd1f7d990a1d0b249586 "tcp: Set
>> CHECKSUM_UNNECESSARY in tcp_init_nondata_skb"), we can't be setting
>> buff->csum as we always have been here in tcp_v6_send_response.  We
>> need to leave it at zero.
>> 
>> Kill that line and checksums are good again.
>> 
>> Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> That line is needed for the CHECKSUM_NONE case.  In fact, if we're
> in the CHECKSUM_PARTIAL case, then that buff->csum calculation
> should be a noop because it immediately gets overwritten when we
> subsequently set csum_start/csum_offset.

Ok, but the checksums were broken.  Can you see why?

I think we changed semantics here when we did this:

-	t1->check = csum_ipv6_magic(&fl.fl6_src, &fl.fl6_dst,
-				    tot_len, IPPROTO_TCP,
-				    buff->csum);
+	__tcp_v6_send_check(buff, &fl.fl6_src, &fl.fl6_dst);

which changes what ->csum needs to be.  It used to need to be computed
over the header as well as any data afterwards, but now it has to
cover only data.  Because now it evaluates to:

		th->check = tcp_v6_check(skb->len, saddr, daddr,
					 csum_partial(th, th->doff << 2,
						      skb->csum));

See?  We were computing the checksum over the TCP header twice now :-)
That's why my patch fixed things for dataless packets, whose
->csum would evaluate to zero.

So to make things work fully as-is, we would need to compute ->csum
over the post-header data only.

But an even easier change is to just be consistent with the rest
of the TCP packet generation code and use CHECKSUM_PARTIAL here.
And that's how I'll fix this.

What confused me here last night is the fact that CHECKSUM_NONE
is zero and the implicit zero initialization of new SKBs givs us
that :-)

Anyways, thanks for pointing this out!

Re: ipv6: Fix tcp_v6_send_response checksum

[Herbert Xu]

On Wed, Apr 21, 2010 at 02:28:41PM -0700, David Miller wrote:
> 
> See?  We were computing the checksum over the TCP header twice now :-)
> That's why my patch fixed things for dataless packets, whose
> ->csum would evaluate to zero.

Oh I see, as send_response packets are always dataless this is
corect.

Thanks!
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt