Client TUN offload (1/3): pre-existing bug fixes - #502
Merged
Conversation
This was referenced Aug 7, 2026
|
Code coverage summary for 2e36b27: ✅ Region coverage 70% passes |
xv-thomas-leong
force-pushed
the
main-tun-offload-fixes
branch
3 times, most recently
from
August 7, 2026 03:38
d31d4c8 to
43f29ed
Compare
xv-thomas-leong
changed the title
xv-thomas-leong
marked this pull request as ready for review
August 7, 2026 05:22
kp-weiwen-goh
approved these changes
Aug 12, 2026
kp-mariappan-ramasamy
left a comment
kp-mariappan-ramasamy
left a comment
Contributor
There was a problem hiding this comment.
LGTM with minor nit!
Commits are using conventional commit format which we dont use in ligihtway repo.
Might be consistent to use module name as prefix as repo convention
xv-thomas-leong
force-pushed
the
main-tun-offload-fixes
branch
from
August 13, 2026 05:18
43f29ed to
44cc49b
Compare
…ader Pre-existing bug on main, independent of any offload feature work. When a TUN device negotiates `IFF_VNET_HDR` the kernel prepends a 10-byte `virtio_net_hdr` to every read, so a read needs `mtu + 10` bytes of buffer to carry a full-size packet. All three inside-path receive sites size their buffer to `mtu` alone, so the tail of a full-size packet is lost. The kernel does not report this. It copies what fits, returns that length, and sets no flag the callers inspect, so the shortfall is silent. Reproduced on Linux 6.17.0 x86_64 against a real TUN, interface MTU 1500, inbound IP packet exactly 1500 bytes: buffer=1500 read=1500 ip_bytes=1490 total_length=1500 TRUNCATED buffer=1510 read=1510 ip_bytes=1500 total_length=1500 intact Adds `Tun::vnet_headroom()`, returning `VIRTIO_NET_HDR_LEN` when the device negotiated the flag and 0 otherwise, surfaced on the inside-IO traits as a defaulted `vnet_headroom()` so both loops can size correctly without knowing which backend they hold. The three sites become `mtu + vnet_headroom()`: - `lightway-client/src/lib.rs` inside_io_task - `lightway-server/src/lib.rs` inside_io_loop - `TunDirect::recv_buf_many` app-utils Zero extra bytes unless offload is in use, and zero for the `IoUring` backend, which reads into its own pooled buffers. This is only half the story: with the header still not stripped on receive, the packet handed to the parser continues to start with 10 bytes of virtio header rather than the IP header. That is a separate defect in the same path, fixed next. Sizing the buffer first means that fix needs no caveat.
Pre-existing bug on main, independent of any offload feature work. The two TUN directions decided whether virtio framing was in play by consulting different things. `try_send` branches on `self.vnet_hdr`, a property of the device. The receive side had no such check: `recv_buf` truncated to the read length and never stripped the 10-byte header. Stripping happened only in `recv_gso`, i.e. it was implied by *which function the caller chose*, which in turn follows from a config flag selecting an inside loop. That leaves two independent sources of truth for one physical fact — does this fd use vnet framing — reconciled only by a single one-directional copy in `client()` that sets `tun_config.offload` from `enable_tun_offload` and never clears it. `ClientConfig::tun_config`, `TunConfig::offload` and `enable_tun_offload` are all public and `connect()` is public, so a consumer can set the first without the last and never execute that line: sends then work while every read hands the parser 10 bytes of virtio header where the IP header should be. `recv_buf` now branches on the same `self.vnet_hdr` bool, via a `strip_vnet_hdr` helper mirroring `recv_gso` — a read no shorter than the header is advanced past it, a shorter one is discarded and reported as `WouldBlock` so the caller's loop retries instead of erroring. Both directions now share one predicate, so a desynced config degrades to "works without GSO batching" rather than corrupting traffic. The non-offload path pays one bool test. Verified against a real TUN on Linux 6.17.0 x86_64, interface MTU 1500, inbound IP packet exactly 1500 bytes: raw read : 1510 bytes (10 hdr + 1500 ip) handed to caller : 1500 bytes, first byte 0x45 (version 4) ip total_length : 1500 -> matches, intact bare IP packet without this fix : parser saw first byte 0x01 (version 0) `strip_vnet_hdr` also compares the IPv4 length field against the bytes actually read. They disagree only if the buffer was not sized `mtu + vnet_headroom()` and the kernel truncated the tail, which is otherwise silent because the packet still looks well-formed. It warns rather than drops: with `TUN_F_TSO*` enabled the kernel may also deliver an aggregate here whose length field legitimately exceeds one MTU, and dropping those would trade a diagnostic for data loss. Also corrects the field doc, which said the header is 12 bytes. It is 10 (`size_of::<VirtioNetHdr>()`, matching the kernel's `TUNGETVNETHDRSZ` default); 12 would be `virtio_net_hdr_mrg_rxbuf`, never negotiated here. Known gap left open: the io-uring backend reads via `opcode::Read` on the raw fd, bypassing `recv_buf` entirely, so it does not strip. That combination is unreachable today — the client rejects offload with io-uring in validation, and the server's `as_gso()` guard aborts startup because `supports_gso()` is false for that backend — but it needs its own fix before offload with io-uring is ever allowed.
Pre-existing bug on main, independent of any offload feature work. A computed UDP checksum of zero was written to the wire as-is. RFC 768 requires it be transmitted as 0xFFFF. Why zero occurs: the internet checksum is the one's complement of the one's-complement sum, so the output is 0x0000 exactly when the folded sum is 0xFFFF. One's-complement arithmetic has two representations of zero, so landing on 0xFFFF is unremarkable -- roughly 1 in 65536 for content uncorrelated with the addresses and length. Why it matters more than that rate suggests: the sum is a deterministic function of the 5-tuple, length and payload. A flow repeatedly sending the same payload between the same endpoints produces the same checksum every time, so if it is zero then every packet in that flow is broken for as long as the pattern holds -- a permanent blackhole for one keepalive/query shape while everything else works. Impact by family. Over IPv6, 0x0000 is invalid (RFC 8200 s8.1) and the receiver must discard the datagram. Over IPv4 it is the "sender computed no checksum" sentinel, so the receiver skips verification and integrity protection silently disappears on exactly the packets that failed to get it. Note the kernel applies this substitution when it computes the checksum itself, so taking `NEEDS_CSUM` packets over inherited the obligation. The substitution is free: 0xFFFF and 0x0000 are the same value in one's complement, so no verifier can distinguish them. `csum_start`/`csum_offset` alone cannot tell UDP from TCP, and blanket substitution would be wrong because 0x0000 is a legal TCP checksum. So `ip_l4_proto` reads the IPv4 protocol byte or the IPv6 next-header field and the substitution applies only to protocol 17. For IPv6 carrying extension headers the value is the first extension header's number rather than the transport protocol, so the substitution is skipped -- the behaviour before it existed. That direction is safe; it can never mis-identify TCP as UDP, which is the only direction that corrupts. The test derives the seed that makes the fold land on zero rather than hard-coding a vector, so it cannot silently stop exercising the substitution if the surrounding arithmetic changes. Mutation-verified: removing the substitution fails exactly that test. Also corrects the `MAX_GSO_SEGS` doc, which asserted `UDP_MAX_SEGMENTS` *is* `1 << 6`. It is not fixed -- 64 before Linux 6.9, 128 from 6.9 onward (raised in kernel commit 1382e3b6a350, backported to stable) -- and 64 is pinned deliberately so a batch is accepted on both.
Pre-existing bug on main, independent of any offload feature work.
A superpacket with more than `MAX_GSO_SEGS` segments was dropped whole,
and the error was then swallowed upstream (`process_inside_packet` maps
`InvalidInsidePacket` to `continue`), so nothing surfaced. The inner TCP
flow retransmits into the same drop until congestion control backs off far
enough to build a smaller aggregate: sawtooth degradation on a path that
works fine with offload disabled.
Reachable with a sub-1024 MSS at a few hundred Mbps. The aggregate size is
`tcp_tso_autosize` -- roughly 1ms of data at the current pacing rate,
capped at 64KB -- not `65535 / MSS`, so it needs both a small MSS and a
high rate. At MSS 1350 the 64KB size cap yields 48 segments and the count
can never be reached at all.
The aggregate does not have to be dropped. The machinery already sends
correctly when no batch is open: `udp_send` passes straight through in
`Passthrough`, and the flush and reset are no-ops on an unopened buffer.
So the fix is to decide up front and simply not open the batch when the
segment count exceeds the cap:
gso_fits_one_batch(gso_segs) -> bool
On the fallback leg each segment goes out via `send_to_outside` -- the
same call the non-offload path makes for an individual packet -- so the
aggregate still reaches the peer, just without wire batching. One `warn!`
plus a metric per skipped aggregate keeps the degradation visible.
`gso_dropped_iov_overflow` is replaced by `gso_batch_skipped`, because the
condition is no longer a drop and the old name would misreport it. Its doc
also described an `IOV_MAX`-derived cap, which was never what
`MAX_GSO_SEGS` bounds.
`gso_fits_one_batch` is a pure predicate specifically so the decision is
unit-testable without standing up a live wolfSSL connection; a boundary
test covers it, including the 122-segment small-MSS case.
Note the activity timestamp is updated on both legs -- it is deliberately
outside the `if batched` block, since a fallback send is still traffic.
Pre-existing bug on main, independent of any offload feature work. `gso_type` in the virtio header is a bitfield, not an enum: Linux ORs `VIRTIO_NET_HDR_GSO_ECN` (0x80) into it for ECN-marked flows, so an ECN-marked TCPv4 aggregate arrives as 0x81. The server's inside GSO loop compared it raw against `VIRTIO_NET_HDR_GSO_NONE`, which means an ECN-marked single packet (0x80) fails the comparison and is misclassified as an aggregate -- routed to `inside_data_received_gso` instead of `inside_data_received`. `gso.rs`'s own `is_tcp()` already masked the bit before comparing, so the codebase knew the flag shares that field; this call site did not. Adds `VirtioNetHdr::is_gso_none()` next to `is_tcp()` and routes the comparison through it. Putting the mask on the type rather than at the call site also keeps the ECN constant private to the module that owns it, so callers in other crates never need to mirror a kernel ABI value. In practice the kernel only sets ECN when `skb_is_gso()`, so a bare 0x80 should not occur -- but the two comparison styles for the same field were an accident waiting to be inherited by the next call site, and there are more of those coming.
xv-thomas-leong
force-pushed
the
main-tun-offload-fixes
branch
from
August 17, 2026 02:51
44cc49b to
ddb8fdf
Compare
kp-mariappan-ramasamy
approved these changes
Aug 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 1 of 3 — pre-existing bug fixes
First of a three-PR stack that adds client-side TUN GSO/GRO offload. This
part lands only the bug fixes for issues that already exist on
main,independent of the offload feature. Each commit message calls out that it
is a pre-existing bug and how it is fixed. Kept in front and reviewable on
its own so the fixes can merge without waiting on the feature work.
See the stack button right below the title to navigate between PRs.
How Has This Been Tested?
CI and Manually tested
Types of changes
Checklist:
main