fix(runtime): route user volumes through bwrap shared tree#945
fix(runtime): route user volumes through bwrap shared tree#945law-chain-hot wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR adds ChangesSandbox bind-mount support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89f0af72df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .and_then(|nc| nc.ca_cert_pem.clone()); | ||
| #[cfg(target_os = "linux")] | ||
| { | ||
| ctx.bind_mounts.extend(built.user_volume_bind_mounts); |
There was a problem hiding this comment.
In rootless Linux this stores the FuseBindMount handles in the launching runtime process, but boxlite run -d -v ... returns immediately (see src/cli/src/commands/run.rs:80-83) and runtime shutdown intentionally skips detached boxes (src/boxlite/src/runtime/rt_impl.rs:626-652). Dropping the LiveState then drops these handles, unmounting/stopping the FUSE-backed aggregate entries while the shim keeps running, so detached boxes lose their user volumes after the CLI exits; the mount owner needs to live in the shim/daemon or otherwise outlive the launcher.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/guest/src/container/spec.rs (1)
587-621: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a read-only variant to the test.
The new test covers the
rwpath (["rbind", "rw"]) but not theropath (["rbind", "ro"]). A second assertion withread_only: truewould guard both branches of the conditional at Line 64.🤖 Prompt for 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. In `@src/guest/src/container/spec.rs` around lines 587 - 621, The new `create_oci_spec_uses_recursive_bind_for_user_mounts` test only verifies the writable mount branch, so add a second case in the same test for a `UserMount` with `read_only: true` to cover the read-only branch in `create_oci_spec`. Reuse the existing `spec.mounts()` lookup pattern to find the `/data` mount and assert it still uses `bind` while its options resolve to the recursive read-only form. This will exercise both outcomes of the conditional that sets mount options for user mounts.src/boxlite/src/litebox/init/tasks/vmm_spawn.rs (1)
350-355: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
USER_VOLUMES_GUEST_PATHfor explicit sources.The share is mounted at
USER_VOLUMES_GUEST_PATH, but this helper reconstructs the same path fromGUEST_BASEplus a literal. Use the mount-path constant so source generation cannot drift from the actual virtiofs mountpoint.♻️ Proposed refactor
fn guest_user_volume_source(tag: &str) -> String { - PathBuf::from(GUEST_BASE) - .join("user-volumes") + PathBuf::from(USER_VOLUMES_GUEST_PATH) .join(tag) .to_string_lossy() .to_string() }🤖 Prompt for 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. In `@src/boxlite/src/litebox/init/tasks/vmm_spawn.rs` around lines 350 - 355, The guest volume source helper is rebuilding the mount path from GUEST_BASE and a literal, which can drift from the actual virtiofs mountpoint. Update guest_user_volume_source to derive the source path from USER_VOLUMES_GUEST_PATH instead, and keep the tag join logic there so all explicit sources stay aligned with the mounted share.
🤖 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 `@src/boxlite/src/fs/bind_mount/native.rs`:
- Around line 106-112: The remount_read_only helper currently adds MS_REC to the
bind remount path, but that does not make the mount tree recursively read-only;
it only affects the top-level target. Update remount_read_only so it either
walks and remounts each submount when recursive is requested, or uses a
mount_setattr-based approach for true recursive read-only behavior. If recursive
behavior is not supported here, remove the MS_REC handling from
remount_read_only and keep the remount limited to the target itself.
In `@src/boxlite/src/volumes/container_volume.rs`:
- Around line 114-132: The add_bind_volume method in ContainerVolume is
triggering Clippy’s too_many_arguments lint with the same shape as add_volume.
Add the same #[allow(clippy::too_many_arguments)] attribute to add_bind_volume
so it matches the existing pattern used by add_volume and passes CI.
---
Nitpick comments:
In `@src/boxlite/src/litebox/init/tasks/vmm_spawn.rs`:
- Around line 350-355: The guest volume source helper is rebuilding the mount
path from GUEST_BASE and a literal, which can drift from the actual virtiofs
mountpoint. Update guest_user_volume_source to derive the source path from
USER_VOLUMES_GUEST_PATH instead, and keep the tag join logic there so all
explicit sources stay aligned with the mounted share.
In `@src/guest/src/container/spec.rs`:
- Around line 587-621: The new
`create_oci_spec_uses_recursive_bind_for_user_mounts` test only verifies the
writable mount branch, so add a second case in the same test for a `UserMount`
with `read_only: true` to cover the read-only branch in `create_oci_spec`. Reuse
the existing `spec.mounts()` lookup pattern to find the `/data` mount and assert
it still uses `bind` while its options resolve to the recursive read-only form.
This will exercise both outcomes of the conditional that sets mount options for
user mounts.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff01c7cf-af15-4e50-938a-fd64024b4053
📒 Files selected for processing (16)
src/boxlite/src/fs/bind_mount/config.rssrc/boxlite/src/fs/bind_mount/fuse.rssrc/boxlite/src/fs/bind_mount/native.rssrc/boxlite/src/jailer/mod.rssrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/init/mod.rssrc/boxlite/src/litebox/init/tasks/filesystem.rssrc/boxlite/src/litebox/init/tasks/vmm_spawn.rssrc/boxlite/src/litebox/init/types.rssrc/boxlite/src/portal/interfaces/container.rssrc/boxlite/src/runtime/layout.rssrc/boxlite/src/volumes/container_volume.rssrc/guest/src/container/spec.rssrc/guest/src/service/container.rssrc/shared/proto/boxlite/v1/service.protosrc/shared/src/constants.rs
| fn remount_read_only(target: &Path, recursive: bool) -> BoxliteResult<()> { | ||
| let mut flags = MsFlags::MS_BIND | MsFlags::MS_REMOUNT | MsFlags::MS_RDONLY; | ||
| if recursive { | ||
| flags |= MsFlags::MS_REC; | ||
| } | ||
|
|
||
| mount(None::<&str>, target, None::<&str>, flags, None::<&str>).map_err(|e| { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the Linux kernel strips MS_REC in the remount path.
# Search the kernel source for the do_mount function's MS_REMOUNT handling.
# If kernel source is not available, check the mount(2) man page for MS_REC usage.
# Check if man pages are available
if command -v man &>/dev/null; then
man 2 mount 2>/dev/null | grep -A5 "MS_REC" || echo "man page not available"
else
echo "man not available — verify online: https://man7.org/linux/man-pages/man2/mount.2.html"
echo "MS_REC is documented only for MS_BIND and propagation type flags, not MS_REMOUNT."
fiRepository: boxlite-ai/boxlite
Length of output: 324
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby helpers.
file="src/boxlite/src/fs/bind_mount/native.rs"
wc -l "$file"
sed -n '1,180p' "$file"
# Find the caller(s) that pass `recursive` into remount_read_only.
rg -n "remount_read_only\(|MS_REC|MS_BIND \\| MS_REMOUNT \\| MS_RDONLY|recursive" src/boxlite/src/fs -nRepository: boxlite-ai/boxlite
Length of output: 5531
🌐 Web query:
Linux mount MS_REC remount read-only recursive bind mount kernel do_mount do_remount util-linux mount --rbind -o ro documentation
💡 Result:
In Linux, the classic mount(2) system call does not support recursive read-only bind remounting [1][2][3]. While the MS_REC flag enables recursive operations for bind mounts, attempting a recursive remount with read-only flags (such as the command mount -o rbind,remount,ro) via the legacy mount(2) interface will not apply the read-only attribute to submounts [1][4][5]. Technical details regarding this behavior include: 1. Limitation of Legacy mount(2): The classic mount(2) syscall only allows changing per-mount-point flags on a single mount point [6][7]. When using MS_REMOUNT with MS_BIND, it modifies only the target mount point, not its subtree, even if MS_REC is provided [1][4]. Historically, attempts to combine recursive bind mounts with read-only settings either failed to propagate the read-only state to nested mounts or were restricted by the kernel [8][9]. 2. Recursive Semantic via New APIs: To perform recursive read-only mounts, modern Linux systems (util-linux v2.39+) utilize the mount_setattr(2) kernel system call [1][5][3]. This API, which is file-descriptor based, allows for atomic and truly recursive attribute changes [1][4]. 3. Modern Implementation in util-linux: Recent versions of the mount(8) command support an experimental recursive read-only option when interacting with the newer kernel mount API [1][2][3]. This is invoked using the syntax: mount -o rbind,ro=recursive
Citations:
- 1: https://www.man7.org/linux/man-pages/man8/mount.8.html
- 2: https://man.archlinux.org/man/mount.8
- 3: https://dyn.manpages.debian.org/testing/mount/mount.8.en.html
- 4: https://man7.org/linux/man-pages/man8/mount.8.html
- 5: https://github.com/util-linux/util-linux/blob/e4c30643/sys-utils/mount.8.adoc
- 6: https://man.archlinux.org/man/mount.2.en
- 7: https://manpages.debian.org/bullseye/manpages-dev/mount.2.en.html
- 8: https://lists.openwall.net/linux-kernel/2014/11/06/27
- 9: https://unix.stackexchange.com/questions/128336/why-doesnt-mount-respect-the-read-only-option-for-bind-mounts
🌐 Web query:
man7 mount(2) MS_REC MS_REMOUNT read-only recursive bind mount util-linux rbind ro
💡 Result:
The classic Linux mount(2) system call does not support changing mount options recursively, such as applying a read-only flag to an existing bind mount and all its submounts using MS_REC, MS_REMOUNT, and MS_BIND simultaneously [1][2][3]. Historically, attempting to use MS_BIND | MS_REMOUNT | MS_RDONLY with MS_REC has failed to achieve recursive effects because the classic kernel mount API treats these flags as non-recursive [4]. The MS_REMOUNT flag, when used with MS_BIND, specifically modifies only the per-mount-point flags of a single mount point, allowing you to set it to read-only without altering the underlying filesystem's superblock [5][6][7][2]. To achieve recursive read-only bind mounts on modern Linux systems (util-linux v2.39+), the recommended approach is to use the mount_setattr(2) system call, or the command-line equivalent provided by the mount(8) utility [1][2][3]. You can now use the experimental recursive option, for example: mount -o rbind,ro=recursive olddir newdir This utilizes the newer file-descriptor-based kernel mount API, which correctly supports applying mount attributes recursively [1][2][3]. Standard flags applied via the classic mount(2) interface (like -o remount,bind,ro) only affect the specific VFS entry targeted and do not propagate down the mount tree [1][2][3].
Citations:
- 1: https://man7.org/linux/man-pages/man8/mount.8.html
- 2: https://github.com/util-linux/util-linux/blob/e4c30643/sys-utils/mount.8.adoc
- 3: https://man.archlinux.org/man/core/util-linux/mount.8.en
- 4: RFE: mount: define new option string "rro" for
MOUNT_ATTR_RDONLY+AT_RECURSIVEutil-linux/util-linux#1501 - 5: https://man7.org/linux/man-pages/man2/mount.2.html
- 6: https://man.archlinux.org/man/mount.2
- 7: https://manpages.debian.org/bullseye/manpages-dev/mount.2.en.html
MS_REC here does not make the read-only remount recursive. mount(2) only remounts the top-level bind target read-only; submounts stay writable. If recursive read-only is required, remount each submount (or switch to mount_setattr(2)/modern mount); otherwise drop MS_REC from this path.
🤖 Prompt for 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.
In `@src/boxlite/src/fs/bind_mount/native.rs` around lines 106 - 112, The
remount_read_only helper currently adds MS_REC to the bind remount path, but
that does not make the mount tree recursively read-only; it only affects the
top-level target. Update remount_read_only so it either walks and remounts each
submount when recursive is requested, or uses a mount_setattr-based approach for
true recursive read-only behavior. If recursive behavior is not supported here,
remove the MS_REC handling from remount_read_only and keep the remount limited
to the target itself.
|
@boxlite-agent review |
038022e to
5de9b86
Compare
📦 BoxLite review — couldn't completepowered by BoxLite |
law-chain-hot
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/boxlite/src/jailer/mod.rs (1)
325-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment implies per-volume skip; actual mechanism is a binary gate at the call site.
The comment says bind-mounted volumes "skip this path" as if
build_path_access/this loop filters them individually, but the exclusion actually happens entirely outside this function, incontext()(Line 538-547), via a coarsebind_mounts.is_empty()check that zeroes out the wholevolumesslice. Worth tightening the wording to avoid confusion for future readers of this function in isolation.🤖 Prompt for 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. In `@src/boxlite/src/jailer/mod.rs` around lines 325 - 327, The comment in the fallback user volumes block is too specific about per-volume skipping, but the exclusion is controlled earlier by the coarse `context()` gate using `bind_mounts.is_empty()`. Update the wording near `build_path_access`/the volume loop to say that direct directory virtio-fs shares are only included when the call site has not already disabled all volumes, and avoid implying this function filters `SandboxBindMount` items individually.src/boxlite/src/vmm/controller/shim.rs (1)
278-302: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
bind_mountsparameter in# Arguments.The
# Argumentsdoc section lists all other parameters but omitsbind_mounts.📝 Add missing doc entry
/// * `layout` - Box filesystem layout + /// * `bind_mounts` - Bind mounts to install in the jailer namespace before the shim starts /// /// # Returns🤖 Prompt for 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. In `@src/boxlite/src/vmm/controller/shim.rs` around lines 278 - 302, The `BoxliteShim::new` documentation is missing the `bind_mounts` entry in the `# Arguments` section. Update the doc comment for `new` to include a concise description of `bind_mounts` alongside the existing parameters so the constructor signature and docs stay aligned.
🤖 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.
Nitpick comments:
In `@src/boxlite/src/jailer/mod.rs`:
- Around line 325-327: The comment in the fallback user volumes block is too
specific about per-volume skipping, but the exclusion is controlled earlier by
the coarse `context()` gate using `bind_mounts.is_empty()`. Update the wording
near `build_path_access`/the volume loop to say that direct directory virtio-fs
shares are only included when the call site has not already disabled all
volumes, and avoid implying this function filters `SandboxBindMount` items
individually.
In `@src/boxlite/src/vmm/controller/shim.rs`:
- Around line 278-302: The `BoxliteShim::new` documentation is missing the
`bind_mounts` entry in the `# Arguments` section. Update the doc comment for
`new` to include a concise description of `bind_mounts` alongside the existing
parameters so the constructor signature and docs stay aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 69486420-7747-4b56-a20f-8018ae357af9
📒 Files selected for processing (13)
src/boxlite/src/jailer/builder.rssrc/boxlite/src/jailer/mod.rssrc/boxlite/src/jailer/sandbox/bwrap.rssrc/boxlite/src/jailer/sandbox/composite.rssrc/boxlite/src/jailer/sandbox/landlock.rssrc/boxlite/src/jailer/sandbox/mod.rssrc/boxlite/src/litebox/init/tasks/vmm_spawn.rssrc/boxlite/src/vmm/controller/shim.rssrc/boxlite/src/vmm/controller/spawn.rssrc/boxlite/src/volumes/container_volume.rssrc/guest/src/container/spec.rssrc/guest/src/service/container.rssrc/shared/proto/boxlite/v1/service.proto
✅ Files skipped from review due to trivial changes (3)
- src/boxlite/src/jailer/sandbox/composite.rs
- src/shared/proto/boxlite/v1/service.proto
- src/guest/src/service/container.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/guest/src/container/spec.rs
|
@boxlite-agent review |
5 similar comments
|
@boxlite-agent review |
|
@boxlite-agent review |
|
@boxlite-agent review |
|
@boxlite-agent review |
|
@boxlite-agent review |
2 participants
Summary
Fixes #935.
This replaces the previous per-user-volume virtiofs path with a namespace-bind model for Linux + jailer + bwrap:
/run/boxlite/shared/containers/{container_id}/volumes/{tag}SandboxBindMountplumbing so bwrap installs user volume binds inside the shim sandbox namespaceI also adjusted an existing macOS-sensitive builder test assertion so it matches
SecurityOptions::default()(seccomp_enabledis Linux-only).Validation
Local clean branch:
make fmt:rustpassedgit diff --checkpassedmake test:unit:rust FILTER=builder_preset_shortcuts_pick_known_profilespassedRemote Linux/KVM E2E on ops dev machine:
i-035e02e1ffdc7d41bap-southeast-1boxlite-sso/var/tmp/boxlite-issue935-bwrap-20260708201925make runtime:debugpassedmake clipassed-vcase passed: marker reads, RW host writeback, RO write rejection-vcase passed: marker reads, RW host writeback, RO write rejection--security disablecase passed: RW hard-link writeback, RO file unchangedprepared_sandbox_bind=13,bwrap_user_bind=11,bwrap_user_ro_bind=2,specific_fs_uservol=0active, health200Remote evidence paths:
/var/tmp/boxlite-issue935-bwrap-20260708201925/logs/make-runtime-debug-rerun.log/var/tmp/boxlite-issue935-bwrap-20260708201925/logs/make-cli.log/var/tmp/boxlite-issue935-bwrap-20260708201925/logs/e2e-volumes.logNotes
ubuntucannot access/dev/kvmbecauseubuntuis not in thekvmgroup. The E2E usedsudoin the isolated task root and did not modify ops service paths or system config.full-test-matrixwas skipped for the final push because the macOS runtime warm-cache step failed in the Code Signing subsystem after Rust unit tests had already passed (779 passedforboxlite,38 passedforboxlite-shared). The Linux/KVM validation above is the higher-signal proof for this issue.Summary by CodeRabbit
New Features
Bug Fixes