From 190e04db0824a1da22ce9bd5028c52b7fbe22807 Mon Sep 17 00:00:00 2001 From: David Bell Date: Thu, 27 Aug 2026 23:25:23 +0300 Subject: [PATCH] tcp: implement the RFC 4015 response to spurious loss recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 3522 Eifel detection sets sender.spuriousRecovery and the SpuriousRecovery metrics, but nothing consumed the signal: leaveRecovery unconditionally deflated SndCwnd to Ssthresh, so a recovery the sender itself proved spurious still cost a permanent multiplicative decrease, and under sustained path jitter the sender ratchets its throughput down episode after episode. Capture pipe_prev (RFC 4015 Section 3 step (0)) at every loss-recovery entry point before the congestion controller reduces Ssthresh, and on exit from a recovery detected spurious restore Ssthresh to it and slow-start back from FlightSize + IW (step (9)) instead of keeping the deflation. The response is applied at the single point every recovery exits through — the Open transition in handleRcvdSegment — after the ACK-removal loop, when Outstanding is the true FlightSize, so the send it allows is bounded by IW. The capture is consumed on use, so the response applies at most once per episode. detectSpuriousRecovery now ignores ACKs carrying no Timestamps option: a TSEcr of zero previously compared as smaller than RetransmitTS and marked a genuine recovery spurious, which was harmless while the flag only fed a metric but wrong once a response consumes it. Extend the two spurious-recovery e2e tests to drive the connection out of recovery on each exit path and assert Ssthresh/SndCwnd are restored; both extensions fail without the fix. Fixes #14102 Assisted-by: Claude Code --- pkg/tcpip/transport/tcp/rack.go | 2 + pkg/tcpip/transport/tcp/snd.go | 88 +++++++++++++++++++ .../transport/tcp/test/e2e/tcp_sack_test.go | 53 +++++++++++ 3 files changed, 143 insertions(+) diff --git a/pkg/tcpip/transport/tcp/rack.go b/pkg/tcpip/transport/tcp/rack.go index b42539f4ec9..be2c7daef0c 100644 --- a/pkg/tcpip/transport/tcp/rack.go +++ b/pkg/tcpip/transport/tcp/rack.go @@ -275,6 +275,7 @@ func (s *sender) detectTLPRecovery(ack seqnum.Value, rcvdSeg *segment) { // Step 2. Either the original packet or the retransmission (in the // form of a probe) was lost. Invoke a congestion control response // equivalent to fast recovery. + s.capturePipePrev() s.cc.HandleLossDetected() s.enterRecovery() s.leaveRecovery() @@ -425,6 +426,7 @@ func (rc *rackControl) reorderTimerExpired() tcpip.Error { fastRetransmit := false if !rc.snd.FastRecovery.Active { + rc.snd.capturePipePrev() rc.snd.cc.HandleLossDetected() rc.snd.enterRecovery() fastRetransmit = true diff --git a/pkg/tcpip/transport/tcp/snd.go b/pkg/tcpip/transport/tcp/snd.go index 056c681d744..4d4e65b7194 100644 --- a/pkg/tcpip/transport/tcp/snd.go +++ b/pkg/tcpip/transport/tcp/snd.go @@ -184,6 +184,14 @@ type sender struct { // RFC3522 Section 3.2. retransmitTS uint32 + // pipePrev is the sender's estimate of the usable network pipe when + // loss recovery was last initiated, captured before the congestion + // controller reduced Ssthresh ("pipe_prev" in RFC4015 Section 3 step + // (0)). It is consumed, and reset to zero, by the RFC4015 response + // when a recovery that was detected spurious (RFC3522) ends. A value + // of zero means there is nothing to restore. + pipePrev int + // startCork start corking the segments. startCork bool @@ -620,6 +628,7 @@ func (s *sender) retransmitTimerExpired() tcpip.Error { // Record retransmitTS if the sender is not in recovery as per: // https://datatracker.ietf.org/doc/html/rfc3522#section-3.2 Step 2 s.recordRetransmitTS() + s.capturePipePrev() s.state = tcpip.RTORecovery s.cc.HandleRTOExpired() @@ -1175,6 +1184,63 @@ func (s *sender) leaveRecovery() { s.cc.PostRecovery() } +// capturePipePrev captures the pre-loss congestion control state before the +// congestion controller responds to a detected loss, so that the RFC4015 +// response can restore it if the recovery turns out to be spurious. It must +// be called before cc.HandleLossDetected/HandleRTOExpired reduce Ssthresh. +// +// See: https://datatracker.ietf.org/doc/html/rfc4015#section-3 step (0). +// +// +checklocks:s.ep.mu +func (s *sender) capturePipePrev() { + // Like RetransmitTS (RFC3522 Section 3.2 step 2), the capture must not + // be overwritten while a recovery is already in progress. + if s.inRecovery() { + return + } + // RFC4015 sets pipe_prev to max(FlightSize, ssthresh). SndCwnd stands + // in for FlightSize: netstack counts both in packets, and at loss + // detection the sender is cwnd-limited, while Outstanding may already + // have been decimated by the ACKs that triggered the detection. An + // Ssthresh that was never reduced (InitialSsthresh) holds no pipe + // estimate to restore and is skipped. + s.pipePrev = s.SndCwnd + if s.Ssthresh != InitialSsthresh && s.Ssthresh > s.pipePrev { + s.pipePrev = s.Ssthresh + } +} + +// undoSpuriousRecovery applies the RFC4015 congestion control response on +// exit from a loss recovery that was detected spurious (RFC3522): restore +// Ssthresh to the pre-loss pipe estimate and slow-start back to it from +// FlightSize + IW. It must run after the exit ACK's removal loop, when +// Outstanding is the true FlightSize, so that the send this allows is +// bounded by IW. +// +// cwnd is deliberately not restored to its pre-loss value: FlightSize is +// small at recovery exit, and sendData would emit the entire restored +// difference as a single line-rate burst, causing genuine loss. RFC4015's +// FlightSize + IW form exists to prevent exactly that burst. +// +// See: https://datatracker.ietf.org/doc/html/rfc4015#section-3 step (9). +// +// +checklocks:s.ep.mu +func (s *sender) undoSpuriousRecovery() { + if s.pipePrev == 0 { + return + } + if s.pipePrev > s.Ssthresh { + s.Ssthresh = s.pipePrev + } + // Outstanding can be transiently negative while an ACK for data sent + // before an RTO is being processed; FlightSize is never negative. + s.SndCwnd = max(s.Outstanding, 0) + InitialCwnd + // Consume the capture so the response applies at most once per + // recovery episode. + s.pipePrev = 0 + s.cc.PostRecovery() +} + // isAssignedSequenceNumber relies on the fact that we only set flags once a // sequencenumber is assigned and that is only done right before we send the // segment. As a result any segment that has a non-zero flag has a valid @@ -1297,6 +1363,7 @@ func (s *sender) detectLoss(seg *segment) (fastRetransmit bool) { s.DupAckCount = 0 return false } + s.capturePipePrev() s.cc.HandleLossDetected() s.enterRecovery() return true @@ -1462,6 +1529,16 @@ func (s *sender) detectSpuriousRecovery(hasDSACK bool, tsEchoReply uint32) { return } + // The Eifel detection algorithm is only defined when the TCP + // Timestamps option is enabled (RFC 3522 Section 3.2): it compares + // the ACK's echoed timestamp against RetransmitTS, so an ACK + // carrying no Timestamps option proves nothing about the + // retransmit. A TSEcr of zero is treated as absent, matching its + // treatment in RTT sampling. + if !s.ep.SendTSOk || tsEchoReply == 0 { + return + } + // See: https://datatracker.ietf.org/doc/html/rfc3522#section-3.2 Step 4 // // If the value of the Timestamp Echo Reply field of the acceptable ACK's @@ -1717,6 +1794,16 @@ func (s *sender) handleRcvdSegment(rcvdSeg *segment) { if !s.FastRecovery.Active { s.cc.Update(originalOutstanding-s.Outstanding, bestRTT, rcvdSeg.rcvdTime) if s.FastRecovery.Last.LessThan(s.SndUna) { + // Every recovery ends at this transition: pure + // RTO recovery directly, and fast/SACK recovery + // via leaveRecovery earlier in this same call. + // The removal loop has run, so Outstanding is + // the true FlightSize: apply the RFC4015 + // response here if the recovery was detected + // spurious (RFC3522). + if s.inRecovery() && s.spuriousRecovery { + s.undoSpuriousRecovery() + } s.state = tcpip.Open // Update RACK when we are exiting fast or RTO // recovery as described in the RFC @@ -1765,6 +1852,7 @@ func (s *sender) handleRcvdSegment(rcvdSeg *segment) { // If any segment is marked as lost by // RACK, enter recovery and retransmit // the lost segments. + s.capturePipePrev() s.cc.HandleLossDetected() s.enterRecovery() fastRetransmit = true diff --git a/pkg/tcpip/transport/tcp/test/e2e/tcp_sack_test.go b/pkg/tcpip/transport/tcp/test/e2e/tcp_sack_test.go index 6202a2aacc4..1b79d271508 100644 --- a/pkg/tcpip/transport/tcp/test/e2e/tcp_sack_test.go +++ b/pkg/tcpip/transport/tcp/test/e2e/tcp_sack_test.go @@ -716,6 +716,35 @@ func verifySpuriousRecoveryMetric(t *testing.T, c *context.Context, numSpuriousR } } +// verifyRecoveryUndo verifies that on exit from a loss recovery that was +// detected spurious (RFC3522), the RFC4015 response restored the congestion +// control state instead of leaving the loss-based reduction in place: +// Ssthresh back at the pre-recovery congestion window and SndCwnd no lower +// than it was before the recovery. +func verifyRecoveryUndo(t *testing.T, c *context.Context) { + t.Helper() + + pollFn := func() error { + info := tcpip.TCPInfoOption{} + if err := c.EP.GetSockOpt(&info); err != nil { + return fmt.Errorf("c.EP.GetSockOpt(&%T) = %s", info, err) + } + if got, want := info.CcState, tcpip.Open; got != want { + return fmt.Errorf("got info.CcState = %v, want = %v", got, want) + } + if got, want := info.SndSsthresh, uint32(tcp.InitialCwnd); got != want { + return fmt.Errorf("Ssthresh was not restored on spurious recovery exit: got info.SndSsthresh = %d, want = %d", got, want) + } + if got, want := info.SndCwnd, uint32(tcp.InitialCwnd); got < want { + return fmt.Errorf("SndCwnd was not restored on spurious recovery exit: got info.SndCwnd = %d, want >= %d", got, want) + } + return nil + } + if err := testutil.Poll(pollFn, 1*time.Second); err != nil { + t.Error(err) + } +} + func checkReceivedPacket(t *testing.T, c *context.Context, tcpHdr header.TCP, bytesRead uint32, b *buffer.View, data []byte) { payloadLen := uint32(len(tcpHdr.Payload())) checker.IPv4(t, b, @@ -742,6 +771,13 @@ func buildTSOptionFromHeader(tcpHdr header.TCP) []byte { func TestDetectSpuriousRecoveryWithRTO(t *testing.T) { probeDone := make(chan struct{}) probe := func(s *tcp.TCPEndpointState) { + select { + case <-probeDone: + // The ACK that exits recovery arrives after detection + // has already been verified. + return + default: + } if s.Sender.RetransmitTS == 0 { t.Fatalf("RetransmitTS did not get updated, got: 0 want > 0") } @@ -818,12 +854,24 @@ func TestDetectSpuriousRecoveryWithRTO(t *testing.T) { <-probeDone verifySpuriousRecoveryMetric(t, c, 1 /* numSpuriousRecovery */, 1 /* numSpuriousRTO */) + + // Acknowledge all outstanding data to exit RTO recovery and verify + // that the RFC4015 response restored the congestion control state. + c.SendAck(seq, numPackets*maxPayload) + verifyRecoveryUndo(t, c) } func TestSACKDetectSpuriousRecoveryWithDupACK(t *testing.T) { numAck := 0 probeDone := make(chan struct{}) probe := func(s *tcp.TCPEndpointState) { + select { + case <-probeDone: + // The ACK that exits recovery arrives after detection + // has already been verified. + return + default: + } if numAck < 3 { numAck++ return @@ -913,6 +961,11 @@ func TestSACKDetectSpuriousRecoveryWithDupACK(t *testing.T) { <-probeDone verifySpuriousRecoveryMetric(t, c, 1 /* numSpuriousRecovery */, 0 /* numSpuriousRTO */) + + // Acknowledge all outstanding data to exit SACK recovery and verify + // that the RFC4015 response restored the congestion control state. + c.SendAck(seq, numPackets*maxPayload) + verifyRecoveryUndo(t, c) } func TestNoSpuriousRecoveryWithDSACK(t *testing.T) {