Herbert Xu wrote:
> On Mon, Jul 31, 2006 at 12:39:51PM +0200, Patrick McHardy wrote:
> 
>>Its actually not that much, if Herbert is fine with putting the
>>CHECKSUM_PARTIAL patch in 2.6.18 I'll do some more testing and
>>then I think these can go in as well.
> 
> 
> You guys know I'm a coward when it comes to pushing things into rc :)
> 
> So I'd rather see a patch to disable the warnings for 2.6.18 so that
> the proper fix can be tested more thoroughly.  We should remember that
> the 2.6.18 minus the warning is still going to be heaps better in this
> regard compared to 2.6.17 where all TSO packets were essentially
> discarded due to the incorrect checksum (when the NAT module is loaded).

I'm fine either way.

>>[NET]: Fix up CHECKSUM_PARTIAL patch for 2.6.18-rc3
>>
>>Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> 
> Please merge this with my earlier patch.  I'm not that fussed about
> having my changeset go in :)

Done.

>>diff --git a/net/ipv4/netfilter/ip_nat_core.c b/net/ipv4/netfilter/ip_nat_core.c
>>index 1741d55..731efbb 100644
>>--- a/net/ipv4/netfilter/ip_nat_core.c
>>+++ b/net/ipv4/netfilter/ip_nat_core.c
>>@@ -443,7 +443,9 @@ int ip_nat_icmp_reply_translation(struct
>> 
>> 	/* We're actually going to mangle it beyond trivial checksum
>> 	   adjustment, so make sure the current checksum is correct. */
>>-	if ((*pskb)->ip_summed != CHECKSUM_UNNECESSARY) {
>>+
>>+	if ((*pskb)->ip_summed != CHECKSUM_UNNECESSARY &&
>>+	    (*pskb)->ip_summed != CHECKSUM_PARTIAL) {
>> 		hdrlen = (*pskb)->nh.iph->ihl * 4;
>> 		if ((u16)csum_fold(skb_checksum(*pskb, hdrlen,
>> 						(*pskb)->len - hdrlen, 0)))
> 
> 
> Call me picky, but I'd prefer it to actually look like
> 
> 	switch ((*pskb)->ip_summed) {
> 	case CHECKSUM_COMPLETE:
> 		if (!(u16)csum_fold(skb->csum))
> 			break;
> 		/* fall through */
> 	case CHECKSUM_NONE:
> 		hdrlen = (*pskb)->nh.iph->ihl * 4;
> 		if ((u16)csum_fold(skb_checksum(*pskb, hdrlen,
> 						(*pskb)->len - hdrlen, 0)))
> 			return 0;
> 	}
> 
> just because we probably won't revisit this code path for another
> million years to add this optimisation :)

I've changed it to just use nf_ip_checksum instead (something I wanted
to do in a follow-up patch to keep this one as small as possible).
I'm also going to get rid of the remaining ip_nat_cheat_check copies
(there are quite a few of them).

> OK, this is so incredibly clever that I probably won't understand it
> until tomorrow :)
> 
> 
>>@@ -238,22 +248,30 @@ ip_nat_mangle_udp_packet(struct sk_buff 
>> 
>> 	iph = (*pskb)->nh.iph;
>> 	udph = (void *)iph + iph->ihl*4;
>>+
>>+	oldlen = (*pskb)->len - iph->ihl*4;
>> 	mangle_contents(*pskb, iph->ihl*4 + sizeof(*udph),
>> 			match_offset, match_len, rep_buffer, rep_len);
>> 
>> 	/* update the length of the UDP packet */
>>-	udph->len = htons((*pskb)->len - iph->ihl*4);
>>+	datalen = (*pskb)->len - iph->ihl*4;
>>+	udph->len = htons(datalen);
>> 
>> 	/* fix udp checksum if udp checksum was previously calculated */
>>-	if (udph->check) {
>>-		int datalen = (*pskb)->len - iph->ihl * 4;
>>+	if (!udph->check)
>>+		return 1;
> 
> 
> Just a quick thought, what if the partial checksum was zero?

Good point. I've fixed it to check for != CHECKSUM_PARTIAL here and
in the UDP NAT helper and also changed it to use -1 when the newly
calculated checksum is 0 so we don't accidentally turn off UDP
checksumming.

> I'm goint to review the rest of your patch tomorrow morning because
> I always fall sleep when looking at checksums :)

Same here, just woke up again :) New patches attached, with one
additional fix: the ip_queue patch tried to free the wrong packet
when queueing a generated segment failed.

I'm going to do some more testing now ..