Repository navigation
test(minvmd): hold a spoofed flow open until its marker is acknowledged - #1864
Merged
norrietaylor merged 2 commits intoOct 3, 2026
Merged
Conversation
spoofed_flow wrote the ACK and the marker's push, returned, and dropped the gate socket: no wait for the switch's acknowledgement and no retransmit. A frame lost in the relay's or the switch's teardown of that connection lost the marker with nothing to say where (#1845). The flow now resends the push every 500 ms until the switch acknowledges every marker byte (SPOOF_ISN + 1 + marker length, compared in wrapping sequence space), answering ARP for its address meanwhile, and reports whether it was acknowledged. An acknowledged marker that never reaches the listener was lost past the switch's stack; an unacknowledged one was lost before it.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Contributor
|
Reviewed: 1 finding, none blocking at Address the comments on the diff. Clobber: none: |
…rker The admit verdict in the attempts record now carries the marker's acknowledgement, so a later red run shows whether the ACK path worked without a pre-fix timing baseline to infer it from.
Member
Author
|
4/4 green on the KVM lane, dispatched with
Before this fix the test passed in one of three merge-group runs that executed it. A pre-fix pass took 27.2 s, in merge-group run 37088544326. The fixed test usually runs faster than that, so the ACK t 8000 hreshold matched. If a flow never matched an ACK, it would sit out its full 10 s deadline, putting a run near 32 s for one flow or 42 s for both. The follow-up commit adds the acknowledgement result to each admitted flow's verdict in the attempts record, which is printed when the test fails. A later red run will then show directly whether the switch acknowledged the marker, without needing a timing baseline. |
Comment on lines
1324
to
+1325
| write_frame(&mut sock, &push).map_err(|e| format!("write push: {e}"))?; | ||
| return Ok(()); | ||
| return Ok(await_marker_ack(&mut sock, flow, &push, deadline)); |
Contributor
There was a problem hiding this comment.
LOW · correctness · confidence 0.60 · in await_marker_ack
await_marker_ack receives the full deadline and recomputes end = Instant::now() + deadline from scratch, discarding the time spoofed_flow's handshake loop already spent against that same budget. A flow that uses most of FLOW_DEADLINE (10 s, documented as "how long one spoofed flow may take to produce its verdict") to reach its SYN-ACK then gets a fresh full 10 s to await the acknowledgement, so an in-plan flow can take up to ~20 s — double the named bound. Pass the remaining time (deadline.saturating_sub(elapsed)) instead of the full duration so the per-flow bound actually bounds the flow.
What the reviewer checked
vm_escape_integration.rs:1325 return Ok(await_marker_ack(&mut sock, flow, &push, deadline)); ... :1194 let end = Instant::now() + deadline;
Hand this to an agent
Act on a code-review finding.
File: crates/minvmd/tests/vm_escape_integration.rs
Lines: 1324-1325
Category: correctness
Severity: low (reviewer confidence 0.60)
Finding:
`await_marker_ack` receives the full `deadline` and recomputes `end = Instant::now() + deadline` from scratch, discarding the time `spoofed_flow`'s handshake loop already spent against that same budget. A flow that uses most of `FLOW_DEADLINE` (10 s, documented as "how long one spoofed flow may take to produce its verdict") to reach its SYN-ACK then gets a fresh full 10 s to await the acknowledgement, so an in-plan flow can take up to ~20 s — double the named bound. Pass the remaining time (`deadline.saturating_sub(elapsed)`) instead of the full duration so the per-flow bound actually bounds the flow.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
Member
Author
|
The head commit c2a3f97 is green on the KVM lane in run 37110272013. |
norrietaylor
merged commit Oct 3, 2026
72333a6
into
fix/ci-gvproxy-before-vm-harness
7 checks passed
yermakoffivan
pushed a commit
to yermakoffivan/minimal
that referenced
this pull request
Oct 3, 2026
…ominimal#1815) * ci: fetch the gvproxy switch before the VM harness step and export it Both VM lanes fetched the pinned gvproxy switch only after the "VM integration harnesses" step and never exported MINVMD_GVPROXY_BIN to it, so every harness that needs the host switch skipped and counted as a pass. Fetch it once ahead of the harness step, drop the later duplicate, and give the harness step MINVMD_GVPROXY_BIN plus the MINVMD_VM_LANE marker. * revert(ci): leave the frozen workflows untouched * test(minvmd): fetch the pinned gvproxy switch for VM harnesses under MINVMD_E2E * test(minvmd): give the vm_escape boxes a package source and the switch The vm_escape harness never ran a box: a shell exec launches the box's sandbox, which resolves the baseline packages against the project's package graph, and the harness uploaded a minimal.toml with no [upstream], so every exec failed with "no such package: base". It also booted with `minvmd boot --foreground`, which never stands up the gvproxy switch, so the guest daemon could not have fetched packages anyway. - Carry the pinned [upstream] (gominimal/pkgs at f4de33d0) in the boxes' minimal.toml, shared as common::PKGS_UPSTREAM. The guest daemon fetches it as node-plane traffic; under MINVMD_E2E=1 a failed fetch fails the test. - Boot through the supervisor path (`minvmd run --detach`, poll `status --json` until Running, `minvmd stop` on drop), with MINVMD_VM_OWN_IP and the helper's MINVMD_GVPROXY_BIN, and read the gate's lines from the detached supervisor's JSON log. - Gate a Pending ConfigureLoadout the way the CLI's no-prompt hook answers vars, failing on any patch or hook, and submit the verdict. - Return each exec's stderr and quote it when an exec exits nonzero, so a box that cannot launch names its cause. * fix(minvmd): reap the VMM child when a boot harness tears down boot_integration and minimald_session_integration SIGKILLed `minvmd boot --foreground` alone. Its `__krun-vmm` child was reparented to init and kept the VM running, holding the harness's inherited stdio open, which nextest reported as LEAK and which piled up on persistent runners. Spawn the boot as its own process group with stdin off the terminal, and tear down with common::kill_group, which kills the whole group before reaping the parent. * fix(minvmd): kill the boot harness's process group with killpg procps `kill -KILL -<pgid>` exits 0 on Linux without signalling the group, so the reap that followed blocked on a still-running `minvmd boot` and the KVM lane timed out in boot_integration. Signal the group with killpg(2) and kill the parent directly as well, so the reap cannot block on a group signal that missed. * fix(minvmd): harden vm_escape's minvmd helper and teardown - Export MINVMD_READY_TIMEOUT_SECS (150 s) to the supervisor: `--timeout` bounds only `run --detach`'s poll, and the READY wait otherwise defaults to 60 s, short of a cold boot on a lane that does not set it. - Drain the subcommand's stdout and stderr on reader threads while it runs, so a child that writes more than a pipe buffer cannot stall into the timeout. The Output shape and the timeout are unchanged. - Guest::drop never panics: `minvmd stop` runs through a non-panicking bounded helper, and when it errors or times out the VMM pid recorded from `status --json` is killed and the failure logged. * test(minvmd): hold a marker's connection open past its handshake (gominimal#1846) The host listener armed a 2s read timeout the moment it accepted a connection, and discarded the connection unrecorded when that timeout fired. gvisor-tap-vsock's TCP forwarder dials the backend *before* it creates the endpoint that answers the SYN, so the listener accepts, and starts that clock, before the sender has seen the SYN-ACK it must answer before it can push. A handshake slower than ~2s — one SYN-ACK retransmit on a loaded runner is enough — left the marker nowhere to land: the flow completed, and the wait meant to grant it 10s spent them against a socket already closed. Hold each connection for a budget longer than the longest wait, record every read as it arrives rather than once at the end, and read each connection on its own thread so one silent sender cannot delay another sender's marker. Report what the listener saw when a wait fails, so a next occurrence says whether the connection was never dialed, was dialed and stayed silent, or arrived after the wait gave up. Refs: gominimal#1845 * test(minvmd): print the gate's own account when a marker is lost (gominimal#1849) The listener's report now shows the spoofed flow's connection being accepted and then reading nothing, so the loss is after the handshake and not in the listener's timing budget. What it cannot show is which side lost the payload. Print the gate's lines for the flow alongside it. An egress-unregistered-source line and nothing else means the gate admitted the frames and the loss is past it, in gvproxy; an ingress-leg line means the relay came off on the switch side and whatever the guest had buffered went with it. Refs: gominimal#1845, #1847 * test(minvmd): hold a spoofed flow open until its marker is acknowledged (gominimal#1864) * test(minvmd): hold a spoofed flow open until its marker is acknowledged spoofed_flow wrote the ACK and the marker's push, returned, and dropped the gate socket: no wait for the switch's acknowledgement and no retransmit. A frame lost in the relay's or the switch's teardown of that connection lost the marker with nothing to say where (gominimal#1845). The flow now resends the push every 500 ms until the switch acknowledges every marker byte (SPOOF_ISN + 1 + marker length, compared in wrapping sequence space), answering ARP for its address meanwhile, and reports whether it was acknowledged. An acknowledged marker that never reaches the listener was lost past the switch's stack; an unacknowledged one was lost before it. * test(minvmd): record whether the switch acknowledged each admitted marker The admit verdict in the attempts record now carries the marker's acknowledgement, so a later red run shows whether the ACK path worked without a pre-fix timing baseline to infer it from.
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
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.
Summary
Stacked on #1815, in the same way as #1849. It addresses #1845.
spoofed_flowused to write the ACK and the marker's data packet, return, and drop the gate socket. It never waited for the switch to acknowledge the data and never retransmitted. Any frame lost in the gate relay's teardown of that connection, or in the switch's, lost the marker with nothing to show where. The gate relay's teardown race is #1847.The #1849 run could not tell those losses apart:
egress-unregistered-sourceline for 100.64.0.99 covers the SYN and says nothing about the frames after it.relay_framestears down silently on theOkandUnexpectedEofpaths, so the absence of an error line rules nothing out.Change
SPOOF_ISN + 1 + marker lengthin the flow's own sequence space, compared with wraparound.SPOOF_ISNis one named constant that feeds the SYN, the data packet and the expected acknowledgement, so they cannot drift apart.Incominggains anAckvariant, andclassifyemits it for a bare ACK on the flow's connection.Ok, carrying the reason, so it never lands in the refusal arm. The marker assertion's message now says whether the switch acknowledged the marker.What the next run tells us
Verification
ci-linux-kvm.ymlon this branch separately and will report the result here.cfg(minvmd_libkrun), which this host does not set, so normal lints skip it. I ranRUSTFLAGS="--cfg minvmd_libkrun" cargo clippy -p minvmd --test vm_escape_integration -- -D warnings, and it is clean. I confirmed that check really compiles the file by planting a type error, which it caught.just fmt-checkis clean.