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) {