[PATCH 2.6] optimization of ip_conntrack_proto_tcp:tcp_packet()

[PATCH 2.6] optimization of ip_conntrack_proto_tcp:tcp_packet()

[Harald Welte]

[-- Attachment #1.1: Type: text/plain, Size: 642 bytes --]

Hi Dave!

This is the first in a set of 2.6.x optimization patches.  Obviosly,
they are meant for 2.6.6, not 2.6.5.  Please apply to your tree, thanks.

A: Pablo Neira
D: This patch cleans up tcp_packet().  No semantical change, just juggling
D: code pieces.  

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #1.2: linux-2.6.patch --]
[-- Type: text/plain, Size: 2419 bytes --]

--- linux-2.6.3-old/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-02-18 04:59:50.000000000 +0100
+++ linux-2.6.3/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	2004-03-02 03:11:10.000000000 +0100
@@ -178,6 +178,16 @@
 	if (skb_copy_bits(skb, skb->nh.iph->ihl * 4, &tcph, sizeof(tcph)) != 0)
 		return -1;
 
+	/* If only reply is a RST, we can consider ourselves not to
+	   have an established connection: this is a fairly common
+	   problem case, so we can delete the conntrack
+	   immediately.  --RR */
+	if (!test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status) && tcph.rst) {
+		if (del_timer(&conntrack->timeout))
+			conntrack->timeout.function((unsigned long)conntrack);
+		return NF_ACCEPT;
+	}
+
 	WRITE_LOCK(&tcp_lock);
 	oldtcpstate = conntrack->proto.tcp.state;
 	newconntrack
@@ -199,29 +209,21 @@
 	/* Poor man's window tracking: record SYN/ACK for handshake check */
 	if (oldtcpstate == TCP_CONNTRACK_SYN_SENT
 	    && CTINFO2DIR(ctinfo) == IP_CT_DIR_REPLY
-	    && tcph.syn && tcph.ack)
+	    && tcph.syn && tcph.ack) {
 		conntrack->proto.tcp.handshake_ack
 			= htonl(ntohl(tcph.seq) + 1);
+		goto out;
+	}
 
-	/* If only reply is a RST, we can consider ourselves not to
-	   have an established connection: this is a fairly common
-	   problem case, so we can delete the conntrack
-	   immediately.  --RR */
-	if (!test_bit(IPS_SEEN_REPLY_BIT, &conntrack->status) && tcph.rst) {
-		WRITE_UNLOCK(&tcp_lock);
-		if (del_timer(&conntrack->timeout))
-			conntrack->timeout.function((unsigned long)conntrack);
-	} else {
-		/* Set ASSURED if we see see valid ack in ESTABLISHED after SYN_RECV */
-		if (oldtcpstate == TCP_CONNTRACK_SYN_RECV
-		    && CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL
-		    && tcph.ack && !tcph.syn
-		    && tcph.ack_seq == conntrack->proto.tcp.handshake_ack)
-			set_bit(IPS_ASSURED_BIT, &conntrack->status);
+	/* Set ASSURED if we see valid ack in ESTABLISHED after SYN_RECV */
+	if (oldtcpstate == TCP_CONNTRACK_SYN_RECV
+	    && CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL
+	    && tcph.ack && !tcph.syn
+	    && tcph.ack_seq == conntrack->proto.tcp.handshake_ack)
+		set_bit(IPS_ASSURED_BIT, &conntrack->status);
 
-		WRITE_UNLOCK(&tcp_lock);
-		ip_ct_refresh(conntrack, *tcp_timeouts[newconntrack]);
-	}
+out:	WRITE_UNLOCK(&tcp_lock);
+	ip_ct_refresh(conntrack, *tcp_timeouts[newconntrack]);
 
 	return NF_ACCEPT;
 }

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

Re: [PATCH 2.6] optimization of ip_conntrack_proto_tcp:tcp_packet()

[David S. Miller]

On Mon, 29 Mar 2004 12:33:48 +0200
Harald Welte <laforge@netfilter.org> wrote:

> This is the first in a set of 2.6.x optimization patches.  Obviosly,
> they are meant for 2.6.6, not 2.6.5.  Please apply to your tree, thanks.
> 
> A: Pablo Neira
> D: This patch cleans up tcp_packet().  No semantical change, just juggling
> D: code pieces.  

I'm going to applies these to my 2.6.6 netfilter pending tree.

BUT!  Please think about these changes very carefully.  The order
of testing these state pieces in the TCP header is important.

For example, how it is valid, in this change, to blindly accept a
RESET packet to kill a conntrack entry before verifying the sequence
number(s) (as appropriate per rfc793 rules)?

This looks really suspicious to me.

Re: [PATCH 2.6] optimization of ip_conntrack_proto_tcp:tcp_packet()

[Pablo Neira]

Hi davem,

David S. Miller wrote:

>For example, how it is valid, in this change, to blindly accept a
>RESET packet to kill a conntrack entry before verifying the sequence
>number(s) (as appropriate per rfc793 rules)?
>  
>
as current tcp tracking doesn't perform so strong tcp transitions 
checking, there won't be any problems. Joszef Kadlecsik is working on a 
full featured tcp tracking system which will take care of this stuff, 
but it still needs a bit of time.

>This looks really suspicious to me.
>  
>
Actually, I think that this change won't modify the current behaviour of 
the tcp connection tracking system because the conntrack will be drop 
sooner or later if a rst is received.

regards,
Pablo

Re: [PATCH 2.6] optimization of ip_conntrack_proto_tcp:tcp_packet()

[David S. Miller]

On Tue, 30 Mar 2004 12:42:39 +0200
Pablo Neira <pablo@eurodev.net> wrote:

> as current tcp tracking doesn't perform so strong tcp transitions 
> checking, there won't be any problems. Joszef Kadlecsik is working on a 
> full featured tcp tracking system which will take care of this stuff, 
> but it still needs a bit of time.
 ...
> Actually, I think that this change won't modify the current behaviour of 
> the tcp connection tracking system because the conntrack will be drop 
> sooner or later if a rst is received.

You're right, in fact I keep getting reminded every few weeks about
the lack of sequence number checking in conntrack so I should have
known this :-)

Thanks for the clarification.