Skip to content

Commit

Permalink
[tcp] Treat ACKs as sent only when successfully transmitted
Browse files Browse the repository at this point in the history 
iPXE currently forces sending (i.e. sends a pure ACK even in the
absence of fresh data to send) only in response to packets that
consume sequence space or that lie outside of the receive window.
This ignores the possibility that a previous ACK was not actually sent
(due to, for example, the retransmission timer running).

This does not cause incorrect behaviour, but does cause unnecessary
retransmissions from our peer.  For example:

 1. Peer sends final data packet (ack      106 seq 521..523)
 2. We send FIN                  (seq 106..107 ack      523)
 3. Peer sends FIN               (ack      106 seq 523..524)
 4. We send nothing since retransmission timer is running for our FIN
 5. Peer ACKs our FIN            (ack      107 seq 524..524)
 6. We send nothing since this packet consumes no sequence space
 7. Peer retransmits FIN         (ack      107 seq 523..524)
 8. We ACK peer's FIN            (seq 107..107 ack      524)

What should happen at step (6) is that we should ACK the peer's FIN,
since we can deduce that we have never sent this ACK.

Fix by maintaining an "ACK pending" flag that is set whenever we are
made aware that our peer needs an ACK (whether by consuming sequence
space or by sending a packet that appears out of order), and is
cleared only when the ACK packet has been transmitted.

Reported-by: Piotr Jaroszyński <p.jaroszynski@gmail.com>
Signed-off-by: Michael Brown <mcb30@ipxe.org>
  • Loading branch information
@mcb30
mcb30 committed Jul 15, 2010
1 parent 7550594 commit f033694
Showing 1 changed file with 20 additions and 21 deletions.
41 changes: 20 additions & 21 deletions src/net/tcp.c
View file
Expand Up @@ -93,6 +93,8 @@ enum tcp_flags {
TCP_XFER_CLOSED = 0x0001,
/** TCP timestamps are enabled */
TCP_TS_ENABLED = 0x0002,
/** TCP acknowledgement is pending */
TCP_ACK_PENDING = 0x0004,
};

/**
Expand Down Expand Up @@ -396,15 +398,14 @@ static size_t tcp_process_queue ( struct tcp_connection *tcp, size_t max_len,
* Transmit any outstanding data
*
* @v tcp TCP connection
* @v force_send Force sending of packet
*
* Transmits any outstanding data on the connection.
*
* Note that even if an error is returned, the retransmission timer
* will have been started if necessary, and so the stack will
* eventually attempt to retransmit the failed packet.
*/
static int tcp_xmit ( struct tcp_connection *tcp, int force_send ) {
static int tcp_xmit ( struct tcp_connection *tcp ) {
struct io_buffer *iobuf;
struct tcp_header *tcphdr;
struct tcp_mss_option *mssopt;
Expand Down Expand Up @@ -438,7 +439,7 @@ static int tcp_xmit ( struct tcp_connection *tcp, int force_send ) {
tcp->snd_sent = seq_len;

/* If we have nothing to transmit, stop now */
if ( ( seq_len == 0 ) && ! force_send )
if ( ( seq_len == 0 ) && ! ( tcp->flags & TCP_ACK_PENDING ) )
return 0;

/* If we are transmitting anything that requires
Expand Down Expand Up @@ -519,6 +520,9 @@ static int tcp_xmit ( struct tcp_connection *tcp, int force_send ) {
return rc;
}

/* Clear ACK-pending flag */
tcp->flags &= ~TCP_ACK_PENDING;

return 0;
}

Expand Down Expand Up @@ -552,7 +556,7 @@ static void tcp_expired ( struct retry_timer *timer, int over ) {
tcp_close ( tcp, -ETIMEDOUT );
} else {
/* Otherwise, retransmit the packet */
tcp_xmit ( tcp, 0 );
tcp_xmit ( tcp );
}
}

Expand Down Expand Up @@ -709,6 +713,7 @@ static void tcp_rx_seq ( struct tcp_connection *tcp, uint32_t seq_len ) {
} else {
tcp->rcv_win = 0;
}
tcp->flags |= TCP_ACK_PENDING;
}

/**
Expand Down Expand Up @@ -927,7 +932,6 @@ static int tcp_rx ( struct io_buffer *iobuf,
struct tcp_options options;
size_t hlen;
uint16_t csum;
uint32_t start_seq;
uint32_t seq;
uint32_t ack;
uint32_t win;
Expand Down Expand Up @@ -967,7 +971,7 @@ static int tcp_rx ( struct io_buffer *iobuf,

/* Parse parameters from header and strip header */
tcp = tcp_demux ( ntohs ( tcphdr->dest ) );
start_seq = seq = ntohl ( tcphdr->seq );
seq = ntohl ( tcphdr->seq );
ack = ntohl ( tcphdr->ack );
win = ntohs ( tcphdr->win );
flags = tcphdr->flags;
Expand Down Expand Up @@ -1002,6 +1006,12 @@ static int tcp_rx ( struct io_buffer *iobuf,
}
}

/* Force an ACK if this packet is out of order */
if ( ( tcp->tcp_state & TCP_STATE_RCVD ( TCP_SYN ) ) &&
( seq != tcp->rcv_ack ) ) {
tcp->flags |= TCP_ACK_PENDING;
}

/* Handle SYN, if present */
if ( flags & TCP_SYN ) {
tcp_rx_syn ( tcp, seq, &options );
Expand Down Expand Up @@ -1031,19 +1041,8 @@ static int tcp_rx ( struct io_buffer *iobuf,
/* Dump out any state change as a result of the received packet */
tcp_dump_state ( tcp );

/* Send out any pending data. We force sending a reply if either
*
* a) the peer is expecting an ACK (i.e. consumed sequence space), or
* b) either end of the packet was outside the receive window
*
* Case (b) enables us to support TCP keepalives using
* zero-length packets, which we would otherwise ignore. Note
* that for case (b), we need *only* consider zero-length
* packets, since non-zero-length packets will already be
* caught by case (a).
*/
tcp_xmit ( tcp, ( ( start_seq != seq ) ||
( ( seq - tcp->rcv_ack ) > tcp->rcv_win ) ) );
/* Send out any pending data */
tcp_xmit ( tcp );

/* If this packet was the last we expect to receive, set up
* timer to expire and cause the connection to be freed.
Expand Down Expand Up @@ -1087,7 +1086,7 @@ static void tcp_xfer_close ( struct tcp_connection *tcp, int rc ) {
tcp_close ( tcp, rc );

/* Transmit FIN, if possible */
tcp_xmit ( tcp, 0 );
tcp_xmit ( tcp );
}

/**
Expand Down Expand Up @@ -1125,7 +1124,7 @@ static int tcp_xfer_deliver ( struct tcp_connection *tcp,
list_add_tail ( &iobuf->list, &tcp->queue );

/* Transmit data, if possible */
tcp_xmit ( tcp, 0 );
tcp_xmit ( tcp );

return 0;
}
Expand Down

0 comments on commit f033694

Please sign in to comment.