netstack: TCP sender permanently leaks SACKed-then-cumulatively-ACKed packets into Outstanding outside recovery, throttling bulk sends under reordering
- Dominant language
- Go
- Stars
- 19.3k
- Forks
- 2k
- Avg merge
- 3d 5h
- Merged PRs (30d)
- 264
Description
### 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`:
```go
// 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
```shell
N/A — netstack used as a library (gvisor.dev/gvisor Go module), no runsc involved.
```
### docker version (if using docker)
```shell
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)
```shell
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)
```shell
```
Contributor guide
Research direction
Start in pkg/tcpip/transport/tcp/snd.go at sender.handleRcvdSegment's ACK-removal loop and review how recovery transitions account for Outstanding. Check the regression coverage referenced in #14101, then run tcp_sack_test.go and tcp_rack_test.go. Done means reordered, lossless traffic no longer accumulates phantom Outstanding packets while recovery accounting remains correct.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 25/100