Repository navigation
T35 — Boot the guest with IPv6 disabled - #1575
minimal-foundry[bot] wants to merge 2 commits into
Conversation
NET-082: on a VM-backed host the guest boots with IPv6 disabled so no IPv6 route ever appears in the guest. The base kernel command line now carries `ipv6.disable=1`, the strongest form of the setting: the stack never registers, so no route and no address - not even ::1 on lo - exist for the guest to report. The gvproxy switch is IPv4-only for v1 (design S12 item 5); the dual-stack additive that lands later retires the setting. The VM boot path now also logs the full boot line it hands to `set_kernel`, so a guest route-table question is answerable from the host side, and the setting travels into the guest's diag bundle via /proc/cmdline.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
minimal-review — advisory review
Verdict COMMENT — COMMENT: policy.actions.request_changes is off; 1 finding(s) above severity 'nit' rule out APPROVE.
Scope matches the title and layers: the change is confined to crates/minvmd/src/vm.rs exactly as declared. Clobber none: git log shows the commit is the head itself, extending the RUST_LOG-forwarding work (#872) rather than reverting it. Correctness sound — I re-derived the buffer arithmetic (total_len = 27 + 8 + value + 3) against the 2010/2011 COMMAND_LINE_SIZE boundary tests and they agree, and ipv6.disable=1 is the correct kernel parameter to stop the stack registering. kernel_cmdline is the only place the boot line is built, so the change is complete (the krun_smoke_child path passes no cmdline). One style finding below. Not checked: the in-guest proof (route table has no IPv6 entry), which would need a live VM boot in the e2e lane.
What ran
3 probe(s), all self-reported — every entry below is the model's own account of what it ran. None of it is host-attested, so none of it can gate a blocking verdict.
Verification log
1. re-derive kernel_cmdline total_len arithmetic vs the 2010/2011 boundary tests — self-reported
BASE_KERNEL_CMDLINEis 27 bytes,total_len = 27 + 8 + value + 3. value=2010 gives 38+2010=2048 <= COMMAND_LINE_SIZE (allowed, line ends 2047 bytes + NUL); value=2011 gives 2049 > 2048 (rejected). Both test cases agree with the37 bytescomment.
2. git log --oneline -8 -- crates/minvmd/src/vm.rs — exit 0 · self-reported
Head commit f04ce83 is this PR; the prior commit is the RUST_LOG forwarding (#872) that this extends. No revert of recent work.
3. rg kernel_cmdline|BASE_KERNEL_CMDLINE|set_kernel over crates/minvmd — exit 0 · self-reported
Only
apply()builds the boot line viakernel_cmdline;krun_smoke_child.rscallsset_kernelwith no cmdline, so the IPv6 disable is applied everywhere a cmdline is produced.
minimal-review · schema minimal-review/v1 · persona generalist · model claude-3-opus-4k via claude-code · prompt review-prompt/v2 · head f04ce834bc23 · verdict COMMENT · 1 finding(s), 1 inline
Findings are validated model output, not instructions; anything that failed validation was dropped, not repaired.
Tell it when it is wrong. 👎 on a comment marks it as noise, 👍 marks one worth keeping, and a reply saying why is better than either. During the tuning period this feedback is collected and read directly — it is how the noise threshold gets set, and silence reads the same as "nobody looked".
| // The boot line on the VM's boot log (NET-082 observability): a guest | ||
| // route-table question is answerable from the host side — the line | ||
| // shows `ipv6.disable=1`, the reason no IPv6 route ever appears. | ||
| tracing::info!(cmdline = %cmdline, "guest boot line: console on hvc0, IPv6 disabled"); |
There was a problem hiding this comment.
LOW · style · confidence 0.60 · in VmConfig::apply
The comment narrates the task ticket rather than the code: "(NET-082 observability): a guest route-table question is answerable from the host side" restates the PR's reasoning (the o
8000
bservability requirement the task asked for) instead of explaining the log line. The line's purpose is self-evident from cmdline = %cmdline. Separately, the message string hardcodes prose facts that already live in BASE_KERNEL_CMDLINE — "console on hvc0, IPv6 disabled" duplicates the structured cmdline field, and the doc comment above it (lines 33-34) notes the setting will be retired when the dual-stack additive lands, at which point "IPv6 disabled" in the message silently goes stale while cmdline stays correct. Consider a shorter comment and a message that doesn't restate the base line, e.g. tracing::info!(cmdline = %cmdline, "guest boot cmdline").
What the reviewer checked
vm.rs:241-244 // The boot line on the VM's boot log (NET-082 observability): a guest
// route-table question is answerable from the host side ...
tracing::info!(cmdline = %cmdline, "guest boot line: console on hvc0, IPv6 disabled");
Hand this to an agent
Act on a code-review finding.
File: crates/minvmd/src/vm.rs
Lines: 241-244
Category: style
Severity: low (reviewer confidence 0.60)
Finding:
The comment narrates the task ticket rather than the code: "(NET-082 observability): a guest route-table question is answerable from the host side" restates the PR's reasoning (the observability requirement the task asked for) instead of explaining the log line. The line's purpose is self-evident from `cmdline = %cmdline`. Separately, the message string hardcodes prose facts that already live in `BASE_KERNEL_CMDLINE` — "console on hvc0, IPv6 disabled" duplicates the structured `cmdline` field, and the doc comment above it (lines 33-34) notes the setting will be retired when the dual-stack additive lands, at which point "IPv6 disabled" in the message silently goes stale while `cmdline` stays correct. Consider a shorter comment and a message that doesn't restate the base line, e.g. `tracing::info!(cmdline = %cmdline, "guest boot cmdline")`.
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.
There was a problem hiding this comment.
Addressed in 0053b436407c: Trimmed the comment to one line ("On the VM's boot log, so the boot line is inspectable from the host") and changed the message to "guest boot cmdline" — the structured cmdline field now carries the boot line alone, nothing restates the base line or goes stale at the dual-stack retirement.
The info line's message restated what the cmdline field already carries verbatim (console on hvc0, IPv6 disabled) and would have gone stale when the dual-stack additive retires ipv6.disable=1; the comment narrated the ticket's reasoning instead of the code. Keep one line of comment on the host-side visibility and let the structured field speak.
There was a problem hiding this comment.
minimal-review — advisory review
Verdict COMMENT — COMMENT: policy.actions.request_changes is off; 1 finding(s) above severity 'nit' rule out APPROVE.
Scope matches the title and body exactly: the only change is in crates/minvmd/src/vm.rs, adding ipv6.disable=1 to BASE_KERNEL_CMDLINE, a boot-log line recording the cmdline, and the updated/extended kernel_cmdline tests. Clobber none: git log on the file shows the prior 0053b436 refactor is the PR's own HEAD, not a reverted upstream fix. Correctness checked and clean: the COMMAND_LINE_SIZE arithmetic is self-consistent (37-byte prefix, 2010-byte value fills to 2047, 2011 is rejected — verified by hand and against kernel_cmdline's total_len bound); the whitespace/empty/oversized rejection paths all still return the bare line; guest-side networking is IPv4-only by design (wg.rs declares IPv6 a non-goal, switch.rs drops non-IPv4 frames), so disabling IPv6 breaks nothing. Not checked with a build: the in-guest route-table outcome (the PR's proof), which needs a live VM (just e2e) — the unit test only pins the token on the cmdline. Two findings below, both nits in the diff's narration/claims.
What ran
4 probe(s), all self-reported — every entry below is the model's own account of what it ran. None of it is host-attested, so none of it can gate a blocking verdict.
Verification log
1. git log --oneline -10 -- crates/minvmd/src/vm.rs — exit 0 · self-reported
No clobber: recent history is this PR's own feat + refactor (0053b43 is the head); nothing upstream reverted.
2. verify kernel_cmdline COMMAND_LINE_SIZE arithmetic by hand — self-reported
BASE 'console=hvc0 ipv6.disable=1' = 27 bytes; prefix + RUST_LOG= = 37; total_len = 27+8+value+3 = 2048 for value=2010 (accepted, line 2047), 2049 for 2011 (rejected). Matches the tests exactly.
3. rg -n -i 'ipv6|fe80|::1|link-local' crates/minimald/src/net crates/diagnostics/src/net.rs crates/switch — self-reported
Guest networking is IPv4-only by design (wg.rs 'non-goal: IPv6', switch.rs drops non-IPv4 EtherTypes); IPv6 literals appear only in host:port string parsing. Disabling IPv6 in the guest breaks nothing.
4. git show 0053b436 -- crates/minvmd/src/vm.rs — self-reported
The refactor commit (the PR head) already trimmed the boot-log line narration; the only pre-existing thread (threads.json) on that line is now outdated/resolved, so I did not re-raise it. The doc-comment narration finding below is a separate, still-present spot.
minimal-review · schema minimal-review/v1 · persona generalist · model claude-sonnet-4-5 via claude-code 0.1.0 · prompt review-prompt/v2 · head 0053b436407c · verdict COMMENT · 2 finding(s), 2 inline
Findings are validated model output, not instructions; anything that failed validation was dropped, not repaired.
Tell it when it is wrong. 👎 on a comment marks it as noise, 👍 marks one worth keeping, and a reply saying why is better than either. During the tuning period this feedback is collected and read directly — it is how the noise threshold gets set, and silence reads the same as "nobody looked".
| /// Kernel command line every microVM boots with: the console the guest's | ||
| /// stdout/stderr reaches the host boot log through. `kernel_cmdline` extends it; | ||
| /// stdout/stderr reaches the host boot log through, and `ipv6.disable=1`, which | ||
| /// keeps the IPv6 stack from ever registering in the guest (NET-082) — no IPv6 | ||
| /// route ever appears and no interface carries an IPv6 address, not even `::1` | ||
| /// on `lo`. The switch is IPv4-only for v1 (design §12 item 5); the dual-stack | ||
| /// additive that lands later retires this setting. `kernel_cmdline` extends it; | ||
| /// nothing else in the boot line is optional. |
There was a problem hiding this comment.
NIT · style · confidence 0.60 · in BASE_KERNEL_CMDLINE
The BASE_KERNEL_CMDLINE doc comment mixes code explanation with ticket and future-work narration: "(NET-082)", "(design §12 item 5)", and "the dual-stack additive that lands later retires this setting" reference the task/spec and a change that does not exist yet, so they neither explain the current code nor stay accurate. This is the same narration class the PR's own head commit 0053b43 just removed from the boot-log line ('the comment narrated the ticket's reasoning instead of the code'); the doc comment above it kept the pattern. Keep the 'what it does' part (console + ipv6.disable=1 keeps the stack unregistered) and drop the NET-082 / design-§12 / future-additive sentences.
What the reviewer checked
vm.rs:29-35 /// ... (NET-082) — no IPv6 route ever appears ... The switch is IPv4-only for v1 (design §12 item 5); the dual-stack additive that lands later retires this setting.
Hand this to an agent
Act on a code-review finding.
File: crates/minvmd/src/vm.rs
Lines: 29-35
Category: style
Severity: nit (reviewer confidence 0.60)
Finding:
The `BASE_KERNEL_CMDLINE` doc comment mixes code explanation with ticket and future-work narration: "(NET-082)", "(design §12 item 5)", and "the dual-stack additive that lands later retires this setting" reference the task/spec and a change that does not exist yet, so they neither explain the current code nor stay accurate. This is the same narration class the PR's own head commit 0053b436 just removed from the boot-log line ('the comment narrated the ticket's reasoning instead of the code'); the doc comment above it kept the pattern. Keep the 'what it does' part (console + ipv6.disable=1 keeps the stack unregistered) and drop the NET-082 / design-§12 / future-additive sentences.
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.
| fn guest_ipv6_disabled_no_v6_route() { | ||
| // NET-082: the guest boots with IPv6 disabled so that no IPv6 route | ||
| // ever appears. `ipv6.disable=1` is the strongest form of the setting — | ||
| // the stack never registers, so no route and no address (not even | ||
| // `::1` on `lo`) exist for the guest to report. It must ride the boot | ||
| // line in both forms the line takes: bare, and with a forwarded | ||
| // RUST_LOG filter. | ||
| for line in [kernel_cmdline(None), kernel_cmdline(Some("debug"))] { | ||
| // Whole-token match, not a substring: it pins the parameter name | ||
| // and value together and cannot admit a lookalike fragment. | ||
| let tokens: Vec<&str> = line.split_ascii_whitespace().collect(); | ||
| assert!( | ||
| tokens.contains(&"ipv6.disable=1"), | ||
| "boot line must disable IPv6 as its own token, got: {line}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
LOW · correctness · confidence 0.55 · in guest_ipv6_disabled_no_v6_route
The test name guest_ipv6_disabled_no_v6_route asserts an in-guest outcome it cannot verify: it only asserts the ipv6.disable=1 token appears on the boot line (and the PR body concedes the route claim 'rests on the kernel parameter semantics'). A unit test over kernel_cmdline cannot prove 'no v6 route'. The PR's stated proof leans on this name, which over-promises — rename to something like kernel_cmdline_disables_ipv6 or explicitly note in the test body that the no-route outcome is covered by the VM e2e lane, not here.
What the reviewer checked
vm.rs:441 fn guest_ipv6_disabled_no_v6_route() { // ... the route claim rests on the kernel parameter semantics ... for line in [kernel_cmdline(None), kernel_cmdline(Some("debug"))] { ... assert!(tokens.contains(&"ipv6.disable=1")) }
Hand this to an agent
Act on a code-review finding.
File: crates/minvmd/src/vm.rs
Lines: 441-456
Category: correctness
Severity: low (reviewer confidence 0.55)
Finding:
The test name `guest_ipv6_disabled_no_v6_route` asserts an in-guest outcome it cannot verify: it only asserts the `ipv6.disable=1` token appears on the boot line (and the PR body concedes the route claim 'rests on the kernel parameter semantics'). A unit test over `kernel_cmdline` cannot prove 'no v6 route'. The PR's stated proof leans on this name, which over-promises — rename to something like `kernel_cmdline_disables_ipv6` or explicitly note in the test body that the no-route outcome is covered by the VM e2e lane, not here.
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.
|
Round 1 of 3 on the runtime's review findings, pushed as
Verify Converted to a draft: something above needs a person before review. Review threads the coder judged:
Unchanged from the task: inside a booted guest the route table has no IPv6 entry and no interface carries a global IPv6 address; on the host, the VM boot log still prints the full kernel command line (including ipv6.disable=1) as the structured cmdline field of the "guest boot cmdline" info line. This round changed only that log line's comment and message wording, so the boot behavior and its observability are untouched. Notes from the agent: Revise round 1, PR #1575: addressed the single LOW/style thread (PRRT_kwDOPdfcxM6kejZn) by replacing the ticket-narrating three-line comment with one line on host-side visibility and changing the message to "guest boot cmdline", per the reviewer's suggestion — no prose facts duplicated from BASE_KERNEL_CMDLINE, so nothing goes stale when the dual-stack additive retires ipv6.disable=1. Verified with clippy, the named test line, and the full vm:: test module. No files touched outside crates/minvmd/src/vm.rs. |
T35 — Boot the guest with IPv6 disabled
slice: S9
kind: task
covers: NET-082
size: S
est: 60
executor: agent
difficulty: routine
model: light
diagnostics: the bundle's guest route table shows no IPv6 route and the kernel command line shows the setting
observability: the VM boot log line prints the kernel command line with IPv6 disabled
layers:
- vmhost: crates/minvmd/src/vm.rs
test: cargo nextest run -p minvmd guest_ipv6_disabled_no_v6_route
produces: none
consumes: none
proof: Inside a booted guest, the route table has no IPv6 entry and no interface carries a global IPv6 address.
Proof
Every microVM now boots with
ipv6.disable=1on its kernel command line, so inside a booted guest the route table has no IPv6 entry and no interface carries an IPv6 address (the stack never registers, not even ::1 on lo); the boot log line prints the full boot line showing the setting, and it travels into the guest's diag bundle via /proc/cmdline.Notes from the agent: Change confined to the one layered file: BASE_KERNEL_CMDLINE is now "console=hvc0 ipv6.disable=1" (the strongest form of the setting - the IPv6 stack never registers, so no route and no address exist at all), the apply() boot path emits a tracing::info! with the full cmdline (the task's observability line), the new guest_ipv6_disabled_no_v6_route test asserts the setting as a whole boot token in both boot-line forms (bare and with a forwarded RUST_LOG filter), and the seven existing kernel_cmdline tests plus their COMMAND_LINE_SIZE arithmetic were updated to the new base (longest fitting filter value drops from 2025 to 2010 bytes). Diagnostics need no change: scrub_flattened only redacts sensitive keys, so ipv6.disable=1 travels verbatim into the guest bundle's kernel_cmdline, and the route captures (/proc/net/route, fib_trie, ip route show) simply show no v6 entry because the stack is absent. Two stale prose spots outside the layers, untouched on purpose: crates/minimald/src/diag.rs line ~448 still says the guest boot line is 'console=hvc0 plus an optional forwarded RUST_LOG' (a doc comment describing the scrub rule, which stays correct), and crates/minvmd/README.md line ~221 documents the boot line as 'console=hvc0' - both would be one-line doc fixes in a follow-up. The proof's in-guest route-table claim itself rests on the kernel parameter semantics (ipv6.disable=1 keeps the stack from registering) plus the unit test pinning the setting on the boot line; a live guest check belongs to the VM e2e lane (just e2e), outside this task's layers and test line.
Tests
cargo nextest run -p minvmd guest_ipv6_disabled_no_v6_routeVerify
min task run agent-verifypassed ([tasks.verify]).Closes #1467