From a518f1baa573bd1d67c69643d0d281e90fe9178e Mon Sep 17 00:00:00 2001 From: Alireza Sanaee Date: Sat, 18 Jul 2026 20:25:51 +0100 Subject: [PATCH] tcp: fix SYN-retransmit sequence bug + enable/seed TCP checksum offload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These are the two defects that stop the handshake from working on real hardware (review items #3 and #2 from the PR #54 review). #3 SYN retransmission (tcp_flow.h): SendSyn sent the SYN from snd_nxt_ and then did snd_nxt_++ on EVERY call. PeriodicCheck retransmits via SendSyn, so after a single lost/slow SYN the retransmit carried seq=isn+1 and bumped snd_nxt_ to isn+2; the peer's SYN-ACK (acking isn+1) then failed the seg_ack == snd_nxt_ check in HandleSynSent and the connection could never establish. Fix: send the SYN from the fixed snd_isn_ and set snd_nxt_ = snd_isn_ + 1 absolutely, mirroring the already-idempotent SendSynAck. Retransmits are now sequence-idempotent. #2 TCP checksum offload (pmd.cc + tcp_flow.h): the port was configured with only IPV4+UDP checksum offload, yet the TCP TX path set RTE_MBUF_F_TX_TCP_CKSUM and wrote checksum=0 without the pseudo-header sum. On any PMD that honors the port config (ixgbe/i40e/virtio), the NIC never computes the TCP checksum, so every segment ships with an invalid checksum and the Linux peer drops it — the stack only "worked" on NICs (mlx5) that recompute L4 checksums in HW. Fix: (a) enable RTE_ETH_TX_OFFLOAD_TCP_CKSUM on the port when supported (warn otherwise), and (b) seed tcph->checksum with rte_ipv4_phdr_cksum() in every TCP sender (control, MSS-option, data, FIN) so the offload contract is met. checksum stays a raw uint16_t (network-order partial sum) — wrapping it in be16_t would byte-swap it. Adds SynRetransmitIsSequenceIdempotent: forces a SYN retransmit, asserts snd_nxt_ is unchanged, and that a SYN-ACK acking isn+1 then establishes the connection (fails on the old code). Stacked on tcp-retransmit-wnd (#55). Not yet compiled — Linux+DPDK target, authored on macOS; needs a build + ctest pass, and the checksum path should be validated on real hardware (e.g. tcpdump on the peer / the tcp_msg_gen interop test). A software checksum fallback for NICs without TCP offload is left as a follow-up. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_019xDGAYTziq2pwPgsEaqzEM --- src/core/drivers/dpdk/pmd.cc | 13 +++++++++++++ src/core/tcp_flow_test.cc | 29 +++++++++++++++++++++++++++++ src/include/tcp_flow.h | 28 ++++++++++++++++++++++++++-- 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/src/core/drivers/dpdk/pmd.cc b/src/core/drivers/dpdk/pmd.cc index 216f646a..0960fc5f 100644 --- a/src/core/drivers/dpdk/pmd.cc +++ b/src/core/drivers/dpdk/pmd.cc @@ -71,6 +71,19 @@ static rte_eth_conf DefaultEthConf(const rte_eth_dev_info *devinfo) { port_conf.txmode.offloads = (RTE_ETH_TX_OFFLOAD_IPV4_CKSUM | RTE_ETH_TX_OFFLOAD_UDP_CKSUM); + // TCP TX checksum offload for the native TCP transport. The TCP datapath + // sets RTE_MBUF_F_TX_TCP_CKSUM per mbuf and pre-loads the pseudo-header + // checksum, but the NIC only completes the checksum if the offload was + // enabled here at port-configure time; otherwise every TCP segment ships with + // an invalid checksum and the peer drops it. Enable it when supported. + if (tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) { + port_conf.txmode.offloads |= RTE_ETH_TX_OFFLOAD_TCP_CKSUM; + } else { + LOG(WARNING) << "NIC does not support TCP TX checksum offload; the native " + "TCP transport will emit invalid checksums until a software " + "checksum fallback is added."; + } + if (tx_offload_capa & RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE) { // TODO(ilias): Add option to the constructor to enable this offload. LOG(WARNING) diff --git a/src/core/tcp_flow_test.cc b/src/core/tcp_flow_test.cc index aa031c22..a2baaada 100644 --- a/src/core/tcp_flow_test.cc +++ b/src/core/tcp_flow_test.cc @@ -360,6 +360,35 @@ TEST_F(TcpFlowTest, ActiveOpen_SynAckWrongAck) { dpdk::Packet::Free(bad_syn_ack); } +// A retransmitted SYN must reuse the original sequence number, not consume a +// fresh one. Previously SendSyn incremented snd_nxt_ on every call, so after a +// single retransmit the peer's SYN-ACK (acking isn+1) no longer matched +// snd_nxt_ and the handshake could never complete. +TEST_F(TcpFlowTest, SynRetransmitIsSequenceIdempotent) { + auto flow = MakeFlow(); + flow->InitiateHandshake(); + const uint32_t isn = flow->snd_isn_; + ASSERT_EQ(flow->snd_nxt_, isn + 1); + + // Force an RTO to retransmit the SYN. + for (uint32_t i = 0; i < TcpFlow::kInitialRTO; i++) { + EXPECT_TRUE(flow->PeriodicCheck()); + } + EXPECT_TRUE(flow->PeriodicCheck()); // RTO fires → SYN retransmit. + + // The retransmit must NOT have advanced the sequence number. + EXPECT_EQ(flow->snd_nxt_, isn + 1); + EXPECT_EQ(flow->state(), TcpFlow::State::kSynSent); + + // A SYN-ACK acking isn+1 now completes the handshake (it would have been + // rejected as a wrong-ack before the fix). + auto* syn_ack = MakePacket(9000, isn + 1, Tcp::kSyn | Tcp::kAck); + flow->InputPacket(syn_ack); + EXPECT_EQ(flow->state(), TcpFlow::State::kEstablished); + EXPECT_EQ(flow->snd_una_, isn + 1); + dpdk::Packet::Free(syn_ack); +} + // ═══════════════════════════════════════════════════════════════ // Passive Open (Server) Handshake // ═══════════════════════════════════════════════════════════════ diff --git a/src/include/tcp_flow.h b/src/include/tcp_flow.h index d3914f62..da4a8f25 100644 --- a/src/include/tcp_flow.h +++ b/src/include/tcp_flow.h @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -519,6 +520,20 @@ class TcpFlow { tcph->urgent_ptr = be16_t(0); } + /// Seed the TCP checksum field with the IPv4 pseudo-header partial checksum. + /// The DPDK TX offload contract for RTE_MBUF_F_TX_TCP_CKSUM (non-TSO) is: + /// software pre-loads the pseudo-header sum here, hardware completes it over + /// the TCP header + payload. Leaving it 0 (valid for UDP, where checksums + /// are optional) yields an invalid TCP checksum that the peer drops. Must run + /// after PrepareL3Header (IP total_length final) and offload_tcpv4_csum(). + /// @note tcph->checksum is a raw uint16_t holding a network-order partial + /// checksum — do NOT wrap it in be16_t, which would byte-swap it. + void FinalizeTcpChecksum(dpdk::Packet* packet) const { + auto* ipv4h = packet->head_data(sizeof(Ethernet)); + auto* tcph = packet->head_data(sizeof(Ethernet) + sizeof(Ipv4)); + tcph->checksum = rte_ipv4_phdr_cksum(ipv4h, /*ol_flags=*/0); + } + // ──────────────── Helpers: Send Control Packets ──────────────── void SendControlPacket(uint32_t seq, uint32_t ack, uint8_t flags) { @@ -532,6 +547,7 @@ class TcpFlow { PrepareL3Header(packet); PrepareL4Header(packet, seq, ack, flags); packet->offload_tcpv4_csum(); + FinalizeTcpChecksum(packet); txring_->SendPackets(&packet, 1); } @@ -563,13 +579,19 @@ class TcpFlow { std::memcpy(&opts[2], &mss_net, sizeof(mss_net)); packet->offload_tcpv4_csum(); + FinalizeTcpChecksum(packet); txring_->SendPackets(&packet, 1); } void SendSyn() { - SendControlPacketWithMSS(snd_nxt_, 0, Tcp::kSyn, + // Always send the SYN from the fixed ISN and set snd_nxt_ absolutely, so a + // retransmitted SYN (from PeriodicCheck) reuses the same sequence number + // instead of consuming a fresh one. Incrementing on every call drifted + // snd_nxt_ past the peer's ack after the first retransmit, permanently + // breaking the handshake. Mirrors SendSynAck, which is already idempotent. + SendControlPacketWithMSS(snd_isn_, 0, Tcp::kSyn, static_cast(kDefaultMSS)); - snd_nxt_++; // SYN consumes one sequence number. + snd_nxt_ = snd_isn_ + 1; // SYN consumes one sequence number. } void SendSynAck() { @@ -632,6 +654,7 @@ class TcpFlow { PrepareL3Header(packet); PrepareL4Header(packet, seq, rcv_nxt_, Tcp::kAck | Tcp::kPsh); packet->offload_tcpv4_csum(); + FinalizeTcpChecksum(packet); txring_->SendPackets(&packet, 1); return true; } @@ -652,6 +675,7 @@ class TcpFlow { PrepareL3Header(packet); PrepareL4Header(packet, snd_nxt_, rcv_nxt_, Tcp::kFin | Tcp::kAck); packet->offload_tcpv4_csum(); + FinalizeTcpChecksum(packet); txring_->SendPackets(&packet, 1); return true; }