[gPXE-devel] [PATCHv4 08/10] [tcp] Use a dedicated timer for the TIME_WAIT state

[Guo-Fu Tseng]

From: Michael Brown <mcb30 at ipxe.org>

iPXE currently repurposes the retransmission timer to hold the TCP
connection in the TIME_WAIT state (i.e. waiting for up to 2*MSL in
case we are required to re-ACK our peer's FIN due to a lost ACK).
However, the fact that this timer is running will prevent such an ACK
from ever being sent, since the logic in tcp_xmit() assumes that a
running timer indicates that we ourselves are waiting for an ACK and
so blocks the transmission.  (We always wait for an ACK before sending
our next packet, to keep our transmit data path as simple as
possible.)

Fix by using an entirely separate timer for the TIME_WAIT state, so
that packets can still be sent.

Reported-by: Piotr Jaroszyński <p.jaroszynski at gmail.com>
Signed-off-by: Michael Brown <mcb30 at ipxe.org>
Merged-by: Guo-Fu Tseng <cooldavid at cooldavid.org>
Signed-off-by: Guo-Fu Tseng <cooldavid at cooldavid.org>
---
 src/net/tcp.c |   42 +++++++++++++++++++++++++++++++++---------
 1 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/src/net/tcp.c b/src/net/tcp.c
index f25a83e..ddd7397 100644
--- a/src/net/tcp.c
+++ b/src/net/tcp.c
@@ -96,6 +96,8 @@ struct tcp_connection {
 	struct list_head queue;
 	/** Retransmission timer */
 	struct retry_timer timer;
+	/** Shutdown (TIME_WAIT) timer */
+	struct retry_timer wait;
 
 	/** Activity started **/
 	int act_started;
@@ -109,6 +111,7 @@ static LIST_HEAD ( tcp_conns );
 /* Forward declarations */
 static struct xfer_interface_operations tcp_xfer_operations;
 static void tcp_expired ( struct retry_timer *timer, int over );
+static void tcp_wait_expired ( struct retry_timer *timer, int over );
 static int tcp_rx_ack ( struct tcp_connection *tcp, uint32_t ack,
 			uint32_t win );
 
@@ -257,6 +260,7 @@ static int tcp_open ( struct xfer_interface *xfer, struct sockaddr *peer,
 	ref_init ( &tcp->refcnt, NULL );
 	xfer_init ( &tcp->xfer, &tcp_xfer_operations, &tcp->refcnt );
 	timer_init ( &tcp->timer, tcp_expired );
+	timer_init ( &tcp->wait, tcp_wait_expired );
 	tcp->prev_tcp_state = TCP_CLOSED;
 	tcp->tcp_state = TCP_STATE_SENT ( TCP_SYN );
 	tcp_dump_state ( tcp );
@@ -563,13 +567,12 @@ static int tcp_xmit ( struct tcp_connection *tcp, int force_send ) {
 /**
  * Retransmission timer expired
  *
- * @v timer	Retry timer
- * @v over	Failure indicator
+ * @v timer		Retransmission timer
+ * @v over		Failure indicator
  */
 static void tcp_expired ( struct retry_timer *timer, int over ) {
 	struct tcp_connection *tcp =
 		container_of ( timer, struct tcp_connection, timer );
-	int graceful_close = TCP_CLOSED_GRACEFULLY ( tcp->tcp_state );
 
 	DBGC ( tcp, "TCP %p timer %s in %s for %08x..%08x %08x\n", tcp,
 	       ( over ? "expired" : "fired" ), tcp_state ( tcp->tcp_state ),
@@ -579,15 +582,13 @@ static void tcp_expired ( struct retry_timer *timer, int over ) {
 		 ( tcp->tcp_state == TCP_SYN_RCVD ) ||
 		 ( tcp->tcp_state == TCP_ESTABLISHED ) ||
 		 ( tcp->tcp_state == TCP_FIN_WAIT_1 ) ||
-		 ( tcp->tcp_state == TCP_TIME_WAIT ) ||
 		 ( tcp->tcp_state == TCP_CLOSE_WAIT ) ||
 		 ( tcp->tcp_state == TCP_CLOSING ) ||
 		 ( tcp->tcp_state == TCP_LAST_ACK ) );
 
-	if ( over || graceful_close ) {
-		/* If we have finally timed out and given up, or if
-		 * this is the result of a graceful close, terminate
-		 * the connection
+	if ( over ) {
+		/* If we have finally timed out and given up,
+		 * terminate the connection
 		 */
 		tcp->tcp_state = TCP_CLOSED;
 		tcp_dump_state ( tcp );
@@ -600,6 +601,28 @@ static void tcp_expired ( struct retry_timer *timer, int over ) {
 }
 
 /**
+ * Shutdown timer expired
+ *
+ * @v timer		Shutdown timer
+ * @v over		Failure indicator
+ */
+static void tcp_wait_expired ( struct retry_timer *timer, int over __unused ) {
+	struct tcp_connection *tcp =
+		container_of ( timer, struct tcp_connection, wait );
+
+	assert ( tcp->tcp_state == TCP_TIME_WAIT );
+
+	DBGC ( tcp, "TCP %p wait complete in %s for %08x..%08x %08x\n", tcp,
+	       tcp_state ( tcp->tcp_state ), tcp->snd_seq,
+	       ( tcp->snd_seq + tcp->snd_sent ), tcp->rcv_ack );
+
+	tcp->tcp_state = TCP_CLOSED;
+	tcp_dump_state ( tcp );
+	tcp_close ( tcp );
+	tcp_xfer_shutdown ( tcp, 0 );
+}
+
+/**
  * Send RST response to incoming packet
  *
  * @v in_tcphdr		TCP header of incoming packet
@@ -1087,7 +1110,8 @@ static int tcp_rx ( struct io_buffer *iobuf,
 		tcp_close ( tcp );
 		tcp_xfer_shutdown ( tcp, 0 );
 	} else if ( TCP_CLOSED_GRACEFULLY ( tcp->tcp_state ) ) {
-		start_timer_fixed ( &tcp->timer, ( 2 * TCP_MSL ) );
+		stop_timer ( &tcp->wait );
+		start_timer_fixed ( &tcp->wait, ( 2 * TCP_MSL ) );
 	}
 
 	return 0;
-- 
1.7.1