Skip to content

the definitive tun offloads branch - #1704

Draft
JackDoan wants to merge 110 commits into
masterfrom
better-tun-interface-ordering
Draft

the definitive tun offloads branch#1704
JackDoan wants to merge 110 commits into
masterfrom
better-tun-interface-ordering

Conversation

@JackDoan

@JackDoan JackDoan commented May 4, 2026

Copy link
Copy Markdown
Collaborator

formerly known as "biggest headache yet"

@JackDoan
JackDoan requested a review from nbrownus May 4, 2026 17:35
@JackDoan
JackDoan force-pushed the better-tun-interface-ordering branch from ef9c707 to e0c6f44 Compare May 6, 2026 16:00
@rawdigits

Copy link
Copy Markdown
Collaborator

Bug: sendNoMetrics panics on relayed connections with small packets (e.g. TestReply)

Panic:

panic: runtime error: slice bounds out of range [:48] with capacity 32

goroutine 57 [running]:
github.com/slackhq/nebula.(*Interface).sendNoMetrics(...)
    inside.go:569

Root cause: sendNoMetrics saves fullOut := out before offsetting out for the relay header. After EncryptDanger, if the buffer was too small (cap < header + ciphertext + AEAD tag), Go allocates a new backing array for out. But fullOut still points at the original small buffer. Then fullOut[:header.Len+len(out)] overflows.

MTU-sized buffers (normal data packets) always have enough capacity so EncryptDanger writes in-place — fullOut and out share the same array and everything works. Small packets like TestReply (~32 bytes) don't — EncryptDanger reallocates, fullOut goes stale, panic.

Conditions to trigger: (1) relayed tunnel (no direct path between peers), AND (2) small packet (TestReply health-check ping is the common case). Direct tunnels never hit the relay branch; large packets never trigger reallocation.

Fix (9 lines, applies cleanly to current PR HEAD):

-			f.SendVia(relayHostInfo, relay, out, nb, fullOut[:header.Len+len(out)], true)
+			needed := header.Len + len(out)
+			if needed <= cap(fullOut) {
+				f.SendVia(relayHostInfo, relay, out, nb, fullOut[:needed], true)
+			} else {
+				// EncryptDanger reallocated out into a new backing array, so fullOut
+				// no longer contains the encrypted payload. Build a new relay buffer.
+				relayBuf := make([]byte, needed)
+				copy(relayBuf[header.Len:], out)
+				f.SendVia(relayHostInfo, relay, out, nb, relayBuf, true)
+			}

Happy path (buffer big enough) is identical behavior — just guarded by a cap check. The else branch handles the reallocation edge case.

Hit this in production on a host behind residential NAT where peer tunnels are relayed through lighthouses. Gateway routers with public IPs never trigger it because their tunnels are direct.

@JackDoan

JackDoan commented May 6, 2026

Copy link
Copy Markdown
Collaborator Author

@rawdigits fixed this a layer up by making relay outbounds also use the tx batcher, which also puts them on the fast path

@JackDoan
JackDoan force-pushed the better-tun-interface-ordering branch from f9fcb74 to b0dd5f1 Compare May 11, 2026 15:55
Comment thread firewall/cache.go Outdated
@JackDoan
JackDoan force-pushed the better-tun-interface-ordering branch 2 times, most recently from 04b5b6d to 220eae7 Compare May 11, 2026 16:11
Comment thread main.go Outdated
}

//todo no merge
go http.ListenAndServe(":6060", nil)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't merge this!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what's causing the e2e fails

@JackDoan
JackDoan force-pushed the better-tun-interface-ordering branch 3 times, most recently from ba2e61b to 6b6dcda Compare May 11, 2026 16:33
@JackDoan
JackDoan force-pushed the better-tun-interface-ordering branch from c08ed7d to 118294a Compare July 13, 2026 20:11
@JackDoan
JackDoan force-pushed the better-tun-interface-ordering branch from 4039484 to 49028cb Compare July 14, 2026 16:01
Comment thread pprof_debug.go
@@ -0,0 +1,33 @@
//go:build debug

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably shouldn't merge in this PR either but it makes clod shut up when asking for reviews

@JackDoan JackDoan changed the title biggest headache yet the definitive tun offloads branch Jul 15, 2026
@JackDoan
JackDoan force-pushed the better-tun-interface-ordering branch 2 times, most recently from 18dc13b to c8bad57 Compare July 17, 2026 20:40
JackDoan and others added 30 commits July 29, 2026 17:12
Both newTunGeneric error branches closed only the tun fd; the
Add-failure branch left the freshly created QueueSet (and its
shutdown eventfd) orphaned.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ugV2edVqoz3tBvq9J6yWp
gsoTypeFromProto returns GSO_NONE when the IP version nibble is
neither 4 nor 6, so a multi-fragment superpacket went out as one
silent jumbo GSO_NONE packet -- exactly the silent mis-emission the
geometry checks promise not to allow.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ugV2edVqoz3tBvq9J6yWp
decodeRead failures were silently swallowed; a kernel emitting an
unnegotiated GSO type would blackhole all tun traffic with nothing in
the logs. Debug-gated per the usual idiom so the happy path pays
nothing, which means plumbing the logger down through the offload
queueset.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ugV2edVqoz3tBvq9J6yWp
A failed IPV6_RECVTCLASS probe disabled ECN RX entirely, taking down
working IPv4 (v4-mapped) delivery with it, while the opposite failure
already only degraded. Treat both directions the same: each family
degrades to Not-ECT independently, and only a full-family failure
turns the cmsg parsing off.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ugV2edVqoz3tBvq9J6yWp
sendInsideEncrypt ran connectionManager.Out for every segment -- up to
~45 extra atomic stores per TSO superpacket, all inside writeLock when
boring crypto serializes encryption. One mark in sendInsideMessage
covers the whole superpacket on both the direct and relay paths.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ugV2edVqoz3tBvq9J6yWp
A slot that stays single-segment is byte-identical to the packet it
was seeded from, but flushSlot re-emitted it via WriteGSO with a
seeded pseudo-sum, forcing the kernel to software-checksum up to
~1400B that arrived with a perfectly valid checksum. Keep the borrowed
seed packet on the slot (valid until Flush per the Commit contract)
and emit it through the plain DATA_VALID path when numSeg is still 1
at flush time. appendPayload and mergeSlots only touch hdrBuf once
numSeg >= 2, so the raw bytes are pristine whenever the fast path
fires. This is every non-coalesced TCP/UDP packet: request/response
flows, many-flow fan-in, and each run's leftover tail.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ugV2edVqoz3tBvq9J6yWp
Every non-coalesceable in-flow packet evicted the flow's open slot, so
a bidirectional connection's inbound data run was broken by each peer
ACK interleaved into it, largely defeating coalescing on concurrent
upload+download. A bare acknowledgment (zero payload, nothing beyond
ACK|PSH|ECE) carries no ordering obligation toward the flow's data --
delivered late it is just a stale ACK the receiver ignores -- so it
can ride the lane as a passthrough without the evict, same as kernel
GRO, which doesn't flush held data on pure ACKs. SYN/FIN/RST/CWR keep
sealing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ugV2edVqoz3tBvq9J6yWp
- multi_coalesce/batch: the ordering contract now states what Flush
  actually guarantees -- per-flow DATA order -- and names the two
  shapes later data may legally overtake (pure ACKs by design, and
  unparseable in-flow shapes as an accepted tradeoff).
- validVnetHdr claimed DATA_VALID makes the stack skip L4 checksum
  verification; the tun write path ignores that bit entirely. What the
  header buys is the absence of NEEDS_CSUM.
- tun_darwin Write said "only valid for single threaded use"; it is
  concurrency-safe and concurrent callers exist.
- udp_coalesce eviction comment said "Seal it" but never sets sealed.
- recordCapability: note the gauges are process-global while the state
  is per-socket (last writer wins).
- drop a stale tunReadBufSize reference.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ugV2edVqoz3tBvq9J6yWp
Behavior untouched; each marker records a known gap and the intended
fix so the next visit doesn't rediscover it: transient zero-sent
sendmmsg errors drop a whole run; the non-vnet Poll queue lacks the
post-wake drain loop; recvmmsg controllen resets touch every entry;
cached handshake packets flush one syscall each; the routines clamp in
activate() would blackhole surplus REUSEPORT sockets if it ever became
reachable; darwin WriteBatch burst-drops on EWOULDBLOCK.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ugV2edVqoz3tBvq9J6yWp
A partial success left the remaining entries' iovecs, sockaddrs, and
cmsgs fully intact, then threw them away and replanned the remainder
from bufs -- doubling the packing work exactly when the socket is
congested. Give sendFn a start offset so the drain resumes the same
prepared array at the first unsent entry, and skip a kernel-rejected
entry in place the same way. Only the GSO-disable path still replans,
since its entries change shape; it now rewinds precisely to the failed
run instead of the whole chunk, so entries already sent are never
duplicated.

New scripted tests pin the two paths that didn't exist before: a mid-
chunk rejected entry (drop it, resume the rest, start offsets advance)
and a mid-chunk EIO (GSO off, replay only the failed run, no dup of
already-sent packets).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ugV2edVqoz3tBvq9J6yWp
The tunnel's real bottleneck queue - the UDP receive buffer feeding the
decrypt loop - is invisible to every kernel AQM, so under overload it
regulates ECN-capable flows with tail-drop loss like it's 1993. Sample
SK_MEMINFO once per recvmmsg batch (tunnels.ecn_mark_threshold, fraction
of rcvbuf, 0=off) and treat depth beyond the threshold as an outer CE:
the existing RFC 6040 fold then CE-marks ECT inner packets and senders
back off without loss.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants