docs(spike): gvproxy v0.8.9 switch-attachment protocol for DM2 - #512

MEDIUM — Correctness: tap_to_switch and switch_to_tap use ? but implicitly return (). The ? operator requires the enclosing function to return a type implementing FromResidual (i.e. Result or similar); () does not. These functions will not compile as written.

Fix — add an explicit Result return type and a terminal Ok(()):

async fn tap_to_switch<R, W>(mut tap: R, mut sock: W) -> anyhow::Result<()>
where
    R: AsyncReadExt + Unpin,
    W: AsyncWriteExt + Unpin,
{
    let mut buf = vec![0u8; 1518 + 14];
    loop {
        let n = tap.read(&mut buf).await?;
        if n == 0 { break; }
        let len = (n as u16).to_le_bytes();
        sock.write_all(&len).await?;
        sock.write_all(&buf[..n]).await?;
    }
    Ok(())
}

The same fix applies to switch_to_tap (line 280). With both functions returning anyhow::Result<()>, tokio::spawn yields JoinHandle<anyhow::Result<()>>, so tokio::try_join!(t1, t2)? in attach_to_switch must also unwrap the inner results:

let (r1, r2) = tokio::try_join!(t1, t2)?;  // propagates JoinError
r1?; r2?;  // propagates relay I/O errors

Since this sketch is the stated U1-T2 implementation target, the compile error will block the implementer immediately.

LOW — Correctness: std::os::unix::net::UnixStream is imported but never used — the code connects via tokio::net::UnixStream on line 239. This would produce an unused_imports warning. The import can be removed.

LOW — Correctness: tokio::fs::File wraps all I/O through a blocking thread pool (spawn_blocking + pread/pwrite), not async I/O. A TAP character device does not support pread/pwrite (it requires plain read/write), and the thread-pool approach occupies a blocking thread per relay direction continuously.

For async-native TAP relay the correct abstraction is tokio::io::unix::AsyncFd with the fd set to non-blocking mode, then using readable()/writable() guards:

use tokio::io::unix::AsyncFd;
let tap_file = unsafe { std::fs::File::from_raw_fd(tap_fd.into_raw_fd()) };
tap_file.set_nonblocking(true)?;
let async_tap = AsyncFd::new(tap_file)?;

This gives epoll-driven readiness notification rather than a blocking thread per fd. Worth noting for U1-T2 (#496) since the relay runs continuously for every OwnIp PTask.

LOW · correctness — The comment // MTU + Ethernet header does not match the expression 1518 + 14.

• Max Ethernet frame a TAP device returns (kernel strips FCS, MTU=1500): 1514 bytes = 1500 (payload) + 14 (Ethernet header).
• 1518 is the IEEE 802.3 wire maximum already including the 14-byte header; adding + 14 again gives 1532 and double-counts the header.

The over-sized buffer is harmless, but the comment misleads the implementer. When U1-T2 implements this relay, consider:

let mut buf = vec![0u8; 1518]; // max Ethernet frame excl. FCS; +4 covers 802.1Q VLAN

or 1514 if VLAN tagging is not expected on the gvproxy switch.

LOW · correctness — The 2-byte length prefix and frame payload are sent as two separate write_all calls.

If the unix socket closes between these two writes (e.g., gvproxy shuts down at the exact wrong moment), gvproxy will have consumed the 2-byte length but not the frame, permanently desynchronising its framing state for any subsequent reconnect on the same socket.

For the U1-T2 implementation, prefer a single vectored write or pre-allocate a combined buffer:

// Option A: vectored
sock.write_vectored(&[IoSlice::new(&len), IoSlice::new(&buf[..n])]).await?;
// Option B: prefix into buf
let mut framed = Vec::with_capacity(2 + n);
framed.extend_from_slice(&len);
framed.extend_from_slice(&buf[..n]);
sock.write_all(&framed).await?;

This is a design note for the real implementation, not a bug in the spike sketch.

MEDIUM — Correctness

The annotated flag table describes -listen-vpnkit as "VPNKit socket (Hyperkit; multi-client accept loop)", but the source code analysis directly below (line 133) correctly concludes that every non--listen interface — including -listen-vpnkit — accepts exactly one connection per gvproxy lifetime. The actual binary output in the Artifacts section also does not include "multi-client accept loop" in the flag description.

A reader scanning the flag table in isolation will form the wrong mental model: that -listen-vpnkit is multi-client when it is single-client and therefore unsuitable for a one-gvproxy-per-host design. Consider removing the "multi-client accept loop" parenthetical from the flag table and instead annotating it "(single-client: not suitable for DM2)" to match the analysis.

MEDIUM — Security / Correctness

This paragraph states the management HTTP API is "served internally within the virtual network on (redacted) (accessible from within PTasks via the gateway's virtual IP)". However, 37 lines later (line 460) the document says the /leases, /cam, and /stats` endpoints are "accessible only from the host side (unix socket), not from within PTasks via the gateway."

These two claims contradict each other, and the contradiction is security-relevant: if POST /services/forwarder/expose or GET /leases is reachable from inside a PTask via (100.64.0.1/redacted) a rogue OwnIp` PTask could:

• expose arbitrary host ports to external traffic (via /expose)
• read the IP↔MAC mapping of every other PTask on the switch (via /leases)
• read switch topology (via /cam)

Based on the gvproxy source cited (pkg/virtualnetwork/mux.go and services.go): Mux() registers /connect on the unix socket; ServicesMux() registers the management routes. The gateway's in-network HTTP on port 80 is separate from the unix-socket listener. The key question is whether the gateway's internal HTTP server (served by gvisor's netstack LinkEndpoint) routes to ServicesMux(). If it does not — if it only serves DNS/DHCP — then the unix-socket-only statement (line 460) is correct and the "accessible from within PTasks" claim here is wrong and should be removed.

This needs a concrete one-line clarification: either confirm which routes the gateway port-80 service actually serves, or remove the parenthetical "accessible from within PTasks via the gateway's virtual IP" and keep only the unix-socket statement.

LOW — Correctness

The UC6 ARP walkthrough has a false-start sentence that describes the wrong direction of frame delivery, then self-corrects with **Wait**. The initial sentence (lines 382–383) says "Relay-B receives the ARP reply from PTask B and writes it to TAP-B" — but relay-B writes to TAP-B to deliver frames inbound to PTask B, not to deliver PTask B's own replies. The ARP flow at this step is: relay-A delivers the ARP request to TAP-B (inbound to PTask B), PTask B then replies, and relay-B picks up that reply from TAP-B and forwards it to the switch — the opposite direction.

The corrected bullet list that follows is correct. But the presence of the false start before the **Wait** means a reader who stops at the first sentence of step 3 comes away with an inverted model of relay-B's role.

Suggestion: remove lines 382–385 (the false start and the **Wait** marker) and keep only the corrected bullet-point walkthrough, since it already covers the same content accurately.

MEDIUM — Correctness

The outer loop in switch_to_tap (line 301) has no break statement — the only exits are via ? operators on lines 302, 305, and 310, which return Err(...) from the function, not from the loop. In Rust, a loop {} with no break has type ! (the "never" type). Code after a !-typed expression is unreachable.

This Ok(()) is therefore dead code and will trigger:

warning: unreachable code

Under cargo clippy --all-targets -- -D warnings (per CLAUDE.md), this becomes a hard error that will block U1-T2's build when this sketch is used as an implementation starting point.

Fix: remove Ok(()) and let the loop { } itself coerce to anyhow::Result<()> (the same pattern tap_to_switch uses — the outer loop there has a break and ends with Ok(()), but switch_to_tap has no break so Ok(()) is unreachable):

async fn switch_to_tap<R>(
    mut sock: R,
    tap: Arc<AsyncFd<std::fs::File>>,
) -> anyhow::Result<()>
where
    R: AsyncReadExt + Unpin,
{
    let mut size_buf = [0u8; 2];
    loop {
        sock.read_exact(&mut size_buf).await?;
        let n = u16::from_le_bytes(size_buf) as usize;
        let mut buf = vec![0u8; n];
        sock.read_exact(&mut buf).await?;
        loop {
            let mut guard = tap.writable().await?;
            match guard.try_io(|inner| inner.get_ref().write_all(&buf)) {
                Ok(result) => { result?; break; }
                Err(_) => continue,
            }
        }
    }
    // No Ok(()) here — loop has type `!`, coerces to anyhow::Result<()>
}

LOW — Correctness

Action item 4 says "pre-assigned MAC→IP entries" but the dhcpStaticLeases YAML format is keyed by IP (not MAC), as shown in the spike's own YAML example at lines 114–116:

dhcpStaticLeases:
  "100.64.0.2": "52:54:00:00:00:02"   # IP → MAC
  "100.64.0.3": "52:54:00:00:00:03"

This is also confirmed by the upstream source cited at the bottom of the spike: pkg/tap/ip_pool.go — Reserve(ip, mac) — where IP is the primary key.

An implementer reading "MAC→IP entries" may form the wrong mental model about which field is the YAML key when implementing the IP allocation table and YAML generation. Consider replacing "MAC→IP entries" with "IP→MAC entries (IP as key, MAC as value)" to match the actual dhcpStaticLeases schema.

LOW · correctness

The unsafe block is missing a // SAFETY: comment, which the project Rust coding standard requires (CLAUDE.md: "unsafe requires a // SAFETY: comment covering every caller invariant").

Since this sketch is the stated U1-T2 implementation target, it should model the project convention. The invariant to document is:

// SAFETY: tap_fd.into_raw_fd() transfers ownership of a valid, open, caller-owned
// file descriptor. File takes exclusive ownership and will close it on drop.
let tap_file = unsafe { std::fs::File::from_raw_fd(tap_fd.into_raw_fd()) };

LOW · correctness

The read buffer is hardcoded at 1518 bytes (max Ethernet frame at the default MTU of 1500 + 18 bytes of header/VLAN overhead). However, the documented binary flags include -mtu int (default 1500), so minimald could start gvproxy with a larger MTU.

If gvproxy is configured with a non-default MTU and the TAP device inherits that MTU, read() on the TAP fd may return frames larger than 1518 bytes. A buffer too small causes read() to return a truncated frame and discard the remainder (Linux silently drops the excess bytes for character devices).

For the implementation, derive the buffer size from the configured MTU:

// mtu: u16 passed from the gvproxy config (default 1500)
let mut buf = vec![0u8; mtu as usize + 18]; // +14 Ethernet header +4 VLAN tag

If the spike is intentionally scoped to the default MTU of 1500, a brief note here would clarify that assumption.

LOW / correctness

The GET /leases response is documented as {ip: mac} (IP as JSON key, MAC as value), but the parenthetical says "(MAC to IP mapping)" — these describe opposite key→value orderings.

Action Item 4 confirms that dhcpStaticLeases uses IP as key and MAC as value, which is consistent with {ip: mac}. If that is also the /leases JSON shape, the parenthetical should read "(IP to MAC mapping)" instead.

If the actual gvproxy /leases endpoint returns MAC as key and IP as value (the more conventional DHCP lease-table direction), then the format notation should be {mac: ip} and the parenthetical is correct. Either way, one of the two descriptions is inverted and should be reconciled when correcting R1.5 (Action Item 1) to avoid confusion in the U1-T2 implementation.

LOW / spec compliance

Issue #511's proof artifact specification listed five deliverables, the last being "residual risks / anything still needing a live trial." The spike answers all four questions via source reading and binary inspection (--help output), but does not explicitly enumerate what would need live netns/TAP testing before U1-T2 implementation begins.

Action Items 5 and 6 note design gaps (egress ACL, no -subnet flag) as forward work, but these are framed as "things to do next" rather than "findings that are source-confirmed but not live-verified." A reader handing this spike off to U1-T2 implementors has no clear list of what to smoke-test first.

Examples of things that are source-confirmed but not yet live-tested:

• The dhcpStaticLeases YAML format and static IP assignment behavior under an actual gvproxy process
• The HyperKit frame relay timing / first-ARP exchange triggering correct CAM population
• Multi-client behaviour under concurrent PTask attachments
• The -ssh-port -1 suppression of the default forward when combined with -config

Adding a brief "Residual Risks" or "Live Trial Needed" section (even a short bulleted list) before or after the Action Items would complete the proof artifact as specified.

LOW · Correctness

Action item 1 proposes adding the phrase "relay goroutine" to docs/specs/03-spec-networking/03-spec-networking.md. This is Go-specific terminology; minimald is a Rust codebase and the Rust sketch earlier in this document uses Tokio tasks, not goroutines.

When this action item is applied to the spec, downstream implementers reading the spec will see Go vocabulary in a Rust context. Suggested replacement for the proposed text:

"minimald runs an async relay task per OwnIp PTask that bridges the netns TAP
device to gvproxy via HTTP POST to `/connect` on the management unix socket,
using HyperKit framing (2-byte LE length prefix + raw Ethernet frames). No
SCM_RIGHTS or fd-passing is involved."

"Tokio task" or "async task" are the Rust equivalents of "goroutine".

LOW · Correctness

The Host header in the Rust sketch (Host: gvproxy) differs from the value shown in the prose handshake description at line 158 (Host: localhost). Since gvproxy ignores the host header on unix domain sockets, both are functionally equivalent — but the inconsistency between the descriptive example and the code sketch could mislead an implementer. Pick one value and use it consistently in both places.

LOW — Correctness

The ioctl constant SIOCNETIFFLAGS does not exist in Linux. The correct constant for setting interface flags (to bring an interface up) is SIOCSIFFLAGS ("Set Interface FLAGS"). SIOCGIFFLAGS is the corresponding read-only getter.

A U1-T2 implementer using this handshake sketch to write the link-up sequence with nix or raw ioctl calls would find SIOCNETIFFLAGS undefined. Suggest correcting to SIOCSIFFLAGS IFF_UP.

LOW — Correctness

Commit d0b28fa ("unify Host header and replace goroutine with async task") updated the prose and topology diagram to use "async relay task" / "relay task" language, but these two labels in the step-by-step handshake diagram still use Go-specific "goroutine" terminology.

The rest of the document consistently uses "relay A", "relay B", "async relay task" — these two diagram lines are the only remaining occurrences of "goroutine". Suggest:

task rx: read(tap_fd) → LE_u16(len) + frame → write(sock)
task tx: read(sock) → LE_u16(len) → read(frame) → write(tap_fd)

LOW — Correctness

The unsafe block calls std::fs::File::from_raw_fd() and OwnedFd::into_raw_fd(), both of which are trait methods (FromRawFd::from_raw_fd and IntoRawFd::into_raw_fd). Neither trait is in the sketch's use block (lines 227–230), so this block would fail to compile:

error[E0599]: no method named `into_raw_fd` found for struct `OwnedFd`
error[E0599]: no associated function `from_raw_fd` found for struct `File`

The real implementation will need:

use std::os::fd::{FromRawFd, IntoRawFd};

(or the platform-specific std::os::unix::io::{FromRawFd, IntoRawFd} on older toolchains). Worth noting in the sketch so the U1-T2 implementer doesn't hit a confusing compile error.