Re: [PATCH] for some issues in tcp window tracking patch

[Pablo Neira <pablo@eurodev.net>]

Hi Jozsef,

Jozsef Kadlecsik wrote:

>It's funny: (practically) connection tracking itself does not drop
>packets, it leaves the administrator to do the dirty job by setting up
>explicit rules to drop INVALID packets (or implicitly, by letting trough
>only valid packets) :-)
>  
>

sorry! I missed that... I should know better iptables !:-)

>
>Some tests are done before checking the window boundaries from good
>reasons: checking special cases when packets are possibly (mostly!) out of
>window. You cannot move them behind the boundary checkings.
>  
>

ok, I also missed that... more issues:

a) Sometimes we destroy the conntrack calling 
conntrack->timeout.function((unsigned long)conntrack) but we are 
returning -NF_ACCEPT, so we are calling implicitely nf_conntrack_put 
from conntrack core, could we have any unexpected problem?

b) We don't need to set assured bit all the time, I think that I told 
you, well if so, sorry for repeating me.

         } else if ((old_state == TCP_CONNTRACK_SYN_RECV
                    || old_state == TCP_CONNTRACK_ESTABLISHED)
-                   && new_state == TCP_CONNTRACK_ESTABLISHED)
+                 && new_state == TCP_CONNTRACK_ESTABLISHED
+                 && !test_bit(IPS_ASSURED_BIT, &conntrack->status)) {

c) In the TCP_CONNTRACK_SYN_SENT case, we saw a SYN packet while staying 
in state TIME_WAIT, so there's a reopened connection. Why do we destroy 
the conntrack and don't recycle /reuse this conntrack? I mean, 
reinitilize all the fields and let it continue its travel through the 
tcp tracking of instead of returning NF_REPEAT.

d) We could move up WRITE_UNLOCK(&tcp_lock) just before timeout is updated.

e) I'm not sure if it's worthy optimizing this case. In tcp_new, we are 
checking if a packet is clean as well as size of tcp header, then in 
tcp_packet we will do the same, so we are doing twice the same checking. 
If we consider that general case is that packets are clean, will be this 
double check worthy?

thanks Jozsef!,
insidious Pablo