improvement for TCP connection tracking

 net/netfilter/nf_conntrack_proto_tcp.c |    4 ++--

 1 files changed, 2 insertions(+), 2 deletions(-)

net/netfilter/nf_conntrack_proto_tcp.c +0 -1

index 3fb2b73..be0b84d 100644
@@ -142,12 +142,12 @@ static const u8 tcp_conntracks[2][6][TCP_CONNTRACK_MAX] = {
        {
  /* ORIGINAL */
  /*          sNO, sSS, sSR, sES, sFW, sCW, sLA, sTW, sCL, sS2   */
-/*syn*/           { sSS, sSS, sIG, sIG, sIG, sIG, sIG, sSS, sSS, sS2 },

Consider the following scenario:

client firewall server

     |              |             |
     |     syn      |     syn     |
     |------------->|------------>|
     |              |             |
     |   syn+ack    |   syn+ack   |
     |      x<------|<------------|      syn+ack got lost!
     |              |             |
     |     syn      |     syn     |
     |------------->|------------>|
     |              |             |
     |   syn+ack    |   syn+ack   |
     |<-------------|<------------|
     |              |             |

Note that the syn+ack is lost after we have seen it. Without this patch, the TCP tracking ignores the retransmitted SYN without checking if the sequence number is in the window.

This patch also helps a lot to conntrackd in stress scenarios (assumming a client that generates lots of small TCP connections). During the failover, consider that the new primary has injected one outdated flow in SYN_RECV state (this is likely to happen if the conntrack event rate is high because the backup will be a bit delayed from the primary). With the current code, if the client starts a new fresh connection that matches the tuple, the SYN packet will be ignored without updating the state tracking, and the SYN+ACK in reply will blocked as it will not pass checkings III or IV (since all state tracking in the original direction is not initialized because of the SYN packet was ignored).

Cc Jozsef Kadlecsik <[email protected]>

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 net/netfilter/nf_conntrack_proto_tcp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

net/netfilter/nf_conntrack_proto_tcp.c +0 -1

index 3fb2b73..be0b84d 100644
@@ -142,12 +142,12 @@ static const u8 tcp_conntracks[2][6][TCP_CONNTRACK_MAX] = {
 	{
 /* ORIGINAL */
 /* 	     sNO, sSS, sSR, sES, sFW, sCW, sLA, sTW, sCL, sS2	*/
-/*syn*/	   { sSS, sSS, sIG, sIG, sIG, sIG, sIG, sSS, sSS, sS2 },

Maybe I'm just blind, but I don't see what you try to achieve. Actually, with your patch you switch off the path which would handle the case when conntrack holds an outdated entry from whatever reason.

Present code, cases:

a. conntrack in sync, SYN/ACK lost and client resends the SYN
- we ignore it but it does not do any harm
b. conntrack entry is outdated, new SYN received
- we ignore it but save the initialization data from it
- when the reply SYN/ACK receives and it matches the saved data, we pick up the new connection

With your patch, cases:

a. conntrack in sync, SYN/ACK lost and client resends the SYN
- SYN is out of window and will be marked as invalid
b. conntrack entry is outdated, new SYN received
- SYN is out of window and will be marked as invalid

What do I miss?

Quoted 11 lines

This patch also helps a lot to conntrackd in stress scenarios (assumming a client that generates lots of small TCP connections). During the failover, consider that the new primary has injected one outdated flow in SYN_RECV state (this is likely to happen if the conntrack event rate is high because the backup will be a bit delayed from the primary). With the current code, if the client starts a new fresh connection that matches the tuple, the SYN packet will be ignored without updating the state tracking, and the SYN+ACK in reply will blocked as it will not pass checkings III or IV (since all state tracking in the original direction is not initialized because of the SYN packet was ignored).

The last_dir entries are updated from the ignored SYN packet and thus the SYN+ACK won't be blocked - if the packet is a true answer to the SYN.

b. conntrack entry is outdated, new SYN received
- (b1) we ignore it but save the initialization data from it
- (b2) when the reply SYN/ACK receives and it matches the saved data, we pick up the new connection

Quoted 4 lines · Jozsef Kadlecsik

Maybe I'm just blind, but I don't see what you try to achieve. Actually, with your patch you switch off the path which would handle the case when conntrack holds an outdated entry from whatever reason.

Quoted 8 lines

Present code, cases:

a. conntrack in sync, SYN/ACK lost and client resends the SYN
- we ignore it but it does not do any harm
b. conntrack entry is outdated, new SYN received
- we ignore it but save the initialization data from it
- when the reply SYN/ACK receives and it matches the saved data, we pick up the new connection

This is what it seems at first sight, but this is not what it happens if the conntrack is in SYN_RECV state.

Initially, the SYN packet hits first item of 'b', thus we save data from it. But the SYN/ACK packet is considered a retransmission given that we're in SYN_RECV state. Therefore, we never hit the second item of 'b'.

Quoted 6 lines

With your patch, cases:

a. conntrack in sync, SYN/ACK lost and client resends the SYN
- SYN is out of window and will be marked as invalid
b. conntrack entry is outdated, new SYN received
- SYN is out of window and will be marked as invalid

Damn, you're right, this patch is broken, that SYN packet won't pass boundary III, IIRC.

Quoted 1 line

What do I miss?

That we're in SYN_RECV state :-).

Quoted 12 lines

Quoted 11 lines

This patch also helps a lot to conntrackd in stress scenarios (assumming a client that generates lots of small TCP connections). During the failover, consider that the new primary has injected one outdated flow in SYN_RECV state (this is likely to happen if the conntrack event rate is high because the backup will be a bit delayed from the primary). With the current code, if the client starts a new fresh connection that matches the tuple, the SYN packet will be ignored without updating the state tracking, and the SYN+ACK in reply will blocked as it will not pass checkings III or IV (since all state tracking in the original direction is not initialized because of the SYN packet was ignored).

Quoted 2 lines

The last_dir entries are updated from the ignored SYN packet and thus the SYN+ACK won't be blocked - if the packet is a true answer to the SYN.

This is what it happens with other states, but not for SYN_RECV.

I have reworked the patch and the description. With this, the SYN_RECV state if we're out of sync receives the same handling.

I have test it here, it works fine. Let me know if you're OK with it.

Quoted 1 line · Pablo Neira Ayuso

I have test it here, it works fine. Let me know if you're OK with it.

The patch looks OK but I think Changli Gao is also right and it'd be simpler to set the [reply][synack][SR] state to sIG. What do you think?

Quoted 5 lines · Jozsef Kadlecsik

Quoted 1 line · Pablo Neira Ayuso

I have test it here, it works fine. Let me know if you're OK with it.

The patch looks OK but I think Changli Gao is also right and it'd be simpler to set the [reply][synack][SR] state to sIG. What do you think?

I read his email before leaving and after I made the new patch.

Indeed, his idea is simpler, here's a new patch. I tested it here, it works fine.

Patrick, please apply!

b. conntrack entry is outdated, new SYN received
- (b1) we ignore it but save the initialization data from it
- (b2) when the reply SYN/ACK receives and it matches the saved data, we pick up the new connection

Quoted 7 lines · Pablo Neira Ayuso

Quoted 5 lines · Jozsef Kadlecsik

Quoted 1 line · Pablo Neira Ayuso

I have test it here, it works fine. Let me know if you're OK with it.

The patch looks OK but I think Changli Gao is also right and it'd be simpler to set the [reply][synack][SR] state to sIG. What do you think?

Quoted 6 lines

I read his email before leaving and after I made the new patch.

Indeed, his idea is simpler, here's a new patch. I tested it here, it works fine.

Patrick, please apply!

Hm, I forgot to include the description. New patch attached.

Quoted 7 lines · Pablo Neira Ayuso

Quoted 5 lines · Jozsef Kadlecsik

Quoted 1 line · Pablo Neira Ayuso

I have test it here, it works fine. Let me know if you're OK with it.

The patch looks OK but I think Changli Gao is also right and it'd be simpler to set the [reply][synack][SR] state to sIG. What do you think?

Quoted 6 lines

I read his email before leaving and after I made the new patch.

Indeed, his idea is simpler, here's a new patch. I tested it here, it works fine.

Patrick, please apply!

Hm, I forgot to include the description. New patch attached.

Quoted 13 lines · Pablo Neira Ayuso

Quoted 12 lines · Pablo Neira Ayuso

Quoted 5 lines · Jozsef Kadlecsik

Quoted 1 line · Pablo Neira Ayuso

I have test it here, it works fine. Let me know if you're OK with it.

The patch looks OK but I think Changli Gao is also right and it'd be simpler to set the [reply][synack][SR] state to sIG. What do you think?

I read his email before leaving and after I made the new patch.

Indeed, his idea is simpler, here's a new patch. I tested it here, it works fine.

Quoted 3 lines

Quoted 1 line

Patrick, please apply!

Hm, I forgot to include the description. New patch attached.

Thanks. I'll apply this tommorrow, assuming Jozsef is fine with this version.

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

Quoted 1 line

Hm, I forgot to include the description. New patch attached.

Thanks. I'll apply this tommorrow, assuming Jozsef is fine with this version.

Yes, of course

Signed-off-by Jozsef Kadlecsik <[email protected]>