[tcp] Improve robustness in the presence of duplicated received packets

gPXE responds to duplicated ACKs with an immediate retransmission,
which can lead to a sorceror's apprentice syndrome.  It also responds
to out-of-range (or old duplicate) ACKs with a RST, which can cause
valid connections to be dropped.

Fix the sorceror's apprentice syndrome by leaving the retransmission
timer running (and so inhibiting the immediate retransmission) when we
receive a potential duplicate ACK.  This seems to match the behaviour
of Linux observed via wireshark traces.

Fix the RST issue by sending RST only on out-of-range ACKs that occur
before the connection is fully established, as per RFC 793.

These problems were exposed during development of the 802.11 wireless
link layer; the 802.11 protocol has a failure mode that can easily
cause duplicated packets.  The fixes were tested in a controlled way
by faking large numbers of duplicated packets in the rtl8139 driver.

Originally-fixed-by: Joshua Oreman <oremanj@rwcr.net>

diff --git a/src/include/gpxe/tcp.h b/src/include/gpxe/tcp.h
index 6fb673a..7ae7eab 100644 (file)
--- a/src/include/gpxe/tcp.h
+++ b/src/include/gpxe/tcp.h
@@ -229,6 +229,16 @@ struct tcp_options {
                        TCP_STATE_SENT ( TCP_FIN ) ) )                      \
          == TCP_STATE_ACKED ( TCP_SYN ) )
 
+/** Have ever been fully established
+ *
+ * We have been fully established if we have both received a SYN and
+ * had our own SYN acked.
+ */
+#define TCP_HAS_BEEN_ESTABLISHED(state)                                            \
+       ( ( (state) & ( TCP_STATE_ACKED ( TCP_SYN ) |                       \
+                       TCP_STATE_RCVD ( TCP_SYN ) ) )                      \
+         == ( TCP_STATE_ACKED ( TCP_SYN ) | TCP_STATE_RCVD ( TCP_SYN ) ) )
+
 /** Have closed gracefully
  *
  * We have closed gracefully if we have both received a FIN and had

diff --git a/src/net/tcp.c b/src/net/tcp.c
index 1de2abf..29e233f 100644 (file)
--- a/src/net/tcp.c
+++ b/src/net/tcp.c
@@ -704,30 +704,45 @@ static int tcp_rx_ack ( struct tcp_connection *tcp, uint32_t ack,
        size_t len;
        unsigned int acked_flags;
 
-       /* Ignore duplicate or out-of-range ACK */
-       if ( ack_len > tcp->snd_sent ) {
-               DBGC ( tcp, "TCP %p received ACK for [%08x,%08zx), "
-                      "sent only [%08x,%08x)\n", tcp, tcp->snd_seq,
-                      ( tcp->snd_seq + ack_len ), tcp->snd_seq,
-                      ( tcp->snd_seq + tcp->snd_sent ) );
-               return -EINVAL;
-       }
-
-       /* Acknowledge any flags being sent */
+       /* Determine acknowledged flags and data length */
        len = ack_len;
        acked_flags = ( TCP_FLAGS_SENDING ( tcp->tcp_state ) &
                        ( TCP_SYN | TCP_FIN ) );
        if ( acked_flags )
                len--;
 
+       /* Stop retransmission timer if necessary */
+       if ( ack_len == 0 ) {
+               /* Duplicate ACK (or just a packet that isn't
+                * intending to ACK any new data).  If the
+                * retransmission timer is running, leave it running
+                * so that we don't immediately retransmit and cause a
+                * sorceror's apprentice syndrome.
+                */
+       } else if ( ack_len <= tcp->snd_sent ) {
+               /* ACK of new data.  Stop the retransmission timer. */
+               stop_timer ( &tcp->timer );
+       } else {
+               /* Out-of-range (or old duplicate) ACK.  Leave the
+                * timer running, as for the ack_len==0 case, to
+                * handle old duplicate ACKs.
+                */
+               DBGC ( tcp, "TCP %p received ACK for [%08x,%08zx), "
+                      "sent only [%08x,%08x)\n", tcp, tcp->snd_seq,
+                      ( tcp->snd_seq + ack_len ), tcp->snd_seq,
+                      ( tcp->snd_seq + tcp->snd_sent ) );
+               /* Send RST if an out-of-range ACK is received on a
+                * not-yet-established connection.
+                */
+               if ( ! TCP_HAS_BEEN_ESTABLISHED ( tcp->tcp_state ) )
+                       return -EINVAL;
+       }
+
        /* Update SEQ and sent counters, and window size */
        tcp->snd_seq = ack;
        tcp->snd_sent = 0;
        tcp->snd_win = win;
 
-       /* Stop the retransmission timer */
-       stop_timer ( &tcp->timer );
-
        /* Remove any acknowledged data from transmit queue */
        tcp_process_queue ( tcp, len, NULL, 1 );