tcp: fix SYN-retransmit sequence bug + enable/seed TCP checksum offload - #56
Draft
sarsanaee wants to merge 1 commit into
Draft
tcp: fix SYN-retransmit sequence bug + enable/seed TCP checksum offload#56sarsanaee wants to merge 1 commit into
sarsanaee wants to merge 1 commit into
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019xDGAYTziq2pwPgsEaqzEM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #55. Fixes the two defects that stop the native TCP handshake from working on real hardware (review items #3 and #2).
#3 — SYN retransmission corrupts the sequence number
SendSynsent the SYN fromsnd_nxt_and then didsnd_nxt_++on every call.PeriodicCheckretransmits viaSendSyn, so after a single lost/slow SYN the retransmit carriedseq = isn+1and bumpedsnd_nxt_toisn+2. The peer's SYN-ACK (ackingisn+1) then failed theseg_ack == snd_nxt_check inHandleSynSent, and the connection could never establish once one SYN was lost.Fix: send the SYN from the fixed
snd_isn_and setsnd_nxt_ = snd_isn_ + 1absolutely — mirroring the already-idempotentSendSynAck. Retransmits are now sequence-idempotent.#2 — TCP checksum offload was never enabled, and the pseudo-header wasn't seeded
The port was configured with only
IPV4 | UDPchecksum offload, yet the TCP TX path setRTE_MBUF_F_TX_TCP_CKSUMand wrotechecksum = 0with no 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 invalid and the Linux peer drops it. It only "worked" on NICs (mlx5) that recompute L4 checksums in hardware regardless.Fix, two parts:
pmd.cc: enableRTE_ETH_TX_OFFLOAD_TCP_CKSUMon the port when the NIC supports it (loud warning otherwise).tcp_flow.h: seedtcph->checksumwithrte_ipv4_phdr_cksum()in every TCP sender (control, MSS-option, data, FIN), satisfying the DPDK offload contract (SW pre-loads the pseudo-header sum; HW completes it over the TCP header + payload).checksumstays a rawuint16_tholding a network-order partial sum — wrapping it inbe16_t(as an earlier review comment suggested) would byte-swap it and re-break it.Test
Adds
SynRetransmitIsSequenceIdempotent: forces a SYN retransmit, assertssnd_nxt_is unchanged, and that a SYN-ACK ackingisn+1then establishes the connection. This fails on the old code.Linux + DPDK target, authored on macOS — not compiled or run. Needs a build +
ctestpass, and the checksum path should be validated on real hardware (tcpdump on the peer, or thetcp_msg_geninterop test). A software checksum fallback for NICs lacking TCP offload is a follow-up.Note on stacking
Based on
tcp-retransmit-wnd(#55) because the checksum seeding must also cover the new data-path senders (SendDataSegment/SendFinSegment) introduced there. Merge #55 first, or rebase ontotcp-7-02-2026if these land independently.🤖 Generated with Claude Code