netfilter: nf_conntrack_tcp: fixing to check the lower bound of valid ACK

Lost connections was reported by Thomas Bätzler (running 2.6.25 kernel) on
the netfilter mailing list (see the thread "Weird nat/conntrack Problem
with PASV FTP upload"). He provided tcpdump recordings which helped to
find a long lingering bug in conntrack.

In TCP connection tracking, checking the lower bound of valid ACK could
lead to mark valid packets as INVALID because:

 - We have got a "higher or equal" inequality, but the test checked
   the "higher" condition only; fixed.
 - If the packet contains a SACK option, it could occur that the ACK
   value was before the left edge of our (S)ACK "window": if a previous
   packet from the other party intersected the right edge of the window
   of the receiver, we could move forward the window parameters beyond
   accepting a valid ack. Therefore in this patch we check the rightmost
   SACK edge instead of the ACK value in the lower bound of valid (S)ACK
   test.

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Signed-off-by: Patrick McHardy <kaber@trash.net>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index ba94004..271cd01 100644 (file)
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -331,12 +331,13 @@ static unsigned int get_conntrack_index(const struct tcphdr *tcph)
 
    I.   Upper bound for valid data:    seq <= sender.td_maxend
    II.  Lower bound for valid data:    seq + len >= sender.td_end - receiver.td_maxwin
-   III.        Upper bound for valid ack:      sack <= receiver.td_end
-   IV. Lower bound for valid ack:      ack >= receiver.td_end - MAXACKWINDOW
+   III.        Upper bound for valid (s)ack:   sack <= receiver.td_end
+   IV. Lower bound for valid (s)ack:   sack >= receiver.td_end - MAXACKWINDOW
 
-   where sack is the highest right edge of sack block found in the packet.
+   where sack is the highest right edge of sack block found in the packet
+   or ack in the case of packet without SACK option.
 
-   The upper bound limit for a valid ack is not ignored -
+   The upper bound limit for a valid (s)ack is not ignored -
    we doesn't have to deal with fragments.
 */
 
@@ -606,12 +607,12 @@ static bool tcp_in_window(const struct nf_conn *ct,
                 before(seq, sender->td_maxend + 1),
                 after(end, sender->td_end - receiver->td_maxwin - 1),
                 before(sack, receiver->td_end + 1),
-                after(ack, receiver->td_end - MAXACKWINDOW(sender)));
+                after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1));
 
        if (before(seq, sender->td_maxend + 1) &&
            after(end, sender->td_end - receiver->td_maxwin - 1) &&
            before(sack, receiver->td_end + 1) &&
-           after(ack, receiver->td_end - MAXACKWINDOW(sender))) {
+           after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1)) {
                /*
                 * Take into account window scaling (RFC 1323).
                 */