RE: [PATCH-2.4-POM] NF_REPEAT was ignored !

RE: [PATCH-2.4-POM] NF_REPEAT was ignored !

[Gaby Schilders]

Hi,

There are quite recent changes to the tcp-window-tracking patches. You can
look them up on the CVS interface to p-o-m at the netfilter site:
http://cvs.netfilter.org/netfilter/patch-o-matic/extra/tcp-window-tracking.patch
I haven't checked if they include a solution to your problem, but maybe you
can. (My knowledge of C is... rather limited ;)

Also, I was wondering if this might not be better suited for the developer
list?

Regards,

Gaby Schilders
IBFD network admin

-----Original Message-----
From: Willy Tarreau [mailto:willy@w.ods.org]
Sent: woensdag 22 oktober 2003 12:26
To: Jozsef Kadlecsik
Cc: Harald Welte; netfilter@lists.netfilter.org; Netfilter Development
Mailinglist
Subject: [PATCH-2.4-POM] NF_REPEAT was ignored !


Hi,

after updating the production firewalls to handle the CW->CL state, I saw the
rate of drops decrease, but not as much as I would have expected it to.

I captured lots of data (/p/n/ip_conntrack, logs, tcpdump) and discovered
another problem with tcp_window_tracking that I could easily reproduce on
a lab : if a client reused a port too early, then the SYN/ACK from the
server was dropped, and the client could only connect after the next SYN
retransmit. I simply checked it with nc -p 1234 server 80. The first one
succeeds immediately, the second one needs 3 seconds to establish. There
is a logical explication to this :

The client completes a first connection to server:80 with spt=1234. A few
seconds later, he reuses the same port to initiate a new connection to the
server. The firewall still sees the connection in TIME_WAIT state, so its
state matrix switches it to SYN_SENT  (orig:sTW--(SY)-->sSS).

In ip_conntrack_proto_tcp.c:tcp_packet(), there is a test for this case. The
existing session is deleted and NF_REPEAT is returned so that the caller tries
again (here, ip_conntrack_core.c:ip_conntrack_in()). This one simply returns
the same code NF_REPEAT to its caller which will call it again (nf_iterate()).

The problem is that once ip_conntrack_in() is called again with the same pskb,
it already has its ->nfct filled, so ip_conntrack_in() immediately returns
NF_ACCEPT without doing any lookup. The result is that the SYN is passed to
the server, and the deleted session is not recreated. When the server replies
with a SYN/ACK, this one has no matching session it is blocked by the firewall
rules. Then, 3 seconds later, the client retransmits its SYN, which reaches
the firewall without any matching session, and correctly initiates a new one.

The solution is to correctly clear the ->nfct field in ip_conntrack_in() if
we return NF_REPEAT. This is what the following patch does. It's to be applied
to 2.4+POM-20030912, but I'm confident it may be easily applied and/or ported
to later versions.

I've not checked yet if the mainline conntrack code is also affected, but this
could be possible.

Regards,
Willy


--- ./net/ipv4/netfilter/ip_conntrack_core.c.orig	Tue Oct 21 14:21:08 2003
+++ ./net/ipv4/netfilter/ip_conntrack_core.c	Tue Oct 21 16:14:53 2003
@@ -856,6 +861,14 @@
 	IP_NF_ASSERT((*pskb)->nfct);
 
 	ret = proto->packet(ct, (*pskb)->nh.iph, (*pskb)->len, ctinfo);
+
+	if (ret == NF_REPEAT) {
+		/* we must loop here again */
+		nf_conntrack_put((*pskb)->nfct);
+		(*pskb)->nfct = NULL;
+		return ret;
+	}
+
 	if (ret == -1) {
 		/* Invalid */
 		nf_conntrack_put((*pskb)->nfct);

Re: [PATCH-2.4-POM] NF_REPEAT was ignored !

[Willy Tarreau]

> There are quite recent changes to the tcp-window-tracking patches.

At least not since the last one I sent, it seems :-)

> Also, I was wondering if this might not be better suited for the developer
> list?

indeed, I replied to a previous mail, hence this cross-posting. The nfdev
list was also cc'd.

Cheers,
Willy

RE: [PATCH-2.4-POM] NF_REPEAT was ignored !

[Gaby Schilders]

What I meant to say is that tcp-window-tracking has been patched two weeks ago,
more recent than p-o-m 2003-09-24 that your patch is against.

I didn't look at the addressing of your post, my bad.

Gaby

-----Original Message-----
From: Willy Tarreau [mailto:willy@w.ods.org]
Sent: woensdag 22 oktober 2003 13:55
To: Gaby Schilders
Cc: netfilter list
Subject: Re: [PATCH-2.4-POM] NF_REPEAT was ignored !


> There are quite recent changes to the tcp-window-tracking patches.

At least not since the last one I sent, it seems :-)

> Also, I was wondering if this might not be better suited for the developer
> list?

indeed, I replied to a previous mail, hence this cross-posting. The nfdev
list was also cc'd.

Cheers,
Willy

Re: [PATCH-2.4-POM] sessions stuck in CLOSE_WAIT state

[Harald Welte]

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

On Wed, Oct 08, 2003 at 12:18:17AM +0200, Willy Tarreau wrote:
> Fortunately, tcpdump showed to me that all stuck sessions had in common the
> fact that the server aborted them with an RST after the first FIN+ACK, so
> in CLOSE_WAIT. Others which were aborted with an RST just before or just after
> were destroyed without any problem.

Ok, this sounds like a pretty clear case than.  This bug has not been
found because replying with RST afer FIN+ACK seems a quite unusual
pattern.
 
> /*rst*/ {sCL, sCL, sCL, sCL, sCL, sCL, sCL, sCL, sLA, sLI },
> 
> But the tcp_window_tracking patch changes this behaviour, I think because of
> a typo, since I see no other reason not to terminate a session upon RST :
> 
> /*          sNO, sSS, sSR, sES, sFW, sCW, sLA, sTW, sCL, sLI   */
> /*rst*/   { sIV, sCL, sCL, sCL, sCL, sIV, sCL, sCL, sCL, sIV },

yes, I totally agree with you.  I'll apply your proposed patch to
patch-o-matic.  If Jozsef disagrees, he can revert that change.

> BTW, is it normal that an RST received in TIME_WAIT state terminates the
> session ? I would have thought that we should keep it in TIME_WAIT anyway.

again agreed.  Jozsef, what do you think?

> BTW, I noticed that the tcp_conntracks[] array takes some memory because every
> entry takes one full int. This is 480 bytes on x86. Wouldn't it be more cache-
> friendly to use compact structures with bit fields instead ?

yes, it was more cache line friendly.  But then, wouldn't we get lots of
unaligned accesses?  oh you made a u32 aligned version...

> For example, either something like this (byte-aligned) :
>   u8 tcp_conntracks[6][10] = { (REPLY_STATE)<<4 | (ORIGINAL_STATE), ... }
>    -> read it from (tcp_conntrack[flag][old_state] >> dir?4:0) & 0xF
> 
> or (u32 aligned) 80 bytes :
>   int tcp_conntracks[2][10] = { /* original */
>                                 { /* old sNO */
>                                   sSS << SYN | sTW << FIN | sES << ACK ...
>   -> read id from (tcp_conntrack[dir][old_state] >> flag*4) & 0xF.

this makes the code really hard to read.  If you can come up with some
easy-to-read macros, I'd be willing to give it a try.

> Same could be done on unclean(). If someone's interested, I can try to look
> at what seems possible, and propose something.

yes, I'm interested - providing the code is not crippled by bit-shifts
and multiplications everywhere.

> Regards,
> Willy

-- 
- 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 #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH-2.4-POM] sessions stuck in CLOSE_WAIT state

[Jozsef Kadlecsik]

On Wed, 8 Oct 2003, Harald Welte wrote:

> On Wed, Oct 08, 2003 at 12:18:17AM +0200, Willy Tarreau wrote:
> > Fortunately, tcpdump showed to me that all stuck sessions had in common the
> > fact that the server aborted them with an RST after the first FIN+ACK, so
> > in CLOSE_WAIT. Others which were aborted with an RST just before or just after
> > were destroyed without any problem.
>
> Ok, this sounds like a pretty clear case than.  This bug has not been
> found because replying with RST afer FIN+ACK seems a quite unusual
> pattern.
>
> > /*rst*/ {sCL, sCL, sCL, sCL, sCL, sCL, sCL, sCL, sLA, sLI },
> >
> > But the tcp_window_tracking patch changes this behaviour, I think because of
> > a typo, since I see no other reason not to terminate a session upon RST :
> >
> > /*          sNO, sSS, sSR, sES, sFW, sCW, sLA, sTW, sCL, sLI   */
> > /*rst*/   { sIV, sCL, sCL, sCL, sCL, sIV, sCL, sCL, sCL, sIV },
>
> yes, I totally agree with you.  I'll apply your proposed patch to
> patch-o-matic.  If Jozsef disagrees, he can revert that change.

I agree completely.

> > BTW, is it normal that an RST received in TIME_WAIT state terminates the
> > session ? I would have thought that we should keep it in TIME_WAIT anyway.
>
> again agreed.  Jozsef, what do you think?

It's normal, but highly unusual I think. (Were it an "optimization" of TCP?)

Thank you very much the nice analysis and report.

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

[PATCH-2.4-POM] NF_REPEAT was ignored !

[Willy Tarreau]

Hi,

after updating the production firewalls to handle the CW->CL state, I saw the
rate of drops decrease, but not as much as I would have expected it to.

I captured lots of data (/p/n/ip_conntrack, logs, tcpdump) and discovered
another problem with tcp_window_tracking that I could easily reproduce on
a lab : if a client reused a port too early, then the SYN/ACK from the
server was dropped, and the client could only connect after the next SYN
retransmit. I simply checked it with nc -p 1234 server 80. The first one
succeeds immediately, the second one needs 3 seconds to establish. There
is a logical explication to this :

The client completes a first connection to server:80 with spt=1234. A few
seconds later, he reuses the same port to initiate a new connection to the
server. The firewall still sees the connection in TIME_WAIT state, so its
state matrix switches it to SYN_SENT  (orig:sTW--(SY)-->sSS).

In ip_conntrack_proto_tcp.c:tcp_packet(), there is a test for this case. The
existing session is deleted and NF_REPEAT is returned so that the caller tries
again (here, ip_conntrack_core.c:ip_conntrack_in()). This one simply returns
the same code NF_REPEAT to its caller which will call it again (nf_iterate()).

The problem is that once ip_conntrack_in() is called again with the same pskb,
it already has its ->nfct filled, so ip_conntrack_in() immediately returns
NF_ACCEPT without doing any lookup. The result is that the SYN is passed to
the server, and the deleted session is not recreated. When the server replies
with a SYN/ACK, this one has no matching session it is blocked by the firewall
rules. Then, 3 seconds later, the client retransmits its SYN, which reaches
the firewall without any matching session, and correctly initiates a new one.

The solution is to correctly clear the ->nfct field in ip_conntrack_in() if
we return NF_REPEAT. This is what the following patch does. It's to be applied
to 2.4+POM-20030912, but I'm confident it may be easily applied and/or ported
to later versions.

I've not checked yet if the mainline conntrack code is also affected, but this
could be possible.

Regards,
Willy


--- ./net/ipv4/netfilter/ip_conntrack_core.c.orig	Tue Oct 21 14:21:08 2003
+++ ./net/ipv4/netfilter/ip_conntrack_core.c	Tue Oct 21 16:14:53 2003
@@ -856,6 +861,14 @@
 	IP_NF_ASSERT((*pskb)->nfct);
 
 	ret = proto->packet(ct, (*pskb)->nh.iph, (*pskb)->len, ctinfo);
+
+	if (ret == NF_REPEAT) {
+		/* we must loop here again */
+		nf_conntrack_put((*pskb)->nfct);
+		(*pskb)->nfct = NULL;
+		return ret;
+	}
+
 	if (ret == -1) {
 		/* Invalid */
 		nf_conntrack_put((*pskb)->nfct);