Client TUN offload (2/3): refactorings - #503
Merged
Merged
Conversation
This was referenced Aug 7, 2026
|
Code coverage summary for a14557d: ✅ Region coverage 70% passes |
xv-thomas-leong
force-pushed
the
main-tun-offload-refactor
branch
from
August 7, 2026 03:05
1c2f53d to
bf2a883
Compare
xv-thomas-leong
force-pushed
the
main-tun-offload-refactor
branch
from
August 7, 2026 03:16
bf2a883 to
89c1389
Compare
xv-thomas-leong
force-pushed
the
main-tun-offload-refactor
branch
from
August 7, 2026 03:38
89c1389 to
10b37d8
Compare
xv-thomas-leong
changed the title
xv-thomas-leong
force-pushed
the
main-tun-offload-refactor
branch
from
August 7, 2026 04:24
10b37d8 to
56ef56b
Compare
xv-thomas-leong
force-pushed
the
main-tun-offload-refactor
branch
from
August 13, 2026 05:18
56ef56b to
c040d4c
Compare
xv-thomas-leong
force-pushed
the
main-tun-offload-refactor
branch
from
August 17, 2026 02:51
c040d4c to
3cc731b
Compare
xv-thomas-leong
force-pushed
the
main-tun-offload-refactor
branch
from
August 17, 2026 04:13
3cc731b to
db6a027
Compare
xv-thomas-leong
marked this pull request as ready for review
August 17, 2026 04:22
| /// in one vectored send — no copy, no allocation. The returned count | ||
| /// excludes the header, matching a plain send. | ||
| #[cfg(target_os = "linux")] | ||
| fn send_with_vnet_hdr(&self, hdr_bytes: &[u8], buf: &[u8]) -> IOCallbackResult<usize> { |
Contributor
There was a problem hiding this comment.
Might be better to rename it to send_chunks() and take a list of buffers ?
| p[at] = 0; | ||
| p[at + 1] = 0; | ||
| !checksum(&p[START..], initial) == 0 | ||
| let mut p = build(6, c); |
Contributor
There was a problem hiding this comment.
Avoid magic numbers and use macro like berfore IPPROTO_UDP
| let csum = match (v4_addrs, v6_addrs) { | ||
| (Some((src, dst)), None) => { | ||
| pnet_packet::udp::ipv4_checksum(&udp.to_immutable(), &src, &dst) | ||
| transport_checksum(&src.octets(), &dst.octets(), 17, udp.packet()) |
Contributor
There was a problem hiding this comment.
Avoid magic numbers. Also applicable to othe places around this
xv-thomas-leong
marked this pull request as draft
August 17, 2026 07:45
xv-thomas-leong
force-pushed
the
main-tun-offload-refactor
branch
2 times, most recently
from
August 19, 2026 04:03
bda0169 to
1ea2dc5
Compare
xv-thomas-leong
marked this pull request as ready for review
August 19, 2026 05:33
xv-thomas-leong
marked this pull request as draft
August 19, 2026 05:38
xv-thomas-leong
force-pushed
the
main-tun-offload-refactor
branch
from
August 19, 2026 05:43
1ea2dc5 to
772b83f
Compare
xv-thomas-leong
marked this pull request as ready for review
August 19, 2026 05:44
kp-mariappan-ramasamy
previously approved these changes
Aug 20, 2026
kp-mariappan-ramasamy
left a comment
kp-mariappan-ramasamy
left a comment
Contributor
There was a problem hiding this comment.
LGTM
Changes are functionally good. Minor nits about commit cleanliness.
| return Some(Message::IpPktinfo(pi)); | ||
| } | ||
|
|
||
| Some(Message::Unknown(item)) |
Contributor
There was a problem hiding this comment.
This change is unnecessary churn, not needed
Contributor
Author
There was a problem hiding this comment.
Thanks. This is a leftover change when separating changes between refactoring and feature. I've removed it.
xv-thomas-leong
force-pushed
the
main-tun-offload-refactor
branch
from
August 20, 2026 06:28
772b83f to
66b0111
Compare
xv-thomas-leong
marked this pull request as draft
August 20, 2026 06:36
xv-thomas-leong
force-pushed
the
main-tun-offload-refactor
branch
from
August 21, 2026 03:56
66b0111 to
13a87b4
Compare
Pure code motion: relocate the control-message module verbatim from lightway-server to lightway-app-utils so other crates can use it, make its items `pub`, and update the import paths in the server's UDP send and batched-receive paths. No behaviour change.
Rename `Buffer::as_mut` to `spare_capacity_mut` so the name states what it returns (the uninitialised spare capacity, not a `&mut Self`), and add a `Default` impl. Callers in the UDP send and batched-receive paths updated. No behaviour change.
Two soundness fixes that matter once the module is public API: - Back `Buffer<N>` with an inline `#[repr(C, align(16))]` array instead of a `BytesMut`. The old code was aligned for `cmsghdr` only because the global allocator happens to hand back 16-byte-aligned blocks at these sizes; nothing in the type system guaranteed it. A `const` assert confirms 16 suffices for `cmsghdr` on the target. - Bounds-check the control length in `iter` and route iteration through a new `iter_control(&[u8])` that `assert!`s the buffer is aligned for a `cmsghdr` (kept in release, not `debug_assert!`, since `Iter::next` turns the pointers into `&cmsghdr` and a misaligned reference is UB). The old path called `BytesMut::set_len` with an unchecked kernel- supplied length, which could produce a slice over out-of-bounds memory. `Iter` drops its now-unused `const N` parameter. Adds a test asserting both buffer types meet `cmsghdr` alignment.
Now that the cmsg module is public API, review flagged the undocumented items. Add doc comments to `Buffer::iter` and `BufferMut::builder`, and extend the module doc to note it also covers `sendmsg(2)`.
The three-arm mapping from `io::Result<usize>` onto `IOCallbackResult` appears once today and is about to appear twice more as the vnet-header send paths land. Extract it first so those commits add a call rather than a copy. Mirrors `map_send_result` on the outside UDP path, which already does this. Note it takes no `len` parameter: unlike the UDP side it has no swallow-and-report-success arms, because a TUN write has no transient errors worth hiding from the caller. No behaviour change.
Pull the `try_io(READABLE)` WouldBlock/Interrupted/ICMP loop out of `recv_bufs` into `try_readable_io`, so the upcoming batched GRO receive shares it instead of copying the arms. No functional change. The connected-UDP `send()` path is left as-is: it has grown its own connected-socket and dead-socket handling, and the GSO send path introduces its own result mapping where it is used.
Pull the source/DNS rewrite + ConnectionError mapping into process_inside_packet and the tracer-trigger bookkeeping into TracerTrigger, so an upcoming GSO variant of the inside loop can share them instead of copying ~100 lines. No functional change.
Replaces the hand-rolled one's-complement loop used by
`gso_none_checksum`, and pnet's protocol-aware helpers used by
`build_segment`, with a single `internet_checksum::Checksum`-based
`transport_checksum` covering the TCP/UDP pseudo-header for both address
families.
pnet's helpers walk the packet per 16-bit word with bounds checks on each
access; `internet-checksum` accumulates wider and uses the carry flag, and
on x86_64 a 128-bit accumulator. Equivalence was checked by differential
fuzzing against `pnet_packet::{tcp,udp}::ipv4_checksum` -- 40,000 random
TCP and UDP cases including odd payload lengths, on x86_64 -- with one
deliberate difference noted below.
Two details worth recording, because both are easy to get wrong:
`Checksum::add_bytes` buffers a trailing byte across calls, so an
odd-length slice mid-stream is handled correctly rather than being padded
in place. docs.rs is misleading on this point; the source is not.
`checksum()` returns `to_ne_bytes()` of a `from_ne_bytes()`-accumulated
sum. That is RFC 1071 s2 byte-order independence, not a bug: on
little-endian the property yields wire order, and on big-endian native
order *is* wire order. So both `copy_from_slice(&c.checksum())` and
`u16::from_be_bytes(c.checksum())` are correct, on either endianness.
`transport_checksum` carries the RFC 768 zero substitution that
`gso_none_checksum` already applies, and needs no protocol sniffing to do
it since the protocol is a parameter here. That is the one deliberate
difference from pnet, which does not substitute and is therefore wrong for
the ~1-in-65536 case; the doc comment now scopes the equivalence claim
accordingly.
`gso_none_checksum`'s test derived its zero-folding seed from the old
internals, which this commit removes. It now derives the same seed by
calling `gso_none_checksum` itself on a TCP packet, where no substitution
applies -- implementation-agnostic, so it survives the next change to how
the sum is computed.
Also adds a `debug_assert` that the transport slice fits the 16-bit
pseudo-header length field. Unreachable from `build_segment`, whose slices
are bounded by `gso_size`, but the pnet code this replaces accumulated the
length in `u32` and so could not wrap.
The vnet-hdr branch of try_send allocated a fresh BytesMut and memcpy'd the whole packet just to prepend the 10-byte zeroed virtio header. Send header and packet as two IoSlices in one vectored write instead — no copy, no allocation. No functional change (identical bytes reach the device).
xv-thomas-leong
force-pushed
the
main-tun-offload-refactor
branch
from
August 21, 2026 04:00
13a87b4 to
ca63247
Compare
xv-thomas-leong
marked this pull request as ready for review
August 21, 2026 04:15
kp-mariappan-ramasamy
approved these changes
Aug 21, 2026
kp-mariappan-ramasamy
left a comment
kp-mariappan-ramasamy
left a comment
Contributor
There was a problem hiding this comment.
LGTM, Thank you
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 2 of 3 — refactorings
Second of a three-PR stack that adds client-side TUN GSO/GRO offload.
Base this on part 1 (bug fixes). This part is pure refactoring — no
behaviour change — to prepare the ground for the feature work in part 3:
hoisting shared cmsg handling, extracting the TUN send/receive syscall
plumbing into reusable helpers, and switching GSO checksums to the
internet-checksumcrate. Reviewing it separately keeps the feature difffocused on new behaviour.
No functional change. Each commit is independently buildable; the full
stack passes
cargo clippy --workspace --all-targets(0 diagnostics) andcargo test --workspace.How Has This Been Tested?
CI and Manually tested
Types of changes
Checklist:
main