Speed up sandbox setup with detached mounts - #788
domenkozar wants to merge 7 commits into
Conversation
05e1ac9 to
3a5eadc
Compare
domenkozar
changed the title
4db3855 to
62cef7e
Compare
|
Tested on kernels:
|
|
Can io_uring help with large numbers of bind mounts? |
smcv
left a comment
smcv
left a comment
There was a problem hiding this comment.
This is a lot of LLM-assisted code in one large commit, and from the description it seems to be making several significant structural changes, all at the same time. bubblewrap is a security-sensitive project, and I am not confident that either the submitter or the maintainers can meaningfully review this single commit or audit it for security vulnerabilities.
(There is also the issue that a large code change from a contributor with no prior involvement could be an attempt to disguise a vulnerability being added deliberately - I'm not accusing you of doing this, but I also can't prove that you aren't!)
There are correctness/robustness reasons why we do want bubblewrap to use the "new mount APIs" (#755) but to be reviewable, that would have to be done more gradually, one refactor at a time, in a way that means we can be more confident about each change as it goes in.
Read the filesystem type from /proc/self/mounts instead of letting libc write a complete, architecture-dependent statfs structure into a two-field ctypes buffer. Validation: all six Meson suites pass in Debian trixie with warnings as errors, SELinux, ASan, UBSan and PYTHONMALLOC=debug. Signed-off-by: Domen Kožar <domen@cachix.org>
Use open_tree, mount_setattr and move_mount while retaining the existing two-pivot root lifecycle. Apply restrictions to the detached clone without clearing inherited flags. Prefer the original descriptor for bind-fd operations, retaining the existing reopen and identity verification for kernels that reject descriptors from the parent mount namespace. Only unavailable syscalls and unsupported clone operations select the existing implementation; permission and attach errors remain fatal. Keep the explicit mount-setattr fallback and builds without libc support. Validation: all seven Meson suites pass with SELinux, warnings as errors, ASan, UBSan and Python debug allocation. New seccomp tests cover absent, denied and invalid operations and forced fallback. Signed-off-by: Domen Kožar <domen@cachix.org>
Introduce shared filesystem creation helpers and use them for unlabelled tmpfs mounts. Preserve mode, size, nosuid and nodev, retaining the existing root lifecycle and SELinux-labelled mount path. Unavailable syscalls select the traditional mount API before attachment; denials remain fatal. Validation: all seven Meson suites pass with SELinux, warnings as errors, ASan, UBSan and Python debug allocation, including tmpfs permissions and size assertions and filesystem syscall fault injection. Signed-off-by: Domen Kožar <domen@cachix.org>
Use the shared descriptor mount helper when creating procfs for a PID namespace. Preserve nosuid, nodev, noexec and the existing masking of sensitive proc entries. Keep host-proc binds and the root lifecycle unchanged. Validation: all seven sanitizer-enabled Meson suites pass. Add proc-specific syscall failure tests and mount flag assertions in a new PID namespace. Signed-off-by: Domen Kožar <domen@cachix.org>
Set newinstance, ptmxmode=0666 and mode=620 through fsconfig, then attach the nosuid/noexec mount through its descriptor. Retain the existing fallback when a syscall is unavailable. Validation: all seven sanitizer-enabled Meson suites pass. Check PTY allocation, master/slave permissions and mount restrictions, and inject failures specifically into devpts newinstance configuration. Signed-off-by: Domen Kožar <domen@cachix.org>
Move the existing root setup and second pivot into two helpers. Preserve every statement and its order, including cwd capture after the temporary tmpfs mount and before chdir. No backend selection or namespace behavior changes. Validation: both extracted bodies match the original statements byte for byte; all seven Meson suites pass with warnings as errors, SELinux, ASan, UBSan and Python debug allocation. Signed-off-by: Domen Kožar <domen@cachix.org>
Reuse the descriptor bind and filesystem helpers to build eligible layouts before entering the root with one pivot. Retain the two-pivot lifecycle for root overmounts, supplied FDs, unsupported setup operations and unavailable kernel semantics. Probe detached clone and attach behavior before selection. Once selected, unavailable APIs and permission failures remain fatal. Root-relative openat2 lookups reject magic links; root overmount aliases are checked against the retained root descriptor. Preserve host-to-sandbox propagation and close both root descriptors at entry. Validation: all eight Meson suites pass in Debian trixie on Linux 7.0.10 with SELinux, warnings as errors, ASan, UBSan and PYTHONMALLOC=debug. The detached-root suite includes 56 checks covering restrictions, working directories below /tmp, devices, tmpfs limits, FD and capability cleanup, propagation through path and inherited-FD sources, unsupported APIs, post-selection failures and traced pivot counts. All six suites pass with the new APIs compiled out, and all seven applicable suites pass with only filesystem creation compiled out. Fresh matched GCC 15.2 release-build benchmarks against 26bb788 show 13.2-21.3% lower median elapsed time for serial launches and 26.9-34.7% lower elapsed time per sandbox with four concurrent launches across minimal and 3/32/128-bind layouts. Twelve alternating pairs of 32 launches per case include probing and cleanup. Root-overmount controls are 2.5%/0.2% slower; direct-exec controls are 2.2%/2.4% slower. These are host-specific startup and throughput measurements, not workload speedups. Runtime validation also passes in Linux 5.10.269 and 5.15.220 VMs. The 5.10 kernel skips tests requiring mount_setattr; 5.15 runs all eight suites, including 43 detached-root fallback checks. Traces confirm two pivots on both older kernels. Capture inherited FDs before ctypes initializes libffi so runtime-owned library descriptors are not mistaken for setup leaks. Signed-off-by: Domen Kožar <domen@cachix.org>
62cef7e to
89088f2
Compare
|
@smcv thank you for the review! I've split it up into 6e558d4 that's a cleanup, plus 9717393 that builds the foundation and then follow up commits replace existing code to use it. It's now a bit bigger, ~1200 lines of code while half of that is tests. It would be great to have some benchmarking machinery in bubblewrap as well for experiments like this, I can contribute that too. |
Given the reputation of security issues in |
That's a different thing. The security issues in io_uring are userspace to kernel exploits not sandbox to host. It's a different security barrier and bwrap using it does not cause any security issues for the sandbox it creates. |
If this is a valid bug fix (which it probably is), please open a separate PR for it, so that landing it isn't blocked by reviewing hundreds of lines of subtle, security-sensitive mount manipulation. |
|
#793 is an alternative to this with some of the same ideas, but a lot simpler. As a general design principle, I'd suggest looking at the cost/benefit ratio of what's in this branch, choosing the lowest-cost or highest-benefit parts, and focusing on just those. Because the limiting factor on bubblewrap is its maintainers' limited time (in the short term for reviewing and testing, but also in the longer term for the maintenance and security vulnerability response that the community expects from us), the "cost" here is mostly a matter of how much code churn there is, and how much code you are asking us to become responsible for. Startup time is not really something that bubblewrap is optimized for: as long as it's "fast enough to not be particularly noticeable", that's good enough. The most important things for bubblewrap are that it has the security properties that it's aiming for, and it's sufficiently simple that it can be reviewed and understood. If we end up with two completely separate code paths that behave differently, that's twice as many places where security vulnerabilities can be lurking, so we do need to push back against increasing complexity. In #793 and previously #756, you'll see that the "happy path" and the fallback are intentionally doing the same thing in different ways, and I think that's a good approach to keeping things comprehensible. In typical projects that make use of bubblewrap, like Flatpak and the Steam Runtime, the complexity and time of starting an app seem to be dominated by the setup steps that need to happen around bubblewrap, rather than bubblewrap itself, and if that's the case then speeding up bubblewrap isn't going to have a particularly noticeable impact on the overall app startup, even if you could somehow speed it up by 99%. Meanwhile, this PR also adds a lot of code to the tests. In general adding test coverage is great, but adding test coverage in the same commit that significantly changes the implementation tends to be a red flag. If the tests are appropriate for how bwrap already behaves, then they don't need to be part of a PR that rewrites bwrap's internals: they could be part of a targeted PR that only improves test coverage. Conversely, if the tests would have failed in current bwrap and are adding coverage for an intentional behaviour change (like a bug fix), it would often be appropriate to add them after the behaviour change, rather than at the same time. I'm sure your LLM has learned that code that comes with tests is more likely to get merged, and that's true up to a point, but tests are code too: even in the test suite, adding lots of code that the project maintainers are expected to review and become responsible for does have a cost. |
That's really only the case if the benefit that it brings us is greater than the ongoing cost of being responsible for it. For bubblewrap's original purpose (Flatpak), launching hundreds of containers per second was never really a goal: Flatpak only needs to start O(1) containers per app launch, and launching hundreds of app instances per second is not a typical pattern. |
There's tricky bits for supporting all kernels, because you need to detect if the kernel supports new mounting and that has bugs, hence why it gets complicated. See my comment at #788 (comment) Tests that come with it try to exercise that from different directions. If you prefer the naive implementation that's fine, I personally don't care about older kernels but it would be good to check against policy of bubblewrap what kernels are supported and tested. |
|
Oh and here we're using single pivot, which is where part of the performance optimizations come from, but also where more code changes are needed. Perfectly fine to me to split those, that 's why I made it 7 commits. Happy to rebase if the other PR is merged with the new mount api. |
This is rather assuming that there is a policy, or resources to write a policy, or resources to test on old kernels... bubblewrap was originally written for Flatpak, which tries to be backportable to old OSs, within reason. In practice Flatpak currently requires Linux 5.8, but even that is apparently too high a requirement for some Flatpak users (flatpak/flatpak#6774). It's also used by Steam, which is in the process of raising its minimum glibc from 2.15 (at least in theory) to 2.31, but does not currently have any formal minimum kernel requirement. (I'm hoping that Steam will start documenting a minimum kernel soon, at which point we'll know what can and can't be assumed there.) The The oldest thing I routinely test on is Ubuntu 22.04, with occasional excursions into 20.04, but I'm sure other contributors have a different idea of what is the minimum. |
I have a NixOS test that basically loops through all major supported kernel releases and runs the test, that's how I managed to test this pretty fast and found that we need a bit more complex logic. I'd be happy to contribute that and benchmarks if you'd be willing to maintain those. Happy to help out. |
4 participants
Bubblewrap currently switches into a temporary root before building the sandbox filesystem, then switches roots again to enter the sandbox. This change builds supported layouts in a detached tmpfs and enters the finished sandbox with a single
pivot_root()call. The new mount APIs let us assemble the mounts through file descriptors while preserving inherited mount restrictions.The new setup path is selected automatically for unprivileged sandboxes that create user and PID namespaces and use supported setup operations. Other layouts, including mounts over
/, keep the existing path. Before selecting the new path, bubblewrap checks that the kernel can clone and attach detached mounts. Missing libc or kernel support falls back to the existing path; permission errors and failures after selection still stop setup.The detached mount operations live in a separate C module with explicit root and host descriptors. Layout checks stay alongside option handling, while preparation, filesystem setup and root entry are separate steps. Mount errors identify the source and destination.
Tests cover layout selection, mount restrictions, file descriptor and capability cleanup, missing or denied kernel operations, device access, tmpfs permissions and size limits, and bind error messages. The mount cases have names and explicit expected outcomes.
Validation in an isolated Debian trixie container on Linux 7.0.10: all seven test suites pass, including all 48 detached-mount checks. With detached mounts compiled out, all six applicable suites pass. Both configurations use GCC 14.2, SELinux support, warnings as errors, AddressSanitizer, UndefinedBehaviorSanitizer and Python's debug allocator. Environment-dependent FUSE, logging, overlayfs and mqueue subtests retain their normal skips.
In static-shell launch benchmarks against upstream
26bb788, median launch time fell by 3.4–18.6% when running one sandbox at a time. With four concurrent launches, elapsed time per sandbox fell by 22.8–28.3%. The concurrent figures measure throughput, not individual launch latency.Median milliseconds per sandbox, before → after:
These measurements used Linux 7.0.10 on a Ryzen 7 7840S (16 logical CPUs), with GCC 15.2.0 and matching Meson release builds. Standard layouts include user, PID, network, IPC and UTS namespaces, proc, dev, tmpfs, a read-only static executable and the listed number of input-file bind mounts. Each result uses 12 alternating matched pairs of 32 launches with shuffled layout order. Timings include the compatibility probe, process creation, setup, execution and cleanup.
For comparison, the unchanged setup path for mounts over
/differed by 2.0% with serial launches and 0.7% with concurrent launches. Running the command directly differed by 5.1% (17 µs) and 1.5%, respectively. These results describe startup costs on this host; longer-running workloads may benefit less.All done by GPT6.