netstack: TCP sender permanently leaks SACKed-then-cumulatively-ACKed packets into Outstanding outside recovery, throttling bulk sends under reordering

[davidbell217]

Description

In sender.handleRcvdSegment's ACK-removal loop (pkg/tcpip/transport/tcp/snd.go), a segment that was previously SACKed and is then cumulatively ACKed only decrements SackedOut:

// If SACK is enabled then only reduce outstanding if
// the segment was not previously SACKED as these have
// already been accounted for in SetPipe().
if !s.ep.SACKPermitted || !s.ep.scoreboard.IsSACKED(seg.sackBlock()) {
	s.Outstanding -= s.pCount(seg, s.MaxPayloadSize)
} else {
	s.SackedOut -= s.pCount(seg, s.MaxPayloadSize)
}

The comment's premise only holds during loss recovery: SetPipe() opens with if !s.ep.SACKPermitted || !s.FastRecovery.Active { return }, so outside recovery it never recomputes Outstanding, and nothing else removes those packets. A segment SACKed outside recovery and then cumulatively ACKed therefore leaks its packet count into Outstanding permanently.

That pattern is exactly what benign reordering produces when RACK is doing its job and not declaring loss: later segments get SACKed, the hole fills, the cumulative ACK covers everything — and Outstanding gains phantom packets on every occurrence. The only healers are entering recovery (SetPipe) or a full pipe drain (SndUna == SndNxt resets Outstanding to 0). A long-lived bulk transfer hits neither, so the leak accumulates until the phantoms consume the congestion window and sendData's s.Outstanding < s.SndCwnd gate admits only the (cwnd − phantom) remainder per RTT.

Expected: Outstanding tracks packets actually in flight; a sender on a reordering-but-lossless path stays cwnd-limited by real flight size.

Observed (state snapshot of a wedged sender, long after its only recovery episode ended, not in recovery):

SndNxt - SndUna = 6,700 B  (≈5 packets actually in flight)
Outstanding=86  SndCwnd=86   (sender believes it is cwnd-limited)
peer's advertised window 8 MB open, zero RTOs (ACKs keep re-arming the resend timer)
SackedOut = -102             (mirror of the same double-count)

Even transfers that complete are silently throttled: one finished with Outstanding=1016 against ~84 packets genuinely in flight — 932 phantoms, i.e. running at ~8 % of the believed window.

Suggested fix — corrected after implementing it against the test suite: the one-liner this issue originally suggested (decrement Outstanding unconditionally in the removal loop's SACKed branch) is wrong, and so is the other obvious one-liner. Both were implemented and measured:

• Unconditional decrement in the removal loop's SACKed branch (originally suggested here): the premise "SetPipe() recomputes Outstanding while processing the same ACK" fails exactly at recovery exit — leaveRecovery() runs before the ACK-removal loop, so segments SACKed during recovery (already excluded from Outstanding by SetPipe()) get subtracted a second time when the recovery-exiting ACK covers them. Breaks TestSACKRecovery: post-recovery Outstanding is under-counted and the sender transmits beyond the expected window.
• Decrement at SACK arrival (walkSACK): frees cwnd slots before the cumulative ACK — effectively unbounded limited transmit — and only covers RACK-enabled senders. Breaks TestRACKWithWindowFull with a transmission after a zero-window ACK.

What works: a sender-local counter of packets newly SACKed while no recovery is in progress (the exact population SetPipe() never accounts for), consumed when those segments are cumulatively ACKed, reset wherever Outstanding is recomputed or reset wholesale (recovery entry, RTO, full window drain). Recovery-path accounting is untouched; both RACK-on and RACK-off senders are fixed. With that shape the repro table below still holds (re-validated: 0/12 wedged, ~1.7 s median) and tcp_sack_test.go / tcp_rack_test.go pass. Implemented with a regression test in #14101.

Related but distinct: #9778, #10343 (RACK performance with Windows receivers).

Steps to reproduce

Netstack is used as a library (no runsc): two stack.Stack instances (SACK enabled, RACK at its default TCPRACKLossDetection, Reno, channel endpoints), a bulk TCP send from A to B through a UDP relay that impairs the path: 195 ms RTT, 3 % of packets delayed +30 ms (reordering only — zero loss), 50 Mbit/s rate cap. Send 4 MiB, 12 runs:

	wedged (>15 s)	median
as shipped	3/12	9.18 s
with the fix above	0/12	1.68 s

Retransmits ≈ 0 in both arms — the throttle is pure pipe accounting, not loss handling. Instrumenting Outstanding vs SndNxt−SndUna shows the phantom count growing by roughly the per-reordering-event SACKed-segment count and never shrinking outside recovery.

runsc version

N/A — netstack used as a library (gvisor.dev/gvisor Go module), no runsc involved.

docker version (if using docker)

N/A

uname

Darwin 25.4.0 arm64 (repro); also observed on windows/amd64 — the defect is platform-independent netstack code.

kubectl (if using Kubernetes)

N/A

repo state (if built from source)

gvisor.dev/gvisor v0.0.0-20260701204157-69c2d17aea96; the cited code is unchanged on master as of 2026-08-12.

runsc debug logs (if available)

[davidbell217]

Updated the "Suggested fix" section in the body: the originally suggested one-liner double-subtracts segments SetPipe() already accounted for (SACKed during recovery, cumulatively ACKed at/after recovery exit) and breaks TestSACKRecovery. The corrected shape — a counter of packets SACKed outside recovery, consumed at their cumulative ACK — is implemented with a regression test in #14101.