From f1ec69628afedc717545ca2d289939f688cdf2bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Duval?= Date: Tue, 11 May 2021 17:25:35 +0200 Subject: [PATCH] tcp: several fixes for SACK handling: * kMaxOptionSize: TCP header size is 60 bytes. process_options() would have accepted a longer header as permitted. add_options() would have possibly added more options or SACK entries as permitted. * tcp_segment: reserve memory for sack entries. * _SendQueued(): "fLastAcknowledgeSent <= fReceiveNext": also send SACK entries for received data when acknowledging a missing segment. * _SendQueue(): after acknowledging, cancel the delayed acknowledgment timer. * _Receive(): if calling Acknowledged() already triggered sending a segment including an acknowledgement (piggy-back), remove "immediate acknowledge" from the action. This helps to reduce empty ACKs for bidirectional streams. Change-Id: I32808fbe549be0f5b25bcf4be17cd91a640b8ec4 Reviewed-on: https://review.haiku-os.org/c/haiku/+/3906 Reviewed-by: Adrien Destugues --- .../network/protocols/tcp/TCPEndpoint.cpp | 25 ++++++++++++------- .../kernel/network/protocols/tcp/tcp.cpp | 16 ++++++------ .../kernel/network/protocols/tcp/tcp.h | 3 ++- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.cpp b/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.cpp index 0cd396d617..9881719088 100644 --- a/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.cpp +++ b/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.cpp @@ -1773,8 +1773,13 @@ TCPEndpoint::_Receive(tcp_segment_header& segment, net_buffer* buffer) } } - if (fState != CLOSED) + if (fState != CLOSED) { + tcp_sequence last = fLastAcknowledgeSent; _Acknowledged(segment); + // we just sent an acknowledge, remove from action + if (last < fLastAcknowledgeSent) + action &= ~IMMEDIATE_ACKNOWLEDGE; + } } } @@ -2024,15 +2029,15 @@ TCPEndpoint::_SendQueued(bool force, uint32 sendWindow) } // SACK information is embedded with duplicate acknowledgements - if (!fReceiveQueue.IsContiguous() && fLastAcknowledgeSent == fReceiveNext + if (!fReceiveQueue.IsContiguous() + && fLastAcknowledgeSent <= fReceiveNext && (fFlags & FLAG_OPTION_SACK_PERMITTED) != 0) { segment.options |= TCP_HAS_SACK; - int maxSackCount = 4 - ((fFlags & FLAG_OPTION_TIMESTAMP) != 0); - segment.sacks = (tcp_sack*)calloc(maxSackCount, sizeof(tcp_sack)); - if (segment.sacks != NULL) - segment.sackCount = fReceiveQueue.PopulateSackInfo(fReceiveNext, maxSackCount, segment.sacks); - else - segment.sackCount = 0; + int maxSackCount = MAX_SACK_BLKS + - ((fFlags & FLAG_OPTION_TIMESTAMP) != 0); + memset(segment.sacks, 0, sizeof(segment.sacks)); + segment.sackCount = fReceiveQueue.PopulateSackInfo(fReceiveNext, + maxSackCount, segment.sacks); } if ((segment.flags & TCP_FLAG_SYNCHRONIZE) != 0 @@ -2215,8 +2220,10 @@ TCPEndpoint::_SendQueued(bool force, uint32 sendWindow) shouldStartRetransmitTimer = false; } - if (segment.flags & TCP_FLAG_ACKNOWLEDGE) + if (segment.flags & TCP_FLAG_ACKNOWLEDGE) { fLastAcknowledgeSent = segment.acknowledge; + gStackModule->cancel_timer(&fDelayedAcknowledgeTimer); + } length -= segmentLength; segment.flags &= ~(TCP_FLAG_SYNCHRONIZE | TCP_FLAG_RESET diff --git a/src/add-ons/kernel/network/protocols/tcp/tcp.cpp b/src/add-ons/kernel/network/protocols/tcp/tcp.cpp index dabd26becf..5ad4014dfc 100644 --- a/src/add-ons/kernel/network/protocols/tcp/tcp.cpp +++ b/src/add-ons/kernel/network/protocols/tcp/tcp.cpp @@ -53,8 +53,8 @@ static EndpointManager* sEndpointManagers[AF_MAX]; static rw_lock sEndpointManagersLock; -// The TCP header length is at most 64 bytes. -static const int kMaxOptionSize = 64 - sizeof(tcp_header); +// The TCP header length is at most 60 bytes (0xf * 4). +static const int kMaxOptionSize = 60 - sizeof(tcp_header); /*! Returns an endpoint manager for the specified domain, if any. @@ -153,7 +153,6 @@ add_options(tcp_segment_header &segment, uint8 *buffer, size_t bufferSize) option->length = 2 + sackCount * sizeof(tcp_sack); memcpy(option->sack, segment.sacks, sackCount * sizeof(tcp_sack)); bump_option(option, length); - free(segment.sacks); } } @@ -221,12 +220,13 @@ process_options(tcp_segment_header &segment, net_buffer *buffer, size_t size) case TCP_OPTION_SACK: if (size >= option->length) { segment.options |= TCP_HAS_SACK; - segment.sackCount = (option->length - 2) / sizeof(tcp_sack); - segment.sacks = option->sack; - + segment.sackCount = min_c((option->length - 2) + / sizeof(tcp_sack), MAX_SACK_BLKS); for(int i = 0; i < segment.sackCount; ++i) { - segment.sacks[i].left_edge = ntohl(segment.sacks[i].left_edge); - segment.sacks[i].right_edge = ntohl(segment.sacks[i].right_edge); + segment.sacks[i].left_edge = ntohl( + option->sack[i].left_edge); + segment.sacks[i].right_edge = ntohl( + option->sack[i].right_edge); } } break; diff --git a/src/add-ons/kernel/network/protocols/tcp/tcp.h b/src/add-ons/kernel/network/protocols/tcp/tcp.h index 057462350a..826ed51ed6 100644 --- a/src/add-ons/kernel/network/protocols/tcp/tcp.h +++ b/src/add-ons/kernel/network/protocols/tcp/tcp.h @@ -255,7 +255,8 @@ struct tcp_segment_header { uint32 timestamp_value; uint32 timestamp_reply; - tcp_sack *sacks; +#define MAX_SACK_BLKS 4 + tcp_sack sacks[MAX_SACK_BLKS]; int sackCount; uint32 options;