From 38055f5e6de3c04e54a706a0eb2f8db8748063d1 Mon Sep 17 00:00:00 2001 From: Tom Henderson Date: Thu, 12 Sep 2024 11:42:15 +0530 Subject: [PATCH] tcp: Prevent DupAck classification on segments with data Credits to Neal Cardwell for finding the issue. --- src/internet/model/tcp-socket-base.cc | 15 +++++++++++---- src/internet/model/tcp-socket-base.h | 4 +++- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/internet/model/tcp-socket-base.cc b/src/internet/model/tcp-socket-base.cc index 433d27c45..94355ae83 100644 --- a/src/internet/model/tcp-socket-base.cc +++ b/src/internet/model/tcp-socket-base.cc @@ -1932,9 +1932,11 @@ TcpSocketBase::ReceivedAck(Ptr packet, const TcpHeader& tcpHeader) NS_LOG_INFO("Update bytes in flight before processing the ACK."); BytesInFlight(); + bool receivedData = packet->GetSize() > 0; + // RFC 6675 Section 5: 2nd, 3rd paragraph and point (A), (B) implementation // are inside the function ProcessAck - ProcessAck(ackNumber, (bytesSacked > 0), currentDelivered, oldHeadSequence); + ProcessAck(ackNumber, (bytesSacked > 0), currentDelivered, oldHeadSequence, receivedData); m_tcb->m_isRetransDataAcked = false; if (m_congestionControl->HasCongControl()) @@ -1952,7 +1954,7 @@ TcpSocketBase::ReceivedAck(Ptr packet, const TcpHeader& tcpHeader) } // If there is any data piggybacked, store it into m_rxBuffer - if (packet->GetSize() > 0) + if (receivedData) { ReceivedData(packet, tcpHeader); } @@ -1966,7 +1968,8 @@ void TcpSocketBase::ProcessAck(const SequenceNumber32& ackNumber, bool scoreboardUpdated, uint32_t currentDelivered, - const SequenceNumber32& oldHeadSequence) + const SequenceNumber32& oldHeadSequence, + bool receivedData) { NS_LOG_FUNCTION(this << ackNumber << scoreboardUpdated << currentDelivered << oldHeadSequence); // RFC 6675, Section 5, 2nd paragraph: @@ -1992,10 +1995,14 @@ TcpSocketBase::ProcessAck(const SequenceNumber32& ackNumber, * (a) the ACK is carrying a SACK block that identifies previously * unacknowledged and un-SACKed octets between HighACK (TCP.UNA) and * HighData (m_highTxMark) + * + * The check below implements conditions a), b), and d), and c) is prevented by virtue of not + * reaching this code if SYN or FIN is set, and e) is not supported. */ bool isDupack = m_sackEnabled ? scoreboardUpdated - : ackNumber == oldHeadSequence && ackNumber < m_tcb->m_highTxMark; + : (ackNumber == oldHeadSequence && + ackNumber < m_tcb->m_highTxMark && !receivedData); NS_LOG_DEBUG("ACK of " << ackNumber << " SND.UNA=" << oldHeadSequence << " SND.NXT=" << m_tcb->m_nextTxSequence diff --git a/src/internet/model/tcp-socket-base.h b/src/internet/model/tcp-socket-base.h index a5cb1e7c7..89b14138a 100644 --- a/src/internet/model/tcp-socket-base.h +++ b/src/internet/model/tcp-socket-base.h @@ -1084,11 +1084,13 @@ class TcpSocketBase : public TcpSocket * \param oldHeadSequence value of HeadSequence before ack * updated with SACK information * \param currentDelivered The number of bytes (S)ACKed + * \param receivedData if true indicates that data is piggybacked with ACK */ virtual void ProcessAck(const SequenceNumber32& ackNumber, bool scoreboardUpdated, uint32_t currentDelivered, - const SequenceNumber32& oldHeadSequence); + const SequenceNumber32& oldHeadSequence, + bool receivedData); /** * \brief Recv of a data, put into buffer, call L7 to get it if necessary