Re: [PATCH 1/4] pch_gbe: Fix the checksum fill to the error location

On 07/17/2012 03:09 PM, Eric Dumazet wrote:

On Mon, 2012-07-16 at 13:03 -0700, Andy Cress wrote:

quoted

Author: Zhong Hongbo [off-list ref]

Due to some unknown hardware limitations the pch_gbe hardware cannot
calculate checksums when the length of network package is less
than 64 bytes, where we will surprisingly encounter a problem of
the destination IP incorrectly changed.

When forwarding network packages at the network layer the IP packages
won't be relayed to the upper transport layer and analyzed there,
consequently, skb->transport_header pointer will be mistakenly remained
the same as that of skb->network_header, resulting in TCP checksum
wrongly
filled into the field of destination IP in IP header.

We can fix this issue by manually calculate the offset of the TCP
checksum
 and update it accordingly.

We would normally use the skb_checksum_start_offset(skb) here, but in
this
case it is sometimes -2 (csum_start=0 - skb_headroom=2 => -2), hence the
manual calculation.

Signed-off-by: Zhong Hongbo <redacted>
Merged-by: Andy Cress [off-list ref]

Hmm... I fail to understand why you care about NIC doing checksums,

Hi Eric,

When forwarding network packages at the network layer, the variable value of skb->transport_header is unknown. In my test, the variable value of skb->transport_header is equal to skb->network_header. So When you count the checksum as following:

offset = skb_transport_offset(skb);

skb->csum = skb_checksum(skb, offset, skb->len - offset, 0);
We should only count the TCP checksum, But it maybe include IP part.

tcp_hdr(skb)->check = csum_tcpudp_magic(iph->saddr, iph->daddr, skb->len - offset, IPPROTO_TCP, skb->csum);
We should fill the checksum in TCP package, But maybe fill it in other location and cover the useful information, such as source ip.

So We should count the TCP checksum and fill it in the correct location. Or else the forwarding network package will be drop for the error checksum.

while pch_gbe_tx_queue() make a _copy_ of each outgoing
packets.

There _must_ be a way to avoid most of these copies (ie not touching
payload), only mess with the header to insert these 2 nul bytes ?

This is other fix, my patch just fix checksum error issue.

Thanks,
hongbo

/* [Header:14][payload] ---> [Header:14][paddong:2][payload]    */

So at device setup : dev->needed_headroom = 2;

and in xmit,

	if (skb_headroom(skb) < 2) {
		struct sk_buff *skb_new;

		skb_new = skb_realloc_headroom(skb, 2);
		if (!skb_new) { handle error }
		consume_skb(skb);
		skb = skb_new;
	}
	ptr = skb_push(skb, 2);
	memmove(ptr, ptr + 2, ETH_HLEN);
	ptr[ETH_HLEN] = 0;
	ptr[ETH_HLEN + 1] = 0;