From 558c1a45fe30ffe3d6c67b2321e0fc973f13e9b7 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Tue, 23 Jun 2009 01:29:43 +0100 Subject: [PATCH] [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 --- src/include/gpxe/tcp.h | 10 ++++++++++ src/net/tcp.c | 41 ++++++++++++++++++++++++++++------------- 2 files changed, 38 insertions(+), 13 deletions(-) diff --git a/src/include/gpxe/tcp.h b/src/include/gpxe/tcp.h index 6fb673ac..7ae7eab9 100644 --- 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 1de2abf0..29e233fa 100644 --- 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 );