feat(virtiofs): use setcap'd wrapper for inode-file-handles=mandatory#22
Merged
feat(virtiofs): use setcap'd wrapper for inode-file-handles=mandatory#22
Conversation
virtiofsd --cache=auto pins one O_PATH FD per cached inode (#18). With --inode-file-handles=mandatory the cache holds opaque file handles instead, decoupling cache size from FD count. PR #20 (raised ceiling) and ADR-015 (no virtiofs for churning caches) couldn't address this for source-tree shares. The flag requires CAP_DAC_READ_SEARCH. Run-as-root alternatives regress ADR-001 (sandbox=namespace silently breaks O_CREAT|O_EXCL) or ADR-002 (sandbox=none/chroot makes guest-created files root-owned on host). Capability-only keeps both ADRs intact. ensure_virtiofsd_cap() keeps a setcap'd copy of virtiofsd under $XDG_DATA_HOME/nixbox and reinstalls on version drift; do_create and do_mount switch to it with --inode-file-handles=mandatory; cmd_doctor reports wrapper status. One sudo prompt on first nixbox up and after each nixbox update -- parallel to the existing sudo prlimit path. Rationale and rejected alternatives in ADR-016. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The function returns the wrapper path via stdout (echo "$wrapper"), but log() also writes to stdout. Captured via $(ensure_virtiofsd_cap), the install-message and the wrapper path got concatenated, so the caller invoked a bogus binary path — virtiofsd never started in CI on the first 'nixbox up' (no preinstalled wrapper).
ADR-001 documents the non-root failure mode only — it doesn't prove that sandbox=namespace breaks under root. The real reason to reject option 2 is the cost (running the daemon as root) vs. the absence of any benefit over option 4.
Three gaps surfaced by review and empirical verification: - mandatory over prefer: silent fallback would mask #18 until OOM. - Memory now scales with cached inodes (~8 KB/store, ~60 KB/workspace). - /proc/<pid>/fd is root-owned due to PR_SET_DUMPABLE=0 from file caps.
There was a problem hiding this comment.
Pull request overview
This PR updates nixbox’s virtiofsd launch path to use --inode-file-handles=mandatory (to prevent FD growth under --cache=auto) by introducing a helper that installs and reuses a setcap’d virtiofsd wrapper outside /nix/store, and documents the design as ADR-016.
Changes:
- Add
ensure_virtiofsd_cap()to install/return aCAP_DAC_READ_SEARCH-capable virtiofsd wrapper under$XDG_DATA_HOME. - Launch all virtiofsd instances with
--inode-file-handles=mandatoryvia the wrapper; addnixbox doctorreporting for wrapper presence. - Add ADR-016 and link it from the decisions index.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/functions.bash | Adds ensure_virtiofsd_cap() to provision a setcap’d virtiofsd wrapper and reuse it across runs. |
| bin/nixbox | Switches virtiofsd invocations to wrapper + --inode-file-handles=mandatory; adds doctor status output. |
| docs/decisions/README.md | Adds ADR-016 to the decisions index. |
| docs/decisions/016-virtiofsd-file-handles-capability.md | Documents rationale, alternatives, and operational consequences of the capability approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Restrict install dir to mode 700 and use cp --remove-destination so a symlink at the wrapper path can't redirect the setcap'd write target. - Fail loud when mkdir fails (consistent with the rest of the function). - Add setcap/getcap to prerequisite checks in ensure_setup and cmd_doctor. Note: kernel cap_inode_killpriv() already clears security.capability on write, so a same-user "swap-then-reuse-cap" attack is not possible. The hardening here is for symlink hygiene and clearer prereq errors.
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
virtiofsd --cache=autopins one O_PATH FD per cached inode (#18).--inode-file-handles=mandatoryswaps that for opaque handles, decoupling cache size from FD count — but it needsCAP_DAC_READ_SEARCH, and run-as-root alternatives regress ADR-001 or ADR-002.ensure_virtiofsd_cap()keeps a setcap'd copy under$XDG_DATA_HOME/nixbox, reinstalled on version drift. One sudo prompt on firstnixbox upand afternixbox update. Full rationale and rejected alternatives in ADR-016.Test plan
nixbox doctorreports wrapper status (NOT INSTALLED → OK after first up)nixbox upspawns daemons with--inode-file-handles=mandatory; no EPERM invirtiofsd-*.lognixbox updatetriggers wrapper reinstall on nextup🤖 Generated with Claude Code