tcp: Fix inconsistency source (CA_Open only when !tcp_left_out(tp))

It is possible that this skip path causes TCP to end up into an
invalid state where ca_state was left to CA_Open while some
segments already came into sacked_out. If next valid ACK doesn't
contain new SACK information TCP fails to enter into
tcp_fastretrans_alert(). Thus at least high_seq is set
incorrectly to a too high seqno because some new data segments
could be sent in between (and also, limited transmit is not
being correctly invoked there). Reordering in both directions
can easily cause this situation to occur.

I guess we would want to use tcp_moderate_cwnd(tp) there as well
as it may be possible to use this to trigger oversized burst to
network by sending an old ACK with huge amount of SACK info, but
I'm a bit unsure about its effects (mainly to FlightSize), so to
be on the safe side I just currently fixed it minimally to keep
TCP's state consistent (obviously, such nasty ACKs have been
possible this far). Though it seems that FlightSize is already
underestimated by some amount, so probably on the long term we
might want to trigger recovery there too, if appropriate, to make
FlightSize calculation to resemble reality at the time when the
losses where discovered (but such change scares me too much now
and requires some more thinking anyway how to do that as it
likely involves some code shuffling).

This bug was found by Brian Vowell while running my TCP debug
patch to find cause of another TCP issue (fackets_out
miscount).

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index b54d9d37b636a79b26de0f0ee36e9f6024ba453e..54a0b741278274fc7ab68bd22b7e758aecee80a5 100644 (file)
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2483,6 +2483,20 @@ static inline void tcp_complete_cwr(struct sock *sk)
        tcp_ca_event(sk, CA_EVENT_COMPLETE_CWR);
 }
 
+static void tcp_try_keep_open(struct sock *sk)
+{
+       struct tcp_sock *tp = tcp_sk(sk);
+       int state = TCP_CA_Open;
+
+       if (tcp_left_out(tp) || tp->retrans_out || tp->undo_marker)
+               state = TCP_CA_Disorder;
+
+       if (inet_csk(sk)->icsk_ca_state != state) {
+               tcp_set_ca_state(sk, state);
+               tp->high_seq = tp->snd_nxt;
+       }
+}
+
 static void tcp_try_to_open(struct sock *sk, int flag)
 {
        struct tcp_sock *tp = tcp_sk(sk);
@@ -2496,15 +2510,7 @@ static void tcp_try_to_open(struct sock *sk, int flag)
                tcp_enter_cwr(sk, 1);
 
        if (inet_csk(sk)->icsk_ca_state != TCP_CA_CWR) {
-               int state = TCP_CA_Open;
-
-               if (tcp_left_out(tp) || tp->retrans_out || tp->undo_marker)
-                       state = TCP_CA_Disorder;
-
-               if (inet_csk(sk)->icsk_ca_state != state) {
-                       tcp_set_ca_state(sk, state);
-                       tp->high_seq = tp->snd_nxt;
-               }
+               tcp_try_keep_open(sk);
                tcp_moderate_cwnd(tp);
        } else {
                tcp_cwnd_down(sk, flag);
@@ -3310,8 +3316,11 @@ no_queue:
        return 1;
 
 old_ack:
-       if (TCP_SKB_CB(skb)->sacked)
+       if (TCP_SKB_CB(skb)->sacked) {
                tcp_sacktag_write_queue(sk, skb, prior_snd_una);
+               if (icsk->icsk_ca_state == TCP_CA_Open)
+                       tcp_try_keep_open(sk);
+       }
 
 uninteresting_ack:
        SOCK_DEBUG(sk, "Ack %u out of %u:%u\n", ack, tp->snd_una, tp->snd_nxt);