google / google/gvisor

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

Open
#14,092 1 comment 0 reactions 0 assignees View on GitHub
type: bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.