Use userspace bounce buffers if secret_free = True and no swiotlb configured #5156
Conversation
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
roypat
force-pushed
the
secret-freedom-userspace-bounce
branch
4 times, most recently
from
6b758d2 to
262639c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/secret-hiding #5156 +/- ##
=========================================================
- Coverage 82.72% 82.51% -0.22%
=========================================================
Files 251 251
Lines 27522 27731 +209
=========================================================
+ Hits 22768 22881 +113
- Misses 4754 4850 +96
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
roypat
force-pushed
the
secret-freedom-userspace-bounce
branch
8 times, most recently
from
c5ec769 to
d6eadd4
Compare
kalyazin
reviewed
Apr 17, 2025
kalyazin
previously approved these changes
Apr 24, 2025
roypat
force-pushed
the
secret-freedom-userspace-bounce
branch
from
d6eadd4 to
da205ce
Compare
kalyazin
previously approved these changes
Apr 24, 2025
JackThomson2
previously approved these changes
Apr 24, 2025
It has a boolean inside of it that determines whether we actually bounce, so rename it to reflect that. Signed-off-by: Patrick Roy <[email protected]>
This is particularly useful for virtio devices, where on-demand allocation of bounce buffers leads to sever performance impacts (~80%) in synthetic throughput tests. Additionally, for virtio devices we can know approximately what the optimal size of a statically allocated bounce buffer is. Allocate bounce buffers on the heap, as trying to even temporarily place a 65k bounce buffer on the stack can lead to stack overflow errors. Signed-off-by: Patrick Roy <[email protected]>
We put the bounds as AsFd to work around limitations in vm-memory about not implementing Read/WriteVolatile for &File and &mut File, but it ended up just causing even more workarounds being required elsewhere (vsock unittests, where AsFd cannot meaningfully be implemented for mocks). So just bite the bullet and clone some files until vm-memory gets some more impls. Signed-off-by: Patrick Roy <[email protected]>
kvm-clock is incompatible with direct map removal for now. This is because kvm-clock tries to access guest memory through the direct map. Additionally, it does not handle failures during guest-attempted activations of kvm-clock gracefully (e.g. it cannot/does not communicate these back to the guest). This means a guest will unconditionally assume that if it wrote to the kvm-clock MSR to activate kvm-clock, it will work. But if KVM internally fails to activate kvm-clock, KVM will never write the information the guest expects into guest memory, resulting in the guest reading garbage data (generally, zeros), resulting in division by zero panics in the guest. Hence, explicitly tells guests that they shouldn't even try to enable kvm-clock, if they value their lives. Signed-off-by: Patrick Roy <[email protected]>
Add support to our virtio devices to allow userspace bounce buffering of virtio buffers. This is an alternative to swiotlb. Don't implement it for vhost-user-blk and for virtio-block with async engine, because I have no idea how that would even work. Signed-off-by: Patrick Roy <[email protected]>
Mark vhost-user and async block engine as incompatible, as for vhost-user-blk, we would need to communicate the need for bounce buffers to the backend somehow, and for the async block engine we would need to somehow keep the bounce buffers around until io_uring finishes requests (which is not impossible, but complicated and not needed for now). Signed-off-by: Patrick Roy <[email protected]>
Now that we have userspace bounce buffers, we can run secret freedom tests (including perf tests!) with then, which additionally allows us to run them on x86 too. Remove the swiotlb test cases from the performance tests, as the performance impact here was shown too significant. Signed-off-by: Patrick Roy <[email protected]>
Ensure that `ps(1)` does not truncate the command, which might result in the grep failing (if the jailer_id gets truncated), using the -ww option. While we're at it, also use -o cmd so that ps only prints the command names and nothing else (as we're not using anything else from this output). This causes false-positives instead of false-negatives funnily enough, because we're using check_output, meaning if the grep doesnt find anything we fail the command (in the "everything works" scenario, firecracker is dead but grep still matches the "ps | grep" process itself). Signed-off-by: Patrick Roy <[email protected]>
Currently, creation of UFFDs fails with Failure to create UFFD: System error Signed-off-by: Patrick Roy <[email protected]>
roypat
force-pushed
the
secret-freedom-userspace-bounce
branch
from
98c83b3 to
a95c1db
Compare
JackThomson2
approved these changes
Apr 24, 2025
kalyazin
approved these changes
Apr 25, 2025
roypat
merged commit 82ca3c9
into
firecracker-microvm:feature/secret-hiding
5 of 7 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Changes
Implement userspace bounce buffering for virtio devices if secret freedom is enabled, yet no swiotlb region is configured or even possible (e.g. x86)
Reason
swiotlb had significantly worse performance than expected (up to 80% throughput degradations), so try userspace bounce buffers instead, as these allow us to persist the bounce buffers (instead of swiotlb which re-allocates a new bounce buffer every time)
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.