ECN target bug report

ECN target bug report

[Andrea Rossato]

below you will find a coupe of emails sent to this list at the end of 
september.

the first one states that there is a problem with tcp checksum in the 
case a packet had been stripped of ecn bits. The problem was also 
reported by Graham Murray in Agust.

the answer was that this is due to tcpdump getting a cloned copy of the 
packet:
now, if I send tcp packet stripped with -ecn-tcp-remove to a box and i 
dump packets there, tcp checksum is incorrect and the box will be not 
respondig. If I remove the rule, packets are getting there with the 
correct checksum and the box responds.

What's interesting is that if I put these rules:

iptables -A OUTPUT -t mangle -o ppp0 -p tcp -d my.host.org --dport 80 -j 
ECN --ecn-tcp-remove
iptables -A OUTPUT -o ppp0 -p tcp -d my.host.org --dport 80 -m unclean 
-j DROP

packets will be actually dropped! something strange for being normal, 
isn't it? or iptables treats as unclean ecn stripped packets, and this 
is supposed to be normal? anyway iptables seems not to be the only one, 
so ECN target is actually preatty useless.

(using linux-2.4.20 and iptables-1.2.7a)

Thanks for you attention.
Andrea



> Subject:-j ECN --ecn-tcp-remove seems to be mangling the TCP checksum...
> From: netfilter@horizon.com
> Date: 27 Sep 2002 06:27:53 -0000
> To: netfilter@lists.netfilter.org

> bash-2.05b# iptables -t mangle -A fix-ecn -d 1.1.1.1 -p tcp -j ECN --ecn-tcp-remove
> bash-2.05b# echo 1 > /proc/sys/net/ipv4/tcp_ecn ; telnet 1.1.1.1 80
> 01:52:20.662338 science.horizon.com.11058 > 1.1.1.1.www: S [bad tcp cksum bf40!] 2655433521:2655433521(0) win 5840 <mss 1460,sackOK,timestamp 14290984 0,nop,wscale 0> (DF) [tos 0x10]  (ttl 64, id 41753, len 60)
>                          4510 003c a319 4000 4006 716c c023 6401
>                          0101 0101 2b32 0050 9e46 b331 0000 0000
>                          a002 16d0 3c55 0000 0204 05b4 0402 080a
>                          00da 1028 0000 0000 0103 0300
> 
> Now I'll turn tcp_ecn off again:
> bash-2.05b# echo 0 > /proc/sys/net/ipv4/tcp_ecn ; telnet 1.1.1.1 80
> 01:52:36.771155 science.horizon.com.11059 > 1.1.1.1.www: S [tcp sum ok] 2671050014:2671050014(0) win 5840 <mss 1460,sackOK,timestamp 14292595 0,nop,wscale 0> (DF) [tos 0x10]  (ttl 64, id 60269, len 60)
>                          4510 003c eb6d 4000 4006 2918 c023 6401
>                          0101 0101 2b33 0050 9f34 fd1e 0000 0000
>                          a002 16d0 2bed 0000 0204 05b4 0402 080a
>                          00da 1673 0000 0000 0103 0300
> 
> Notice the bad tcp checksum in the third case.

> Subject: Re: -j ECN --ecn-tcp-remove seems to be mangling the TCP checksum...
> From: Maciej Soltysiak <solt@dns.toxicfilms.tv>
> Date: Mon, 30 Sep 2002 11:55:56 +0200 (CEST)
> To: netfilter@horizon.com
> CC: netfilter@lists.netfilter.org

>>> Is this a bug?  The ipt_ECN.c file is
>>> ipt_ECN.c,v 1.4 2002/08/05 19:36:51 laforge Exp
> 
> No it is not. Do the same with a remote host.
> Send a ECNstripped packets to some other host, and tcpdump there.
> The checksum will be ok.
> It is the problem with tcpdump getting a cloned copy of the packet,
> read the RR's FIXME notes in netfilter sources about it.
> 
> I noticed that too, once, and thought it's a checksum calculation bug.
> Maciej Soltysiak

Re: ECN target bug report

[Andrea Rossato]

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

attached you will find 2 patches. the first one is just a hack that 
provides a solution, the second is an attempt to prove the existence of 
the problem.
this is far from being the solution, it's just a workaround: checksum of 
the packets will be recalculated from scratch. this means that unclean 
packets with ec e cwr bits set will be cleaned after passing through 
this rule. that's the reason for partial checksum recalculation: if a 
packet was unclean it must remain as such.

The problem, as far as I can see it, could be located in csum_partial 
(arch/i386/checksum.S): i'm not a kernel hacker (i'm a lawyer, a legal 
scholar actually), but i do not see any mistake in the way partial 
checksum is carried out in tcp_etc_set. anyway checksum after partial or 
total recalculation differ. That's a fact. Evidence of the fact  can be 
gained with the second patch: in this case the kernel will log the 
checksum after partial recalculation and after total recalculation (that 
means that two calculations will take place). The two values differ!

now, is this the right place for such a bug submission? is this code 
manteined? should i refer to the kernel mailing list?

If someone could give me directions I would very much appreciate. I 
would like to see this problem solved.
Thanks for your attention.

1. check the bug:
echo 1 /proc/sys/net/ipv4/tcp_ecn
iptables -A OUTPUT -t mangle -o ppp0 -p tcp -d my.host.org --dport 80 -j 
ECN --ecn-tcp-remove
iptables -A OUTPUT -o ppp0 -p tcp -d my.host.org --dport 80 -m unclean 
-j DROP
packets will be dropped


2. apply one of the patches and try again:
packets will get though and the connection will be established.

andrea

[-- Attachment #2: ecn_csum.patch --]
[-- Type: text/plain, Size: 1224 bytes --]

--- linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c.orig	2002-12-07 23:22:37.000000000 +0100
+++ linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c	2002-12-09 10:13:56.000000000 +0100
@@ -11,6 +11,7 @@
 #include <linux/skbuff.h>
 #include <linux/ip.h>
 #include <net/checksum.h>
+#include <net/tcp.h>
 
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv4/ipt_ECN.h>
@@ -62,6 +63,7 @@
 	struct tcphdr *tcph = (void *) iph + iph->ihl * 4;
 	u_int16_t *tcpflags = (u_int16_t *)tcph + 6;
 	u_int16_t diffs[2];
+	u_int32_t tcplen;
 
 	/* raw socket (tcpdump) may have clone of incoming
 	 * skb: don't disturb it --RR */
@@ -87,13 +89,14 @@
 	}
 	
 	if (diffs[0] != *tcpflags) {
-		diffs[0] = htons(diffs[0]) ^ 0xFFFF;
-		diffs[1] = htons(*tcpflags);
-		tcph->check = csum_fold(csum_partial((char *)diffs,
-		                                    sizeof(diffs),
-		                                    tcph->check^0xFFFF));
-		(*pskb)->nfcache |= NFC_ALTERED;
 
+		tcplen = (*pskb)->len - iph->ihl*4;
+		tcph->check = 0;
+		tcph->check = tcp_v4_check(tcph, tcplen, iph->saddr, iph->daddr,
+				   csum_partial((char *)tcph, tcph->doff*4,
+					   (*pskb)->csum));
+		(*pskb)->nfcache |= NFC_ALTERED;
+	
 		return 1;
 	}
 

[-- Attachment #3: ecn_csum_with_debug_info.patch --]
[-- Type: text/plain, Size: 1260 bytes --]

--- linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c.orig	2002-12-09 10:44:03.000000000 +0100
+++ linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c	2002-12-09 10:48:46.000000000 +0100
@@ -11,6 +11,7 @@
 #include <linux/skbuff.h>
 #include <linux/ip.h>
 #include <net/checksum.h>
+#include <net/tcp.h>
 
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv4/ipt_ECN.h>
@@ -62,6 +63,7 @@
 	struct tcphdr *tcph = (void *) iph + iph->ihl * 4;
 	u_int16_t *tcpflags = (u_int16_t *)tcph + 6;
 	u_int16_t diffs[2];
+	u_int32_t tcplen;
 
 	/* raw socket (tcpdump) may have clone of incoming
 	 * skb: don't disturb it --RR */
@@ -92,6 +94,17 @@
 		tcph->check = csum_fold(csum_partial((char *)diffs,
 		                                    sizeof(diffs),
 		                                    tcph->check^0xFFFF));
+
+		printk(KERN_WARNING "ECN: checksum after partial recalculation \"%x\"\n", tcph->check );
+		
+		tcplen = (*pskb)->len - iph->ihl*4;
+		tcph->check = 0;
+		tcph->check = tcp_v4_check(tcph, tcplen, iph->saddr, iph->daddr,
+				   csum_partial((char *)tcph, tcph->doff*4,
+					   (*pskb)->csum));
+
+		printk(KERN_WARNING "ECN: checksum after total recalculation \"%x\"\n", tcph->check );
+
 		(*pskb)->nfcache |= NFC_ALTERED;
 
 		return 1;

Re: ECN target bug report

[Andrea Rossato]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

attached you will find 2 patches. the first one is just a hack that
provides a solution, the second is an attempt to prove the existence of
the problem.
this is far from being the solution, it's just a workaround: checksum of
the packets will be recalculated from scratch. this means that unclean
packets with ec e cwr bits set will be cleaned after passing through
this rule. that's the reason for partial checksum recalculation: if a
packet was unclean it must remain as such.

The problem, as far as I can see it, could be located in csum_partial
(arch/i386/checksum.S): i'm not a kernel hacker (i'm a lawyer, a legal
scholar actually), but i do not see any mistake in the way partial
checksum is carried out in tcp_etc_set. anyway checksum after partial or
total recalculation differ. That's a fact. Evidence of the fact  can be
gained with the second patch: in this case the kernel will log the
checksum after partial recalculation and after total recalculation (that
means that two calculations will take place). The two values differ!

now, is this the right place for such a bug submission? is this code
manteined? should i refer to the kernel mailing list?

If someone could give me directions I would very much appreciate. I
would like to see this problem solved.
Thanks for your attention.

1. check the bug:
echo 1 /proc/sys/net/ipv4/tcp_ecn
iptables -A OUTPUT -t mangle -o ppp0 -p tcp -d my.host.org --dport 80 -j
ECN --ecn-tcp-remove
iptables -A OUTPUT -o ppp0 -p tcp -d my.host.org --dport 80 -m unclean
-j DROP
packets will be dropped


2. apply one of the patches and try again:
packets will get though and the connection will be established.

andrea

please send me a cc as I'm not a member of this list.

[-- Attachment #2: ecn_csum.patch --]
[-- Type: text/plain, Size: 1225 bytes --]

--- linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c.orig	2002-12-07 23:22:37.000000000 +0100
+++ linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c	2002-12-09 10:13:56.000000000 +0100
@@ -11,6 +11,7 @@
 #include <linux/skbuff.h>
 #include <linux/ip.h>
 #include <net/checksum.h>
+#include <net/tcp.h>
 
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv4/ipt_ECN.h>
@@ -62,6 +63,7 @@
 	struct tcphdr *tcph = (void *) iph + iph->ihl * 4;
 	u_int16_t *tcpflags = (u_int16_t *)tcph + 6;
 	u_int16_t diffs[2];
+	u_int32_t tcplen;
 
 	/* raw socket (tcpdump) may have clone of incoming
 	 * skb: don't disturb it --RR */
@@ -87,13 +89,14 @@
 	}
 	
 	if (diffs[0] != *tcpflags) {
-		diffs[0] = htons(diffs[0]) ^ 0xFFFF;
-		diffs[1] = htons(*tcpflags);
-		tcph->check = csum_fold(csum_partial((char *)diffs,
-		                                    sizeof(diffs),
-		                                    tcph->check^0xFFFF));
-		(*pskb)->nfcache |= NFC_ALTERED;
 
+		tcplen = (*pskb)->len - iph->ihl*4;
+		tcph->check = 0;
+		tcph->check = tcp_v4_check(tcph, tcplen, iph->saddr, iph->daddr,
+				   csum_partial((char *)tcph, tcph->doff*4,
+					   (*pskb)->csum));
+		(*pskb)->nfcache |= NFC_ALTERED;
+	
 		return 1;
 	}
 


[-- Attachment #3: ecn_csum_with_debug_info.patch --]
[-- Type: text/plain, Size: 1261 bytes --]

--- linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c.orig	2002-12-09 10:44:03.000000000 +0100
+++ linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c	2002-12-09 10:48:46.000000000 +0100
@@ -11,6 +11,7 @@
 #include <linux/skbuff.h>
 #include <linux/ip.h>
 #include <net/checksum.h>
+#include <net/tcp.h>
 
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv4/ipt_ECN.h>
@@ -62,6 +63,7 @@
 	struct tcphdr *tcph = (void *) iph + iph->ihl * 4;
 	u_int16_t *tcpflags = (u_int16_t *)tcph + 6;
 	u_int16_t diffs[2];
+	u_int32_t tcplen;
 
 	/* raw socket (tcpdump) may have clone of incoming
 	 * skb: don't disturb it --RR */
@@ -92,6 +94,17 @@
 		tcph->check = csum_fold(csum_partial((char *)diffs,
 		                                    sizeof(diffs),
 		                                    tcph->check^0xFFFF));
+
+		printk(KERN_WARNING "ECN: checksum after partial recalculation \"%x\"\n", tcph->check );
+		
+		tcplen = (*pskb)->len - iph->ihl*4;
+		tcph->check = 0;
+		tcph->check = tcp_v4_check(tcph, tcplen, iph->saddr, iph->daddr,
+				   csum_partial((char *)tcph, tcph->doff*4,
+					   (*pskb)->csum));
+
+		printk(KERN_WARNING "ECN: checksum after total recalculation \"%x\"\n", tcph->check );
+
 		(*pskb)->nfcache |= NFC_ALTERED;
 
 		return 1;

Re: ECN target bug report

[Andrea Rossato]

[-- Attachment #1: Type: text/plain, Size: 340 bytes --]

attached you will find what could be a suitable, even though temporary, 
solution for ECN target.
a packet with ec and cwr bits set and a bad checksum will not be 
processed. If the checksum is good the bits will be stripped.
andrea

-- 


~  GnuGP public key for arossato@istitutocolli.org
~      http://www.istitutocolli.org/pubkey.asc



[-- Attachment #2: ecn_checksum.patch --]
[-- Type: text/plain, Size: 1520 bytes --]

--- linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c.orig	2002-12-09 10:44:03.000000000 +0100
+++ linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c	2002-12-09 17:16:11.000000000 +0100
@@ -11,6 +11,7 @@
 #include <linux/skbuff.h>
 #include <linux/ip.h>
 #include <net/checksum.h>
+#include <net/tcp.h>
 
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv4/ipt_ECN.h>
@@ -62,6 +63,7 @@
 	struct tcphdr *tcph = (void *) iph + iph->ihl * 4;
 	u_int16_t *tcpflags = (u_int16_t *)tcph + 6;
 	u_int16_t diffs[2];
+	u_int32_t tcplen;
 
 	/* raw socket (tcpdump) may have clone of incoming
 	 * skb: don't disturb it --RR */
@@ -74,6 +76,15 @@
 		iph = (*pskb)->nh.iph;
 	}
 
+
+	/* Checksum invalid?  Ignore. */
+	/* FIXME: Source route IP option packets --RR */
+	tcplen = (*pskb)->len - iph->ihl*4;
+	if (tcp_v4_check(tcph, tcplen, iph->saddr, iph->daddr,
+			 csum_partial((char *) tcph, tcplen, 0))) {
+		return 0;
+	}
+
 	diffs[0] = *tcpflags;
 
 	if (einfo->operation & IPT_ECN_OP_SET_ECE
@@ -87,13 +98,12 @@
 	}
 	
 	if (diffs[0] != *tcpflags) {
-		diffs[0] = htons(diffs[0]) ^ 0xFFFF;
-		diffs[1] = htons(*tcpflags);
-		tcph->check = csum_fold(csum_partial((char *)diffs,
-		                                    sizeof(diffs),
-		                                    tcph->check^0xFFFF));
+		tcph->check = 0;
+		tcph->check = tcp_v4_check(tcph, tcplen, iph->saddr, iph->daddr,
+				   csum_partial((char *)tcph, tcph->doff*4,
+					   (*pskb)->csum));
 		(*pskb)->nfcache |= NFC_ALTERED;
-
+	
 		return 1;
 	}
 

Re: ECN target bug report

[Andrea Rossato]

[-- Attachment #1: Type: text/plain, Size: 262 bytes --]

attached you will find what could be a suitable, even though temporary,
solution for ECN target.
a packet with ec and cwr bits set and a bad checksum will not be
processed. If the checksum is good the bits will be stripped and a new 
checksum calculated.
andrea

[-- Attachment #2: ecn_checksum.patch --]
[-- Type: text/plain, Size: 1521 bytes --]

--- linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c.orig	2002-12-09 10:44:03.000000000 +0100
+++ linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c	2002-12-09 17:16:11.000000000 +0100
@@ -11,6 +11,7 @@
 #include <linux/skbuff.h>
 #include <linux/ip.h>
 #include <net/checksum.h>
+#include <net/tcp.h>
 
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv4/ipt_ECN.h>
@@ -62,6 +63,7 @@
 	struct tcphdr *tcph = (void *) iph + iph->ihl * 4;
 	u_int16_t *tcpflags = (u_int16_t *)tcph + 6;
 	u_int16_t diffs[2];
+	u_int32_t tcplen;
 
 	/* raw socket (tcpdump) may have clone of incoming
 	 * skb: don't disturb it --RR */
@@ -74,6 +76,15 @@
 		iph = (*pskb)->nh.iph;
 	}
 
+
+	/* Checksum invalid?  Ignore. */
+	/* FIXME: Source route IP option packets --RR */
+	tcplen = (*pskb)->len - iph->ihl*4;
+	if (tcp_v4_check(tcph, tcplen, iph->saddr, iph->daddr,
+			 csum_partial((char *) tcph, tcplen, 0))) {
+		return 0;
+	}
+
 	diffs[0] = *tcpflags;
 
 	if (einfo->operation & IPT_ECN_OP_SET_ECE
@@ -87,13 +98,12 @@
 	}
 	
 	if (diffs[0] != *tcpflags) {
-		diffs[0] = htons(diffs[0]) ^ 0xFFFF;
-		diffs[1] = htons(*tcpflags);
-		tcph->check = csum_fold(csum_partial((char *)diffs,
-		                                    sizeof(diffs),
-		                                    tcph->check^0xFFFF));
+		tcph->check = 0;
+		tcph->check = tcp_v4_check(tcph, tcplen, iph->saddr, iph->daddr,
+				   csum_partial((char *)tcph, tcph->doff*4,
+					   (*pskb)->csum));
 		(*pskb)->nfcache |= NFC_ALTERED;
-
+	
 		return 1;
 	}
 

Re: ECN target bug report

[Andrea Rossato]

[-- Attachment #1: Type: text/plain, Size: 1307 bytes --]

attached you will find 3 patches. the first one is an attempt to prove 
the existence of the problem. the second is just a hack that provides a 
temporary solution it's just a workaround: packets with ec e cwr bits 
set and good checksum will be stripped and checksum recalculated from 
scratch.

The problem, as far as I can see it, could be located in csum_partial 
(arch/i386/lib/checksum.S, meaning a platform dependent problem): i'm 
not a kernel hacker (i'm a lawyer, a legal scholar actually), but i do 
not see any mistake in the way partial checksum is carried out in 
tcp_etc_set. anyway checksum after partial or total recalculation 
differ. That's a fact. Evidence of the fact  can be gained with the 
second patch: in this case the kernel will log the checksum after 
partial recalculation and after total recalculation (that means that two 
calculations will take place). The two values differ!

Thanks for your attention.

1. check the bug:
echo 1 /proc/sys/net/ipv4/tcp_ecn
iptables -A OUTPUT -t mangle -o ppp0 -p tcp -d my.host.org --dport 80 -j 
ECN --ecn-tcp-remove
iptables -A OUTPUT -o ppp0 -p tcp -d my.host.org --dport 80 -m unclean 
-j DROP
packets will be dropped

2. apply one of the patches and try again:
packets will get though and the connection will be established.

andrea



[-- Attachment #2: ecn_csum_with_debug_info.patch --]
[-- Type: text/plain, Size: 1262 bytes --]

--- linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c.orig	2002-12-09 10:44:03.000000000 +0100
+++ linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c	2002-12-09 10:48:46.000000000 +0100
@@ -11,6 +11,7 @@
 #include <linux/skbuff.h>
 #include <linux/ip.h>
 #include <net/checksum.h>
+#include <net/tcp.h>
 
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv4/ipt_ECN.h>
@@ -62,6 +63,7 @@
 	struct tcphdr *tcph = (void *) iph + iph->ihl * 4;
 	u_int16_t *tcpflags = (u_int16_t *)tcph + 6;
 	u_int16_t diffs[2];
+	u_int32_t tcplen;
 
 	/* raw socket (tcpdump) may have clone of incoming
 	 * skb: don't disturb it --RR */
@@ -92,6 +94,17 @@
 		tcph->check = csum_fold(csum_partial((char *)diffs,
 		                                    sizeof(diffs),
 		                                    tcph->check^0xFFFF));
+
+		printk(KERN_WARNING "ECN: checksum after partial recalculation \"%x\"\n", tcph->check );
+		
+		tcplen = (*pskb)->len - iph->ihl*4;
+		tcph->check = 0;
+		tcph->check = tcp_v4_check(tcph, tcplen, iph->saddr, iph->daddr,
+				   csum_partial((char *)tcph, tcph->doff*4,
+					   (*pskb)->csum));
+
+		printk(KERN_WARNING "ECN: checksum after total recalculation \"%x\"\n", tcph->check );
+
 		(*pskb)->nfcache |= NFC_ALTERED;
 
 		return 1;



[-- Attachment #3: ecn_checksum.patch --]
[-- Type: text/plain, Size: 1520 bytes --]

--- linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c.orig	2002-12-09 10:44:03.000000000 +0100
+++ linux-2.4.20/net/ipv4/netfilter/ipt_ECN.c	2002-12-09 17:16:11.000000000 +0100
@@ -11,6 +11,7 @@
 #include <linux/skbuff.h>
 #include <linux/ip.h>
 #include <net/checksum.h>
+#include <net/tcp.h>
 
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv4/ipt_ECN.h>
@@ -62,6 +63,7 @@
 	struct tcphdr *tcph = (void *) iph + iph->ihl * 4;
 	u_int16_t *tcpflags = (u_int16_t *)tcph + 6;
 	u_int16_t diffs[2];
+	u_int32_t tcplen;
 
 	/* raw socket (tcpdump) may have clone of incoming
 	 * skb: don't disturb it --RR */
@@ -74,6 +76,15 @@
 		iph = (*pskb)->nh.iph;
 	}
 
+
+	/* Checksum invalid?  Ignore. */
+	/* FIXME: Source route IP option packets --RR */
+	tcplen = (*pskb)->len - iph->ihl*4;
+	if (tcp_v4_check(tcph, tcplen, iph->saddr, iph->daddr,
+			 csum_partial((char *) tcph, tcplen, 0))) {
+		return 0;
+	}
+
 	diffs[0] = *tcpflags;
 
 	if (einfo->operation & IPT_ECN_OP_SET_ECE
@@ -87,13 +98,12 @@
 	}
 	
 	if (diffs[0] != *tcpflags) {
-		diffs[0] = htons(diffs[0]) ^ 0xFFFF;
-		diffs[1] = htons(*tcpflags);
-		tcph->check = csum_fold(csum_partial((char *)diffs,
-		                                    sizeof(diffs),
-		                                    tcph->check^0xFFFF));
+		tcph->check = 0;
+		tcph->check = tcp_v4_check(tcph, tcplen, iph->saddr, iph->daddr,
+				   csum_partial((char *)tcph, tcph->doff*4,
+					   (*pskb)->csum));
 		(*pskb)->nfcache |= NFC_ALTERED;
-
+	
 		return 1;
 	}
 

Re: ECN target bug report

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]

Hi Andrea,

the first attached patch fixes the issue you reported (verified), 
altough i'm not entirely sure why ;).
the second one is untested but probably couldn't hurt neither.

bye,
patrick

Andrea Rossato wrote:

> attached you will find 3 patches. the first one is an attempt to prove 
> the existence of the problem. the second is just a hack that provides 
> a temporary solution it's just a workaround: packets with ec e cwr 
> bits set and good checksum will be stripped and checksum recalculated 
> from scratch.
>
> The problem, as far as I can see it, could be located in csum_partial 
> (arch/i386/lib/checksum.S, meaning a platform dependent problem): i'm 
> not a kernel hacker (i'm a lawyer, a legal scholar actually), but i do 
> not see any mistake in the way partial checksum is carried out in 
> tcp_etc_set. anyway checksum after partial or total recalculation 
> differ. That's a fact. Evidence of the fact  can be gained with the 
> second patch: in this case the kernel will log the checksum after 
> partial recalculation and after total recalculation (that means that 
> two calculations will take place). The two values differ!
>
> Thanks for your attention.
>
> 1. check the bug:
> echo 1 /proc/sys/net/ipv4/tcp_ecn
> iptables -A OUTPUT -t mangle -o ppp0 -p tcp -d my.host.org --dport 80 
> -j ECN --ecn-tcp-remove
> iptables -A OUTPUT -o ppp0 -p tcp -d my.host.org --dport 80 -m unclean 
> -j DROP
> packets will be dropped
>
> 2. apply one of the patches and try again:
> packets will get though and the connection will be established.
>
> andrea
>

[-- Attachment #2: ipt_ECN.diff.1 --]
[-- Type: text/plain, Size: 499 bytes --]

--- net/ipv4/netfilter/ipt_ECN.c.orig	2002-12-09 23:14:20.000000000 +0100
+++ net/ipv4/netfilter/ipt_ECN.c	2002-12-09 23:13:27.000000000 +0100
@@ -88,8 +88,8 @@
 	}
 	
 	if (diffs[0] != *tcpflags) {
-		diffs[0] = htons(diffs[0]) ^ 0xFFFF;
-		diffs[1] = htons(*tcpflags);
+		diffs[0] = diffs[0] ^ 0xFFFF;
+		diffs[1] = *tcpflags;
 		tcph->check = csum_fold(csum_partial((char *)diffs,
 		                                    sizeof(diffs),
 		                                    tcph->check^0xFFFF));

[-- Attachment #3: ipt_ECN.diff.2 --]
[-- Type: text/plain, Size: 594 bytes --]

--- net/ipv4/netfilter/ipt_ECN.c.orig	2002-12-09 23:14:20.000000000 +0100
+++ net/ipv4/netfilter/ipt_ECN.c	2002-12-09 23:25:54.000000000 +0100
@@ -41,10 +41,10 @@
 			iph = (*pskb)->nh.iph;
 		}
 
-		diffs[0] = htons(iph->tos) ^ 0xFFFF;
+		diffs[0] = iph->tos ^ 0xFF;
 		iph->tos = iph->tos & ~IPT_ECN_IP_MASK;
 		iph->tos = iph->tos | (einfo->ip_ect & IPT_ECN_IP_MASK);
-		diffs[1] = htons(iph->tos);
+		diffs[1] = iph->tos;
 		iph->check = csum_fold(csum_partial((char *)diffs,
 		                                    sizeof(diffs),
 		                                    iph->check^0xFFFF));

Re: ECN target bug report

[Andrea Rossato]

Patrick McHardy wrote:
> the first attached patch fixes the issue you reported (verified), 
> altough i'm not entirely sure why ;).
> the second one is untested but probably couldn't hurt neither.

great patrick! faster then light!

BTW, your second patch seems to imply that byte order is just a matter 
of taste ;)

iph and tcph should give little endian int on i386 arch, so tons should 
be needed when writing to the sk_buff. but in computer programming there 
are misteries a lawyar will never be able to grasp...

thanks a lot.
andrea

Re: ECN target bug report

[Patrick McHardy]

Hi,

Andrea Rossato wrote:

> Patrick McHardy wrote:
>
>> the first attached patch fixes the issue you reported (verified), 
>> altough i'm not entirely sure why ;).
>> the second one is untested but probably couldn't hurt neither.
>
>
> great patrick! faster then light!
>
> BTW, your second patch seems to imply that byte order is just a matter 
> of taste ;)
>
> iph and tcph should give little endian int on i386 arch, so tons 
> should be needed when writing to the sk_buff. but in computer 
> programming there are misteries a lawyar will never be able to grasp...

I thought calculating checksum for x and htons(x) should give me same 
result, obiously that is wrong (which i learned after trying it out).
For the first patch: *tcpflags are bitfields (position declared 
dependant of endianess), so no htons here.
For the second patch, it's working with or without, although i think the 
correct way is with the patch:

u_int16_t diffs[2];
diffs[0] = htons(iph->tos) ^ 0xFFFF;
...
diffs[1] = htons(iph->tos);
...

iph->tos is of type u_int8_t. so no htons is necessary.

Bye,
Patrick

Re: ECN target bug report

[Andrea Rossato]

Patrick McHardy wrote:
> I thought calculating checksum for x and htons(x) should give me same 
> result, obiously that is wrong (which i learned after trying it out).
> For the first patch: *tcpflags are bitfields (position declared 
> dependant of endianess), so no htons here.
> For the second patch, it's working with or without, although i think the 
> correct way is with the patch:
> 
> u_int16_t diffs[2];
> diffs[0] = htons(iph->tos) ^ 0xFFFF;
> ...
> diffs[1] = htons(iph->tos);
> ...
> 
> iph->tos is of type u_int8_t. so no htons is necessary.


I know netiquette suggest not to send messages just to thank...but i 
spet the whole afternoon trying to understand when exactly host-to-net 
byte order translation was needed without results!
Now I've got it!!
Thanks,
andrea

Re: ECN target bug report

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

On Mon, Dec 09, 2002 at 10:19:06PM +0100, Andrea Rossato wrote:
> attached you will find 3 patches. the first one is an attempt to prove 
> the existence of the problem. 

> the second is just a hack that provides a temporary solution it's just
> a workaround: packets with ec e cwr bits set and good checksum will be
> stripped and checksum recalculated from scratch.

I agree that this is not 

> The problem, as far as I can see it, could be located in csum_partial 
> (arch/i386/lib/checksum.S, meaning a platform dependent problem): i'm 
> not a kernel hacker (i'm a lawyer, a legal scholar actually), but i do 

hey, usually people say IANAL, but you could say IANAP ;)

> not see any mistake in the way partial checksum is carried out in 
> tcp_etc_set. anyway checksum after partial or total recalculation 
> differ. That's a fact. Evidence of the fact  can be gained with the 
> second patch: in this case the kernel will log the checksum after 
> partial recalculation and after total recalculation (that means that two 
> calculations will take place). The two values differ!

It's most certainly not a bug in csum_partial(), since this function is
used all over the place, including other targets like TTL and even the
NAT core itself.

I think there is an error in the way I used the function to recalculate
the TCP checksum.  I'll have a look.

> Thanks for your attention.

Thanks for helping us!

> andrea

-- 
- Harald Welte / laforge@gnumonks.org               http://www.gnumonks.org/
============================================================================
"If this were a dictatorship, it'd be a heck of a lot easier, just so long
 as I'm the dictator."  --  George W. Bush Dec 18, 2000

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]