fix(minvmd): replace raw-PID gvproxy signalling with pidfd - #555
Conversation
Use pidfd_open(2) to obtain a process-identity file descriptor for the gvproxy child at spawn time, and route all teardown signals through pidfd_send_signal(2) via libc::syscall(SYS_pidfd_send_signal, …). A pidfd is bound to the exact process instance, not the numeric PID. After the process exits, pidfd_send_signal returns ESRCH rather than silently delivering a signal to an unrelated process that recycled the same PID — structurally closing the crash-then-recycle window documented in the PtaskAttachment::Drop comment. Changes: - GvproxySwitch::supervise opens a pidfd immediately after extracting the child PID, before the supervision task is spawned (Arc<OwnedFd> shared with PtaskAttachment so all three teardown paths use it). - signal_via_pidfd replaces signal_child on Linux for all three teardown paths: stop(), GvproxySwitch::Drop, PtaskAttachment::Drop. - signal_child is retained under #[cfg(not(target_os = "linux"))] for macOS development builds. - New test pidfd_signal_to_reaped_child_returns_esrch: spawns a short-lived child, opens its pidfd, reaps the child, then asserts pidfd_send_signal returns ESRCH rather than hitting a recycled PID. Refs: #550
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Linux pidfd-based signalling to ChangesPidfd-based recycle-safe signalling for gvproxy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minvmd/src/net.rs`:
- Around line 405-426: When the `pidfd_open` syscall fails (in the `if raw < 0`
check), the function returns an error before the supervision task is spawned,
causing the `child` process to be dropped without being killed due to the
default `kill_on_drop = false` behavior. Before returning the error on the
failed `pidfd_open` call, call the appropriate kill method on the `child`
process to ensure it is terminated and prevent the process leak. This ensures
the child does not remain orphaned when `pidfd_open` fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5cedf65-c347-41c8-8e24-cb7ec04fc9f2
📒 Files selected for processing (1)
crates/minvmd/src/net.rs
|
Revise claim for head ef5bb03. |
|
Auto-revise 1 of 3. |
This comment has been minimized.
This comment has been minimized.
When pidfd_open(2) fails, the owned Child was returned without being killed. Because tokio::process::Child defaults to kill_on_drop = false, the gvproxy process would be orphaned. Call start_kill() before propagating the error so the child is always terminated on this path.
|
Commit pushed:
|
sdd-validate findingsBoundary: Implementation Gate 1 — Proof artifacts re-executed and passingArtifact 1 — File:
Artifact 2 — Test:
Gate 2 — Changed files within task scopeExecution plan (
Gate 3 — No real credentials in the diffThe diff contains Rust code using Linux pidfd syscalls and
VerdictImplementation boundary passes clean. No Blockers, no Warnings. This is a fast-path feature: lifecycle advance (
|
…sh proof Restore main's crates/minvmd/src/net.rs: this branch had reverted #555's pidfd-based gvproxy signalling (a stale-base content-revert unrelated to the WireGuard mesh task), re-introducing the PID-recycle bug. WireGuard lives in minimald; minvmd net.rs is out of scope here. Wire the UC7 two-namespace mesh proof into ci-netns.yml: it was covered by no gate (mesh_uc7.rs is networking-wg-gated, the netns name filter did not match remote_ptask_packet_crosses_the_mesh_tunnel, and the test read MINIMAL_NETNS_TESTS while the lane sets MINIMALD_NETNS_TEST). Add a dedicated --features networking-wg --test mesh_uc7 step and fix the env var name in the test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
…sh proof Restore main's crates/minvmd/src/net.rs: this branch had reverted #555's pidfd-based gvproxy signalling (a stale-base content-revert unrelated to the WireGuard mesh task), re-introducing the PID-recycle bug. WireGuard lives in minimald; minvmd net.rs is out of scope here. Wire the UC7 two-namespace mesh proof into ci-netns.yml: it was covered by no gate (mesh_uc7.rs is networking-wg-gated, the netns name filter did not match remote_ptask_packet_crosses_the_mesh_tunnel, and the test read MINIMAL_NETNS_TESTS while the lane sets MINIMALD_NETNS_TEST). Add a dedicated --features networking-wg --test mesh_uc7 step and fix the env var name in the test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
… advertisement, and minimal mesh CLI (#561) * feat(minimald,minimal2): boringtun WireGuard mesh peer and mesh CLI * ci: gate the networking-wg WireGuard mesh proofs The WireGuard mesh peer is behind the non-default networking-wg feature, so the workspace test job never compiled or ran its proof artifacts (two_meshes_handshake_and_relay_a_packet, rpc get_mesh_status). Add an explicit step so the mesh proofs run in CI (sdd-validate Gate 1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS * fix(minimald): restore pidfd net.rs revert + wire up the UC7 netns mesh proof Restore main's crates/minvmd/src/net.rs: this branch had reverted #555's pidfd-based gvproxy signalling (a stale-base content-revert unrelated to the WireGuard mesh task), re-introducing the PID-recycle bug. WireGuard lives in minimald; minvmd net.rs is out of scope here. Wire the UC7 two-namespace mesh proof into ci-netns.yml: it was covered by no gate (mesh_uc7.rs is networking-wg-gated, the netns name filter did not match remote_ptask_packet_crosses_the_mesh_tunnel, and the test read MINIMAL_NETNS_TESTS while the lane sets MINIMALD_NETNS_TEST). Add a dedicated --features networking-wg --test mesh_uc7 step and fix the env var name in the test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS * ci(netns): run the UC7 mesh proof binary as root mesh_uc7 binds sockets inside the namespaces via in-process setns, so the test binary itself needs root (the sudo-per-command model the UC6 netns tests use does not cover an in-process setns). Build unprivileged, then run the test binary under the netns runner's passwordless sudo, fixing the 'mkdir /run/netns: Permission denied' / 'ip netns add' failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS * fix(minimald,minimal2): address mesh review threads - mesh join: validate host:port at entry before persisting the enrolment so a typo never lands a bad address on disk (CR/bot R4.3 input check). - wg route_inbound: probe every candidate peer (exact-endpoint match first, then each endpoint-less peer) until one authenticates, instead of stopping at the first endpoint-less peer — handles roaming source addresses and multiple endpoint-less peers. Still exactly one decapsulate for the owning peer. - wg mesh_status: treat a finished pump as unconfigured (MeshHandle:: is_alive) so GetMeshStatus never serves frozen, stale peer state after the pump exits on a socket error. - wg loopback test: pre-bind ephemeral sockets via start_with_socket instead of hard-coding 51820/51821, removing CI port-collision flakes. - mesh_uc7: RAII Drop guard tears down namespaces on every exit path (success, panic, timeout); fix stale env-var name in the doc comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(minvmd): always SIGKILL gvproxy on stop() timeout When a PtaskAttachment drop already claimed teardown (SIGTERM sent) and gvproxy ignores SIGTERM, stop() would hit its grace timeout but skip SIGKILL under the `!already_claimed` guard, then block forever on `supervisor.await` and hang daemon shutdown. Escalate to SIGKILL on timeout unconditionally; on Linux the fd-based signal targets the exact process instance (ESRCH after exit is benign), so it never lands on a recycled PID. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(minimal2): reject port 0 in mesh-join enrolment address port.parse::<u16>() accepts 0, but a WireGuard endpoint on port zero is unusable; was still written as a successful enrolment. Require non-zero. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS --------- Co-authored-by: gominimal-aw-bot[bot] <281738952+gominimal-aw-bot[bot]@users.noreply.github.com> Co-authored-by: Norrie Taylor <norrie@minimal.dev> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
[sdd-fastpath: tracking=550 tier=sonnet]
Fixes the independent-crash recycled-PID window in
GvproxySwitch(#550).What changed
Replaces raw-PID
kill(2)on all three teardown paths withpidfd_send_signal(2)vialibc::syscall(SYS_pidfd_send_signal, ...):GvproxySwitch::stop()GvproxySwitch::DropPtaskAttachment::Droppidfd_open(2)is called immediately after the child PID is extracted insupervise(), before the supervision task is spawned. TheArc<OwnedFd>is shared with everyPtaskAttachmentso all teardown paths use the same pidfd. A pidfd is bound to the exact process instance —pidfd_send_signalreturnsESRCHafter the process exits, never resolving to a recycled PID.signal_child(rawkill(2)) is retained under#[cfg(not(target_os = "linux"))]for macOS development builds.Proof artifacts
Test —
cargo test -p minvmd net::tests::pidfd_signal_to_reaped_child_returns_esrch:File —
grep -q 'pidfd' crates/minvmd/src/net.rs:Returns exit code 0 (
pidfdis present innet.rs).All 19 net tests (including existing teardown tests):
Merging this pull request advances the tracking issue from
sdd:in-progresstosdd:done; a human does the final close.Summary by CodeRabbit
Release Notes
Bug Fixes
Tests