Message ID | 20100421.015823.266647388.davem@davemloft.net |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
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,
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! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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!
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);