TCP window tracking has bad side effects

TCP window tracking has bad side effects

[Ludwig Nussel]

Hi,

Recent state matching code apparently added some kind of TCP window
tracking which marks out of sequence packets as INVALID.

Previously one could use some minimal filter rules like this on a
client machine:

iptables -F 
iptables -X 
iptables -P INPUT DROP 
iptables -P FORWARD DROP 
iptables -P OUTPUT ACCEPT 
iptables -A INPUT -j ACCEPT -i lo 
iptables -A INPUT -j ACCEPT -m state --state ESTABLISHED,RELATED 


With TCP window tracking those rules no longer work for services
that use fixed ports (e.g. NFS) and one side crashes or terminates
the connection in other ways without notifying the peer (e.g. link
down). When the crashed machine comes up again and tries to
reestablish the connection it sends a SYN. The remote end finds that
confusing and replies with an ACK as probe. Since that ACK does not
fit any window it's discarded as INVALID. The remote side can now
sit there forever sending ACKs and no new connection can be
established. Previously, without window tracking, the ACK was
accepted and answered with RST, the remote closed the connection and
a new one could be established. 

Is there a way to disable the window tracking and revert to the old
behavior?

cu
Ludwig

-- 
 (o_   Ludwig Nussel
 //\   SUSE LINUX AG, Development
 V_/_  http://www.suse.de/

Re: TCP window tracking has bad side effects

[Jozsef Kadlecsik]

Hi,

On Wed, 1 Dec 2004, Ludwig Nussel wrote:

> Recent state matching code apparently added some kind of TCP window
> tracking which marks out of sequence packets as INVALID.
>
> Previously one could use some minimal filter rules like this on a
> client machine:
>
> iptables -F
> iptables -X
> iptables -P INPUT DROP
> iptables -P FORWARD DROP
> iptables -P OUTPUT ACCEPT
> iptables -A INPUT -j ACCEPT -i lo
> iptables -A INPUT -j ACCEPT -m state --state ESTABLISHED,RELATED
>
>
> With TCP window tracking those rules no longer work for services
> that use fixed ports (e.g. NFS) and one side crashes or terminates
> the connection in other ways without notifying the peer (e.g. link
> down). When the crashed machine comes up again and tries to
> reestablish the connection it sends a SYN. The remote end finds that
> confusing and replies with an ACK as probe. Since that ACK does not
> fit any window it's discarded as INVALID.

The remote end must send an ACK segment which is in the window (see
RFC793, p68), thus the window tracking code could let it through.

> The remote side can now
> sit there forever sending ACKs and no new connection can be
> established. Previously, without window tracking, the ACK was
> accepted and answered with RST, the remote closed the connection and
> a new one could be established.
>
> Is there a way to disable the window tracking and revert to the old
> behavior?

Yes, you can disable it anytime:

echo 1 > /proc/sys/net/ipv4/netfilter/ip_conntrack_tcp_be_liberal

But a full tcpdump from such a session and the log entries on the
invalid packets would be useful for us to recheck the code.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: TCP window tracking has bad side effects

[Ludwig Nussel]

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

Jozsef Kadlecsik wrote:
> On Wed, 1 Dec 2004, Ludwig Nussel wrote:
> > With TCP window tracking those rules no longer work for services
> > that use fixed ports (e.g. NFS) and one side crashes or terminates
> > the connection in other ways without notifying the peer (e.g. link
> > down). When the crashed machine comes up again and tries to
> > reestablish the connection it sends a SYN. The remote end finds that
> > confusing and replies with an ACK as probe. Since that ACK does not
> > fit any window it's discarded as INVALID.
> 
> The remote end must send an ACK segment which is in the window (see
> RFC793, p68), thus the window tracking code could let it through.

My description probably wasn't unambiguous. The client has the
packetfilter, crashes and reboots. The server does not notice and
just sees an out of sequence SYN for an existing connection (same
ip/port). The server responds with an ACK which contains the
sequence numbers it expects for that connection (p36 in above cited
rfc). On the client side this ACK does not belong to the SYN it sent
and is discarded whereas it should be answered with RST.

> > The remote side can now
> > sit there forever sending ACKs and no new connection can be
> > established. Previously, without window tracking, the ACK was
> > accepted and answered with RST, the remote closed the connection and
> > a new one could be established.
> >
> > Is there a way to disable the window tracking and revert to the old
> > behavior?
> 
> Yes, you can disable it anytime:
> 
> echo 1 > /proc/sys/net/ipv4/netfilter/ip_conntrack_tcp_be_liberal

Doesn't help.

> But a full tcpdump from such a session and the log entries on the
> invalid packets would be useful for us to recheck the code.

tcpdump file attached. 192.168.42.1 is the server and 192.168.42.2
the client with packetfilter.

The log of a dropped ACK packet looks like this:

SRC=192.168.42.1 DST=192.168.42.2 LEN=52 TOS=0x00 PREC=0x00 TTL=64 ID=26022 DF PROTO=TCP SPT=9999 DPT=8888 WINDOW=1448 RES=0x00 ACK URGP=0 OPT (0101080A015EB588FFFD6ED1)

cu
Ludwig

-- 
 (o_   Ludwig Nussel
 //\   SUSE LINUX Products GmbH, Development
 V_/_  http://www.suse.de/

[-- Attachment #2: tcpdump46818 --]
[-- Type: application/octet-stream, Size: 975 bytes --]

Re: TCP window tracking has bad side effects

[Phil Oester]

On Wed, Dec 01, 2004 at 01:16:31PM +0100, Jozsef Kadlecsik wrote:
> Yes, you can disable it anytime:
> 
> echo 1 > /proc/sys/net/ipv4/netfilter/ip_conntrack_tcp_be_liberal
> 
> But a full tcpdump from such a session and the log entries on the
> invalid packets would be useful for us to recheck the code.

This sounds remarkably similar to bugzilla #258, where a TCP
session which works in 2.6.8.1 fails in 2.6.9:

https://bugzilla.netfilter.org/bugzilla/show_bug.cgi?id=258

Attached to the report is a binary tcpdump.  I've replayed it
and tried a few things but can't figure out what the problem
is.

Disabling window tracking did nothing -- were there any other
interesting changes in 2.6.9 series?

Phil

Re: TCP window tracking has bad side effects

[Jozsef Kadlecsik]

On Wed, 1 Dec 2004, Ludwig Nussel wrote:

> My description probably wasn't unambiguous. The client has the
> packetfilter, crashes and reboots.

I see - that case was not handled in the code. Please try the patch below
and report the result.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

diff -urN --exclude-from=/usr/src/diff.exclude linux-2.6.9-orig/net/ipv4/netfilter/ip_conntrack_proto_tcp.c linux-2.6.9-tcp-win/net/ipv4/netfilter/ip_conntrack_proto_tcp.c
--- linux-2.6.9-orig/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-10-18 23:55:29.000000000 +0200
+++ linux-2.6.9-tcp-win/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-12-03 07:06:55.000000000 +0100
@@ -275,7 +275,7 @@
 /* 	     sNO, sSS, sSR, sES, sFW, sCW, sLA, sTW, sCL, sLI	*/
 /*ack*/	   { sIV, sIV, sIV, sES, sCW, sCW, sTW, sTW, sCL, sIV },
 /*
- *	sSS -> sIV	ACK is invalid: we haven't seen a SYN/ACK yet.
+ *	sSS -> sIG	Might be a half-open connection.
  *	sSR -> sIV	Simultaneous open.
  *	sES -> sES	:-)
  *	sFW -> sCW	Normal close request answered by ACK.
@@ -847,7 +847,9 @@

 	switch (new_state) {
 	case TCP_CONNTRACK_IGNORE:
-		/* Either SYN in ORIGINAL, or SYN/ACK in REPLY direction. */
+		/* Either SYN in ORIGINAL
+		 * or SYN/ACK in REPLY
+		 * or ACK in REPLY direction. */
 		if (index == TCP_SYNACK_SET
 		    && conntrack->proto.tcp.last_index == TCP_SYN_SET
 		    && conntrack->proto.tcp.last_dir != dir
@@ -876,7 +878,7 @@
 		WRITE_UNLOCK(&tcp_lock);
 		if (LOG_INVALID(IPPROTO_TCP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL,
-				  "ip_ct_tcp: invalid SYN (ignored) ");
+				  "ip_ct_tcp: invalid packet ignored ");
 		return NF_ACCEPT;
 	case TCP_CONNTRACK_MAX:
 		/* Invalid packet */
@@ -901,11 +903,11 @@
 		break;
 	case TCP_CONNTRACK_CLOSE:
 		if (index == TCP_RST_SET
-		    && test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status)
-		    && conntrack->proto.tcp.last_index <= TCP_SYNACK_SET
+		    && (conntrack->proto.tcp.last_index <= TCP_SYNACK_SET
+		        || conntrack->proto.tcp.last_index == TCP_ACK_SET)
 		    && after(ntohl(th->ack_seq),
 		    	     conntrack->proto.tcp.last_seq)) {
-			/* Ignore RST closing down invalid SYN
+			/* Ignore RST closing down invalid SYN or ACK
 			   we had let trough. */
 		    	WRITE_UNLOCK(&tcp_lock);
 			if (LOG_INVALID(IPPROTO_TCP))

Re: TCP window tracking has bad side effects

[Jozsef Kadlecsik]

On Fri, 3 Dec 2004, Jozsef Kadlecsik wrote:

> On Wed, 1 Dec 2004, Ludwig Nussel wrote:
>
> > My description probably wasn't unambiguous. The client has the
> > packetfilter, crashes and reboots.
>
> I see - that case was not handled in the code. Please try the patch below
> and report the result.

Oh, I'm sorry that was an incomplete patch. This is the fixed code, please
give it a try.

diff -urN --exclude-from=/usr/src/diff.exclude linux-2.6.9-orig/include/linux/netfilter_ipv4/ip_conntrack_tcp.h linux-2.6.9-tcp-win/include/linux/netfilter_ipv4/ip_conntrack_tcp.h
--- linux-2.6.9-orig/include/linux/netfilter_ipv4/ip_conntrack_tcp.h	2004-10-18 23:53:51.000000000 +0200
+++ linux-2.6.9-tcp-win/include/linux/netfilter_ipv4/ip_conntrack_tcp.h	2004-12-06 06:52:07.000000000 +0100
@@ -18,7 +18,7 @@
 };

 /* Window scaling is advertised by the sender */
-#define IP_CT_TCP_STATE_FLAG_WINDOW_SCALE	0x01
+#define IP_CT_TCP_FLAG_WINDOW_SCALE		0x01

 /* SACK is permitted by the sender */
 #define IP_CT_TCP_FLAG_SACK_PERM		0x02
diff -urN --exclude-from=/usr/src/diff.exclude linux-2.6.9-orig/net/ipv4/netfilter/ip_conntrack_proto_tcp.c linux-2.6.9-tcp-win/net/ipv4/netfilter/ip_conntrack_proto_tcp.c
--- linux-2.6.9-orig/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-10-18 23:55:29.000000000 +0200
+++ linux-2.6.9-tcp-win/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-12-06 07:03:52.000000000 +0100
@@ -273,9 +273,9 @@
  *	sCL -> sCL
  */
 /* 	     sNO, sSS, sSR, sES, sFW, sCW, sLA, sTW, sCL, sLI	*/
-/*ack*/	   { sIV, sIV, sIV, sES, sCW, sCW, sTW, sTW, sCL, sIV },
+/*ack*/	   { sIV, sIG, sIV, sES, sCW, sCW, sTW, sTW, sCL, sIV },
 /*
- *	sSS -> sIV	ACK is invalid: we haven't seen a SYN/ACK yet.
+ *	sSS -> sIG	Might be a half-open connection.
  *	sSR -> sIV	Simultaneous open.
  *	sES -> sES	:-)
  *	sFW -> sCW	Normal close request answered by ACK.
@@ -436,7 +436,7 @@
 					state->td_scale = 14;
 				}
 				state->flags |=
-					IP_CT_TCP_STATE_FLAG_WINDOW_SCALE;
+					IP_CT_TCP_FLAG_WINDOW_SCALE;
 			}
 			ptr += opsize - 2;
 			length -= opsize;
@@ -552,8 +552,8 @@
 			 * Both sides must send the Window Scale option
 			 * to enable window scaling in either direction.
 			 */
-			if (!(sender->flags & IP_CT_TCP_STATE_FLAG_WINDOW_SCALE
-			      && receiver->flags & IP_CT_TCP_STATE_FLAG_WINDOW_SCALE))
+			if (!(sender->flags & IP_CT_TCP_FLAG_WINDOW_SCALE
+			      && receiver->flags & IP_CT_TCP_FLAG_WINDOW_SCALE))
 				sender->td_scale =
 				receiver->td_scale = 0;
 		} else {
@@ -566,9 +566,11 @@
 			sender->td_maxwin = (win == 0 ? 1 : win);
 			sender->td_maxend = end + sender->td_maxwin;
 		}
-	} else if (state->state == TCP_CONNTRACK_SYN_SENT
-		   && dir == IP_CT_DIR_ORIGINAL
-		   && after(end, sender->td_end)) {
+	} else if (((state->state == TCP_CONNTRACK_SYN_SENT
+		     && dir == IP_CT_DIR_ORIGINAL)
+		    || (state->state == TCP_CONNTRACK_SYN_RECV
+		        && dir == IP_CT_DIR_REPLY))
+		    && after(end, sender->td_end)) {
 		/*
 		 * RFC 793: "if a TCP is reinitialized ... then it need
 		 * not wait at all; it must only be sure to use sequence
@@ -847,7 +849,9 @@

 	switch (new_state) {
 	case TCP_CONNTRACK_IGNORE:
-		/* Either SYN in ORIGINAL, or SYN/ACK in REPLY direction. */
+		/* Either SYN in ORIGINAL
+		 * or SYN/ACK in REPLY
+		 * or ACK in REPLY direction (half-open connection). */
 		if (index == TCP_SYNACK_SET
 		    && conntrack->proto.tcp.last_index == TCP_SYN_SET
 		    && conntrack->proto.tcp.last_dir != dir
@@ -876,7 +880,7 @@
 		WRITE_UNLOCK(&tcp_lock);
 		if (LOG_INVALID(IPPROTO_TCP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL,
-				  "ip_ct_tcp: invalid SYN (ignored) ");
+				  "ip_ct_tcp: invalid packet ignored ");
 		return NF_ACCEPT;
 	case TCP_CONNTRACK_MAX:
 		/* Invalid packet */
@@ -901,11 +905,11 @@
 		break;
 	case TCP_CONNTRACK_CLOSE:
 		if (index == TCP_RST_SET
-		    && test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status)
-		    && conntrack->proto.tcp.last_index <= TCP_SYNACK_SET
+		    && (conntrack->proto.tcp.last_index <= TCP_SYNACK_SET
+		        || conntrack->proto.tcp.last_index == TCP_ACK_SET)
 		    && after(ntohl(th->ack_seq),
 		    	     conntrack->proto.tcp.last_seq)) {
-			/* Ignore RST closing down invalid SYN
+			/* Ignore RST closing down invalid SYN or ACK
 			   we had let trough. */
 		    	WRITE_UNLOCK(&tcp_lock);
 			if (LOG_INVALID(IPPROTO_TCP))


Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: TCP window tracking has bad side effects

[Ludwig Nussel]

Jozsef Kadlecsik wrote:
> On Fri, 3 Dec 2004, Jozsef Kadlecsik wrote:
> 
> > On Wed, 1 Dec 2004, Ludwig Nussel wrote:
> >
> > > My description probably wasn't unambiguous. The client has the
> > > packetfilter, crashes and reboots.
> >
> > I see - that case was not handled in the code. Please try the patch below
> > and report the result.
> 
> Oh, I'm sorry that was an incomplete patch. This is the fixed code, please
> give it a try.

Seems to work. The client machine no longer drops the packet and
properly sends RST.

cu
Ludwig

-- 
 (o_   Ludwig Nussel
 //\   SUSE LINUX Products GmbH, Development
 V_/_  http://www.suse.de/

Re: TCP window tracking has bad side effects

[Jozsef Kadlecsik]

Hi Ludwing,

On Thu, 9 Dec 2004, Ludwig Nussel wrote:

> Seems to work. The client machine no longer drops the packet and
> properly sends RST.

Thank you for the bug and test reports. Attached is the patch for kernel
inclusion by Dave: Patrick rewived the previous one and noted that a
condition was errorneously dropped - it's back now and a typo in logging
is fixed as well. If you run the previous code on a production machine,
please replace it with the new one.

Best regards,
Jozsef

-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

diff -urN --exclude-from=/usr/src/diff.exclude linux-2.6.9-orig/include/linux/netfilter_ipv4/ip_conntrack_tcp.h linux-2.6.9-tcp-win/include/linux/netfilter_ipv4/ip_conntrack_tcp.h
--- linux-2.6.9-orig/include/linux/netfilter_ipv4/ip_conntrack_tcp.h	2004-10-18 23:53:51.000000000 +0200
+++ linux-2.6.9-tcp-win/include/linux/netfilter_ipv4/ip_conntrack_tcp.h	2004-12-06 06:52:07.000000000 +0100
@@ -18,7 +18,7 @@
 };

 /* Window scaling is advertised by the sender */
-#define IP_CT_TCP_STATE_FLAG_WINDOW_SCALE	0x01
+#define IP_CT_TCP_FLAG_WINDOW_SCALE		0x01

 /* SACK is permitted by the sender */
 #define IP_CT_TCP_FLAG_SACK_PERM		0x02
diff -urN --exclude-from=/usr/src/diff.exclude linux-2.6.9-orig/net/ipv4/netfilter/ip_conntrack_proto_tcp.c linux-2.6.9-tcp-win/net/ipv4/netfilter/ip_conntrack_proto_tcp.c
--- linux-2.6.9-orig/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-10-18 23:55:29.000000000 +0200
+++ linux-2.6.9-tcp-win/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-12-10 08:52:55.000000000 +0100
@@ -273,9 +273,9 @@
  *	sCL -> sCL
  */
 /* 	     sNO, sSS, sSR, sES, sFW, sCW, sLA, sTW, sCL, sLI	*/
-/*ack*/	   { sIV, sIV, sIV, sES, sCW, sCW, sTW, sTW, sCL, sIV },
+/*ack*/	   { sIV, sIG, sIV, sES, sCW, sCW, sTW, sTW, sCL, sIV },
 /*
- *	sSS -> sIV	ACK is invalid: we haven't seen a SYN/ACK yet.
+ *	sSS -> sIG	Might be a half-open connection.
  *	sSR -> sIV	Simultaneous open.
  *	sES -> sES	:-)
  *	sFW -> sCW	Normal close request answered by ACK.
@@ -436,7 +436,7 @@
 					state->td_scale = 14;
 				}
 				state->flags |=
-					IP_CT_TCP_STATE_FLAG_WINDOW_SCALE;
+					IP_CT_TCP_FLAG_WINDOW_SCALE;
 			}
 			ptr += opsize - 2;
 			length -= opsize;
@@ -552,8 +552,8 @@
 			 * Both sides must send the Window Scale option
 			 * to enable window scaling in either direction.
 			 */
-			if (!(sender->flags & IP_CT_TCP_STATE_FLAG_WINDOW_SCALE
-			      && receiver->flags & IP_CT_TCP_STATE_FLAG_WINDOW_SCALE))
+			if (!(sender->flags & IP_CT_TCP_FLAG_WINDOW_SCALE
+			      && receiver->flags & IP_CT_TCP_FLAG_WINDOW_SCALE))
 				sender->td_scale =
 				receiver->td_scale = 0;
 		} else {
@@ -566,9 +566,11 @@
 			sender->td_maxwin = (win == 0 ? 1 : win);
 			sender->td_maxend = end + sender->td_maxwin;
 		}
-	} else if (state->state == TCP_CONNTRACK_SYN_SENT
-		   && dir == IP_CT_DIR_ORIGINAL
-		   && after(end, sender->td_end)) {
+	} else if (((state->state == TCP_CONNTRACK_SYN_SENT
+		     && dir == IP_CT_DIR_ORIGINAL)
+		    || (state->state == TCP_CONNTRACK_SYN_RECV
+		        && dir == IP_CT_DIR_REPLY))
+		    && after(end, sender->td_end)) {
 		/*
 		 * RFC 793: "if a TCP is reinitialized ... then it need
 		 * not wait at all; it must only be sure to use sequence
@@ -685,7 +687,7 @@
 			"ip_ct_tcp: %s ",
 			before(end, sender->td_maxend + 1) ?
 			after(seq, sender->td_end - receiver->td_maxwin - 1) ?
-			before(ack, receiver->td_end + 1) ?
+			before(sack, receiver->td_end + 1) ?
 			after(ack, receiver->td_end - MAXACKWINDOW(sender)) ? "BUG"
 			: "ACK is under the lower bound (possibly overly delayed ACK)"
 			: "ACK is over the upper bound (ACKed data has never seen yet)"
@@ -847,7 +849,9 @@

 	switch (new_state) {
 	case TCP_CONNTRACK_IGNORE:
-		/* Either SYN in ORIGINAL, or SYN/ACK in REPLY direction. */
+		/* Either SYN in ORIGINAL
+		 * or SYN/ACK in REPLY
+		 * or ACK in REPLY direction (half-open connection). */
 		if (index == TCP_SYNACK_SET
 		    && conntrack->proto.tcp.last_index == TCP_SYN_SET
 		    && conntrack->proto.tcp.last_dir != dir
@@ -876,7 +880,7 @@
 		WRITE_UNLOCK(&tcp_lock);
 		if (LOG_INVALID(IPPROTO_TCP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL,
-				  "ip_ct_tcp: invalid SYN (ignored) ");
+				  "ip_ct_tcp: invalid packet ignored ");
 		return NF_ACCEPT;
 	case TCP_CONNTRACK_MAX:
 		/* Invalid packet */
@@ -901,11 +905,12 @@
 		break;
 	case TCP_CONNTRACK_CLOSE:
 		if (index == TCP_RST_SET
-		    && test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status)
-		    && conntrack->proto.tcp.last_index <= TCP_SYNACK_SET
+		    && ((test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status)
+		         && conntrack->proto.tcp.last_index <= TCP_SYNACK_SET)
+		        || conntrack->proto.tcp.last_index == TCP_ACK_SET)
 		    && after(ntohl(th->ack_seq),
 		    	     conntrack->proto.tcp.last_seq)) {
-			/* Ignore RST closing down invalid SYN
+			/* Ignore RST closing down invalid SYN or ACK
 			   we had let trough. */
 		    	WRITE_UNLOCK(&tcp_lock);
 			if (LOG_INVALID(IPPROTO_TCP))

Re: TCP window tracking has bad side effects

[Bill Rugolsky Jr.]

On Thu, Dec 09, 2004 at 05:26:56PM +0100, Ludwig Nussel wrote:
> Seems to work. The client machine no longer drops the packet and
> properly sends RST.
 
Jozsef,

Will you push this to DaveM for 2.6.10?

Thanks.

	Bill Rugolsky

Re: TCP window tracking has bad side effects

[Jozsef Kadlecsik]

On Fri, 10 Dec 2004, Bill Rugolsky Jr. wrote:

> On Thu, Dec 09, 2004 at 05:26:56PM +0100, Ludwig Nussel wrote:
> > Seems to work. The client machine no longer drops the packet and
> > properly sends RST.
>
> Will you push this to DaveM for 2.6.10?

Yes, he fully aware of the issue and the slightly modified version I have
posted to the list today (not arrived yet, it seems) will hopefully be
added to 2.6.10.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: TCP window tracking has bad side effects

[David S. Miller]

On Fri, 10 Dec 2004 12:22:17 -0500
"Bill Rugolsky Jr." <brugolsky@telemetry-investments.com> wrote:

> On Thu, Dec 09, 2004 at 05:26:56PM +0100, Ludwig Nussel wrote:
> > Seems to work. The client machine no longer drops the packet and
> > properly sends RST.
>  
> Will you push this to DaveM for 2.6.10?

Patrick and I are working on getting this integrated.  We just
came up with a final version of the patch, so I'll push it at
my next opportunity.

Re: TCP window tracking has bad side effects

[David S. Miller]

On Fri, 10 Dec 2004 11:03:29 +0100 (CET)
Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote:

> Hi Ludwing,
> 
> On Thu, 9 Dec 2004, Ludwig Nussel wrote:
> 
> > Seems to work. The client machine no longer drops the packet and
> > properly sends RST.
> 
> Thank you for the bug and test reports. Attached is the patch for kernel
> inclusion by Dave: Patrick rewived the previous one and noted that a
> condition was errorneously dropped - it's back now and a typo in logging
> is fixed as well. If you run the previous code on a production machine,
> please replace it with the new one.

Jozsef, btw, can I ask you to stop using whatever patch generating
tool you use that removes trailing spaces?  It makes it so that I have
to apply your patches by hand.

For example:

> @@ -552,8 +552,8 @@
>  			 * Both sides must send the Window Scale option
>  			 * to enable window scaling in either direction.
>  			 */
> -			if (!(sender->flags & IP_CT_TCP_STATE_FLAG_WINDOW_SCALE
> -			      && receiver->flags & IP_CT_TCP_STATE_FLAG_WINDOW_SCALE))
> +			if (!(sender->flags & IP_CT_TCP_FLAG_WINDOW_SCALE
> +			      && receiver->flags & IP_CT_TCP_FLAG_WINDOW_SCALE))
>  				sender->td_scale =
>  				receiver->td_scale = 0;
>  		} else {

In the sources, there is a space at the end of the line
with the "sender->td_scale =" assignment statement.  Yet
in your patch, it is not there.

Same thing here:

> @@ -876,7 +880,7 @@
>  		WRITE_UNLOCK(&tcp_lock);
>  		if (LOG_INVALID(IPPROTO_TCP))
>  			nf_log_packet(PF_INET, 0, skb, NULL, NULL,
> -				  "ip_ct_tcp: invalid SYN (ignored) ");
> +				  "ip_ct_tcp: invalid packet ignored ");
>  		return NF_ACCEPT;
>  	case TCP_CONNTRACK_MAX:
>  		/* Invalid packet */

The "nf_log_packet(..." line should have a trailing space,
yet you are patching against a line that does not have it.

And finally it occurs a third time in this hunk.

> @@ -901,11 +905,12 @@
>  		break;
>  	case TCP_CONNTRACK_CLOSE:
>  		if (index == TCP_RST_SET
> -		    && test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status)
> -		    && conntrack->proto.tcp.last_index <= TCP_SYNACK_SET
> +		    && ((test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status)
> +		         && conntrack->proto.tcp.last_index <= TCP_SYNACK_SET)
> +		        || conntrack->proto.tcp.last_index == TCP_ACK_SET)
>  		    && after(ntohl(th->ack_seq),
>  		    	     conntrack->proto.tcp.last_seq)) {
> -			/* Ignore RST closing down invalid SYN
> +			/* Ignore RST closing down invalid SYN or ACK
>  			   we had let trough. */
>  		    	WRITE_UNLOCK(&tcp_lock);
>  			if (LOG_INVALID(IPPROTO_TCP))

The line with the "/* Ignore ..." comment should have a trailing
space, yet it does not in what your patch is against.

I've read recently that one of the commonly used patch
generating tools removes the training spaces.  But whatever
it is, please stop doing this :-)

Re: TCP window tracking has bad side effects

[Jozsef Kadlecsik]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 611 bytes --]

On Fri, 10 Dec 2004, David S. Miller wrote:

> Jozsef, btw, can I ask you to stop using whatever patch generating
> tool you use that removes trailing spaces?  It makes it so that I have
> to apply your patches by hand.

It's my MUA (pine, ughhh) which messes with the trailing spaces when
the patch is included inline. Attached the patch instead and sorry.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

[-- Attachment #2: tcp-win-half-open.patch --]
[-- Type: TEXT/PLAIN, Size: 4585 bytes --]

diff -urN --exclude-from=/usr/src/diff.exclude linux-2.6.9-orig/include/linux/netfilter_ipv4/ip_conntrack_tcp.h linux-2.6.9-tcp-win/include/linux/netfilter_ipv4/ip_conntrack_tcp.h
--- linux-2.6.9-orig/include/linux/netfilter_ipv4/ip_conntrack_tcp.h	2004-10-18 23:53:51.000000000 +0200
+++ linux-2.6.9-tcp-win/include/linux/netfilter_ipv4/ip_conntrack_tcp.h	2004-12-06 06:52:07.000000000 +0100
@@ -18,7 +18,7 @@
 };
 
 /* Window scaling is advertised by the sender */
-#define IP_CT_TCP_STATE_FLAG_WINDOW_SCALE	0x01
+#define IP_CT_TCP_FLAG_WINDOW_SCALE		0x01
 
 /* SACK is permitted by the sender */
 #define IP_CT_TCP_FLAG_SACK_PERM		0x02
diff -urN --exclude-from=/usr/src/diff.exclude linux-2.6.9-orig/net/ipv4/netfilter/ip_conntrack_proto_tcp.c linux-2.6.9-tcp-win/net/ipv4/netfilter/ip_conntrack_proto_tcp.c
--- linux-2.6.9-orig/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-10-18 23:55:29.000000000 +0200
+++ linux-2.6.9-tcp-win/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-12-10 08:52:55.000000000 +0100
@@ -273,9 +273,9 @@
  *	sCL -> sCL
  */
 /* 	     sNO, sSS, sSR, sES, sFW, sCW, sLA, sTW, sCL, sLI	*/
-/*ack*/	   { sIV, sIV, sIV, sES, sCW, sCW, sTW, sTW, sCL, sIV },
+/*ack*/	   { sIV, sIG, sIV, sES, sCW, sCW, sTW, sTW, sCL, sIV },
 /*
- *	sSS -> sIV	ACK is invalid: we haven't seen a SYN/ACK yet.
+ *	sSS -> sIG	Might be a half-open connection.
  *	sSR -> sIV	Simultaneous open.
  *	sES -> sES	:-)
  *	sFW -> sCW	Normal close request answered by ACK.
@@ -436,7 +436,7 @@
 					state->td_scale = 14;
 				}
 				state->flags |=
-					IP_CT_TCP_STATE_FLAG_WINDOW_SCALE;
+					IP_CT_TCP_FLAG_WINDOW_SCALE;
 			}
 			ptr += opsize - 2;
 			length -= opsize;
@@ -552,8 +552,8 @@
 			 * Both sides must send the Window Scale option
 			 * to enable window scaling in either direction.
 			 */
-			if (!(sender->flags & IP_CT_TCP_STATE_FLAG_WINDOW_SCALE
-			      && receiver->flags & IP_CT_TCP_STATE_FLAG_WINDOW_SCALE))
+			if (!(sender->flags & IP_CT_TCP_FLAG_WINDOW_SCALE
+			      && receiver->flags & IP_CT_TCP_FLAG_WINDOW_SCALE))
 				sender->td_scale = 
 				receiver->td_scale = 0;
 		} else {
@@ -566,9 +566,11 @@
 			sender->td_maxwin = (win == 0 ? 1 : win);
 			sender->td_maxend = end + sender->td_maxwin;
 		}
-	} else if (state->state == TCP_CONNTRACK_SYN_SENT
-		   && dir == IP_CT_DIR_ORIGINAL
-		   && after(end, sender->td_end)) {
+	} else if (((state->state == TCP_CONNTRACK_SYN_SENT
+		     && dir == IP_CT_DIR_ORIGINAL)
+		    || (state->state == TCP_CONNTRACK_SYN_RECV
+		        && dir == IP_CT_DIR_REPLY))
+		    && after(end, sender->td_end)) {
 		/*
 		 * RFC 793: "if a TCP is reinitialized ... then it need
 		 * not wait at all; it must only be sure to use sequence 
@@ -685,7 +687,7 @@
 			"ip_ct_tcp: %s ",
 			before(end, sender->td_maxend + 1) ?
 			after(seq, sender->td_end - receiver->td_maxwin - 1) ?
-			before(ack, receiver->td_end + 1) ?
+			before(sack, receiver->td_end + 1) ?
 			after(ack, receiver->td_end - MAXACKWINDOW(sender)) ? "BUG"
 			: "ACK is under the lower bound (possibly overly delayed ACK)"
 			: "ACK is over the upper bound (ACKed data has never seen yet)"
@@ -847,7 +849,9 @@
 
 	switch (new_state) {
 	case TCP_CONNTRACK_IGNORE:
-		/* Either SYN in ORIGINAL, or SYN/ACK in REPLY direction. */
+		/* Either SYN in ORIGINAL
+		 * or SYN/ACK in REPLY
+		 * or ACK in REPLY direction (half-open connection). */
 		if (index == TCP_SYNACK_SET
 		    && conntrack->proto.tcp.last_index == TCP_SYN_SET
 		    && conntrack->proto.tcp.last_dir != dir
@@ -876,7 +880,7 @@
 		WRITE_UNLOCK(&tcp_lock);
 		if (LOG_INVALID(IPPROTO_TCP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
-				  "ip_ct_tcp: invalid SYN (ignored) ");
+				  "ip_ct_tcp: invalid packet ignored ");
 		return NF_ACCEPT;
 	case TCP_CONNTRACK_MAX:
 		/* Invalid packet */
@@ -901,11 +905,12 @@
 		break;
 	case TCP_CONNTRACK_CLOSE:
 		if (index == TCP_RST_SET
-		    && test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status)
-		    && conntrack->proto.tcp.last_index <= TCP_SYNACK_SET
+		    && ((test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status)
+		         && conntrack->proto.tcp.last_index <= TCP_SYNACK_SET)
+		        || conntrack->proto.tcp.last_index == TCP_ACK_SET)
 		    && after(ntohl(th->ack_seq),
 		    	     conntrack->proto.tcp.last_seq)) {
-			/* Ignore RST closing down invalid SYN 
+			/* Ignore RST closing down invalid SYN or ACK
 			   we had let trough. */ 
 		    	WRITE_UNLOCK(&tcp_lock);
 			if (LOG_INVALID(IPPROTO_TCP))

Re: TCP window tracking has bad side effects

[Jan Du Caju]

Hi,

On Fri, Dec 10, 2004 at 11:51:49AM -0800, David S. Miller wrote:
> On Fri, 10 Dec 2004 12:22:17 -0500
> "Bill Rugolsky Jr." <brugolsky@telemetry-investments.com> wrote:
> 
> > On Thu, Dec 09, 2004 at 05:26:56PM +0100, Ludwig Nussel wrote:
> > > Seems to work. The client machine no longer drops the packet and
> > > properly sends RST.
> >  
> > Will you push this to DaveM for 2.6.10?
> 
> Patrick and I are working on getting this integrated.  We just
> came up with a final version of the patch, so I'll push it at
> my next opportunity.

Is it possible to also consider a version for 2.4? Maybe to be included 
in 2.4.29 ;-)

-- 
adTHANKSvance
Jan.
---------------------------------------------------- KULeuvenNet ------
Jan.DuCaju@kuleuven.net         http://www.kuleuven.net/e_index.html            
K.U.Leuven                      http://www.kuleuven.ac.be/english/              
LUDIT - KULeuvenNet             http://ludit.kuleuven.be/index_en.html          
de Croylaan 52A                 3001 Leuven                     Belgium         
-----------------------------------------------------------------------

Re: TCP window tracking has bad side effects

[Phil Oester]

On Mon, Jan 10, 2005 at 06:13:49PM +0100, Jan Du Caju wrote:
> Is it possible to also consider a version for 2.4? Maybe to be included 
> in 2.4.29 ;-)

It doesn't affect 2.4.x

Phil