Use pointers instead of slices when operating on guest memory #217

roypat · 2023-02-28T11:03:26Z

Summary of the PR

Currently, vm-memory internally uses slices to operate on guest memory, in addition to providing methods for passing slices to guest memory to arbitrary user-code (via read_from and write_to). This PR makes sure that all internal operations on guest memory happen through pointers, to prevent accidental triggering of undefined behavior via violation of aliasing rules (either in rust-land, or due to the fact that the guest can modify its own memory at will while we hold references to guest memory).

As a consequence of this PR, the unittests of vm-memory/src/volatile_memory.rs pass under miri

Addresses #45

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

All commits in this PR are signed (with git commit -s), and the commit
message has max 60 characters for the summary and max 75 characters for each
description line.
All added/changed functionality has a corresponding unit/integration
test.
All added/changed public-facing functionality has entries in the "Upcoming
Release" section of CHANGELOG.md (if no such section exists, please create one).
Any newly added unsafe code is properly documented.

andreeaflorescu · 2023-03-06T10:34:49Z

@zachreizner @jiangliu @bonzini you were part of the initial discussions in #45. Can you also take a look at this PR?

andreeaflorescu

We discussed offline about the performance impact of this change. Can you also please add the relevant data to this PR for tracking purposes?

andreeaflorescu · 2023-03-06T11:37:44Z

src/volatile_memory.rs

+            }
+
+            loop {
+                if left < min_align {


Why not use this as the while condition and keep the while instead of a loop?

...good question - will change!

src/volatile_memory.rs

roypat · 2023-03-06T15:56:13Z

We discussed offline about the performance impact of this change. Can you also please add the relevant data to this PR for tracking purposes?

Sure, I can summarize (firecracker's performance pipelines are not public, so I sadly cannot simply link).

The main performance impact here is the additional copy in the VolatileSlice functions that either read from a Read implementation to guest memory or write to guest memory using a Write implementation. We could observe the performance impact on a firecracker fork that simply updates its vm-memory dependency to point towards this patch using our performance test suite. Firecracker uses the above-mentioned functions for the vsock and block devices, where the Read/Write impl is a UnixStream and a File respectively. Here, we noticed a degradation of throughput between 5% and 10% compared to our baselines (the block device degradation was actually within statistical error bounds). All other test in the suite passed.

andreeaflorescu · 2023-03-06T15:57:38Z

@roypat I just realized that we also have some microbenchmarks in this repository. Do you think you could run them as well?

roypat · 2023-03-06T16:01:03Z

@roypat I just realized that we also have some microbenchmarks in this repository. Do you think you could run them as well?

I did run the microbenchmarks (first on main and then on this patch to get criterion to print the difference) and got

     Running benches/main.rs (target/release/deps/main-dd966502b4f16b8b)
WARNING: HTML report generation will become a non-default optional feature in Criterion.rs 0.4.0.
This feature is being moved to cargo-criterion (https://github.com/bheisler/cargo-criterion) and will be optional in a future version of Criterion.rs. To silence this warning, either switch to cargo-criterion or enable the 'html_reports' feature in your Cargo.toml.

Gnuplot not found, using plotters backend
VolatileSlice::copy_to_u8                                                                             
                        time:   [15.029 ns 15.073 ns 15.129 ns]
                        change: [-2.6517% -2.2393% -1.7593%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 11 outliers among 200 measurements (5.50%)
  2 (1.00%) low mild
  3 (1.50%) high mild
  6 (3.00%) high severe

VolatileSlice::copy_to_u16                                                                             
                        time:   [106.06 ns 106.12 ns 106.20 ns]
                        change: [-0.0024% +0.0465% +0.1002%] (p = 0.09 > 0.05)
                        No change in performance detected.
Found 23 outliers among 200 measurements (11.50%)
  12 (6.00%) high mild
  11 (5.50%) high severe

VolatileSlice::copy_to_volatile_slice                                                                             
                        time:   [131.21 ns 131.56 ns 131.98 ns]
                        change: [+0.1682% +0.5149% +0.9015%] (p = 0.01 < 0.05)
                        Change within noise threshold.
Found 19 outliers among 200 measurements (9.50%)
  1 (0.50%) low mild
  7 (3.50%) high mild
  11 (5.50%) high severe

VolatileSlice::copy_from_u8                                                                             
                        time:   [13.817 ns 13.823 ns 13.831 ns]
                        change: [-0.1741% -0.0820% +0.0181%] (p = 0.10 > 0.05)
                        No change in performance detected.
Found 36 outliers among 200 measurements (18.00%)
  14 (7.00%) high mild
  22 (11.00%) high severe

VolatileSlice::copy_from_u16                                                                             
                        time:   [105.78 ns 105.82 ns 105.87 ns]
                        change: [-0.0194% +0.0627% +0.1505%] (p = 0.13 > 0.05)
                        No change in performance detected.
Found 36 outliers among 200 measurements (18.00%)
  7 (3.50%) high mild
  29 (14.50%) high severe

First commit in a series of commits to elimate slices to guest memory. Additionally, changes pointer arithmetic to use std:ptr::add and adds safety comments explaining why invariants of pointer arithmetic are upheld. Function is now marked unsafe because part of the invariants have to be upheld by the caller. Requirement for the memory regions to be non overlapping is due to the copy loop not correctly copying values if the regions do overlap (however, since the function is only called for copy from guest-memory to rust-memory or vice-versa, this is trivially unheld in praxis). Lastly, changes `copy_single` to take pointer arguments instead of usize, to avoid unneccessary conversion of the form *u8 -> usize -> *u8. Related to rust-vmm#45 Signed-off-by: Patrick Roy <[email protected]>

This function previously took slices, where one slice pointed to rust-allocated memory (fine), and one slices pointed to mmap'd guest memory (potentially problematic). The latter could result in undefined behavior, as we cannot assume exclusive ownership of guest memory if a virtual machine is running: We need to assume that the guest inside the VM implicitly holds a mutable reference to the entire guest memory. By using pointers instead of reference, this is still a problem due to data races being possible, but the worst case scenario here is inconsistent reads/writes, whereas the aliasing rules are something the compiler potentially uses for optimizations. Signed-off-by: Patrick Roy <[email protected]>

Taking rust-style references to guest memory of a running virtual machine might violate the aliasing rules, as the guest can modify its memory at will. Additionally, since the `read_from` and `write_to` functions "leak" references to guest memory to code outside of the vm-memory crate, it is possible to trigger undefined behavior in single-threaded, safe code, for example by creating a `Read` implementation the reads from a volatile slice: struct VolatileSliceReader<'a> { src: VolatileSlice<'a, ()> } impl<'a> Read for VolatileSliceReader<'a> { fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> { Ok(self.src.copy_to(buf)) } } \#[test] // test fails under miri fn undefined_behavior() { let mut data = vec![0u8; 20]; let vslice = unsafe {VolatileSlice::new(data.as_mut_ptr(), 20)}; let mut reader = VolatileSliceReader {src: vslice}; vslice.read_from(0, &mut reader, 10); } Since this trait was marked `pub(crate)`, this is not a breaking change. As a consequence, the `read_from` and `write_to` functions of `Bytes` implementations now always first copy to a buffer allocated within the function, before transferign data from/to the Read/Write object provided. There is no safe way to keep the existing abstraction, as Rust does not give us a low-level interface to Read/Write that operates with pointers. Signed-off-by: Patrick Roy <[email protected]>

As outlined in the previous commit message, these function had only a singular call-site, which could not guarantee the safety invariants were uheld. Since these functions were non-public, removing them is not a breaking change. Signed-off-by: Patrick Roy <[email protected]>

Due to the API exposed by the VolatileMemory trait constructing a VolatileSlice from an immutable reference, this impl allowed mutation through an immutable reference. This is because the signature of `get_slice` has its first argument desugar to `&&mut [u8]` instead of `&mut [u8]` (note the double reference). This caused undefined behavior due to a cast from immutable reference to mutable pointer. The implementation has been replaced with `From<&mut [u8]> for VolatileSlice`, which is safe. Since this impl was only used in testing code, the impact of this breaking change should be low. For the same reasons, the `VecMem` struct in volatile_memory::tests has been removed. Signed-off-by: Patrick Roy <[email protected]>

Signed-off-by: Patrick Roy <[email protected]>

stsquad

Having read through I'm pretty happy that the end state is better and easier to follow code with much clearer safety reasoning.

These are essentially clones of the Read/Write traits from the standard library, but instead of operating on &[u8] and &mut [u8], they operate on VolatileSlices. We cannot use the stdlib traits to operate on guest memory due to unsoundness concerns, see rust-vmm/vm-memory#217 and 228. A default implementation is provided for `File` and `UnixStream` (as these are the types for which firecracker needs them). I have done experiements with instead providing a blanket implementation for `T: AsRawFd`, however ran into some problems with trait coherence rules, as such a blanket implementation makes the implementation of Read/WriteVolatile for &[u8]/&mut [u8] impossible. This allows us to also potentially choose different implementations for different kind of `AsRawFd`s (e.g. using `recv` for sockets). This can all be revised later though. These traits would fit nicely into rust-vmm, and I'll put out feelers about interest regarding these traits after our release tasks are done. Signed-off-by: Patrick Roy <[email protected]>

With rust-vmm#217, various `read_from` and `write_to` methods across vm-memory incurred a performance penalty due to an unneccesary copy of all data read and written. This is due to these functions operating on arbitrary Read/Write implementations, which cannot generally be assumed to respect the volatile semantics of guest memory. Additionally, they required taking slices to guest memory, which was undefined behavior. This patch series fixes those short-comings by introducing a pair of new traits, `ReadVolatile` and `WriteVolatile` which are identical to the standard library's `Read` and `Write` traits, with the difference being that they operate on `VolatileSlice`s instead of `&mut [u8]`. Using these traits will therefore not incur the performance penalty of rust-vmm#217. Additionally, this will make using the vm-memory library easier for users familiar with std::io, as the APIs will be more familiar (in particular, the unintuitive "swapping" of reader/writer src and dst of the old read_from/write_to APIs is resolved). Signed-off-by: Patrick Roy <[email protected]>

These are essentially clones of the Read/Write traits from the standard library, but instead of operating on &[u8] and &mut [u8], they operate on VolatileSlices. We cannot use the stdlib traits to operate on guest memory due to unsoundness concerns, see rust-vmm/vm-memory#217 and 228. A default implementation is provided for `File` and `UnixStream` (as these are the types for which firecracker needs them). I have done experiements with instead providing a blanket implementation for `T: AsRawFd`, however ran into some problems with trait coherence rules, as such a blanket implementation makes the implementation of Read/WriteVolatile for &[u8]/&mut [u8] impossible. This allows us to also potentially choose different implementations for different kind of `AsRawFd`s (e.g. using `recv` for sockets). This can all be revised later though. These traits would fit nicely into rust-vmm, and I'll put out feelers about interest regarding these traits after our release tasks are done. Signed-off-by: Patrick Roy <[email protected]>

With rust-vmm#217, various `read_from` and `write_to` methods across vm-memory incurred a performance penalty due to an unneccesary copy of all data read and written. This is due to these functions operating on arbitrary Read/Write implementations, which cannot generally be assumed to respect the volatile semantics of guest memory. Additionally, they required taking slices to guest memory, which was undefined behavior. This patch series fixes those short-comings by introducing a pair of new traits, `ReadVolatile` and `WriteVolatile` which are identical to the standard library's `Read` and `Write` traits, with the difference being that they operate on `VolatileSlice`s instead of `&mut [u8]`. Using these traits will therefore not incur the performance penalty of rust-vmm#217. Additionally, this will make using the vm-memory library easier for users familiar with std::io, as the APIs will be more familiar (in particular, the unintuitive "swapping" of reader/writer src and dst of the old read_from/write_to APIs is resolved). Please note that `impl<T: AsRawFd> ReadVolatile for T` is not be possible, as the orphan rules will note that "upstream crates may add a new impl of trait std::os::fd::AsRawFd for type &[u8] in future versions". Signed-off-by: Patrick Roy <[email protected]>

With #217, various `read_from` and `write_to` methods across vm-memory incurred a performance penalty due to an unneccesary copy of all data read and written. This is due to these functions operating on arbitrary Read/Write implementations, which cannot generally be assumed to respect the volatile semantics of guest memory. Additionally, they required taking slices to guest memory, which was undefined behavior. This patch series fixes those short-comings by introducing a pair of new traits, `ReadVolatile` and `WriteVolatile` which are identical to the standard library's `Read` and `Write` traits, with the difference being that they operate on `VolatileSlice`s instead of `&mut [u8]`. Using these traits will therefore not incur the performance penalty of #217. Additionally, this will make using the vm-memory library easier for users familiar with std::io, as the APIs will be more familiar (in particular, the unintuitive "swapping" of reader/writer src and dst of the old read_from/write_to APIs is resolved). Please note that `impl<T: AsRawFd> ReadVolatile for T` is not be possible, as the orphan rules will note that "upstream crates may add a new impl of trait std::os::fd::AsRawFd for type &[u8] in future versions". Signed-off-by: Patrick Roy <[email protected]>

With vm-memory 0.13.0, we introduced versions of the Read and Write traits for operations on guest memory (`VolatileRead` and `VolatileWrite`). These replace various `Read` and `Write` based APIs that had suboptional performance due to requiring to first copy all data from guest memory to hypervisor memory to ensure soundness. See also rust-vmm/vm-memory#217. Signed-off-by: Patrick Roy <[email protected]>

Updates the requirements on [vm-memory](https://github.com/rust-vmm/vm-memory) to permit the latest version. - [Release notes](https://github.com/rust-vmm/vm-memory/releases) - [Changelog](https://github.com/rust-vmm/vm-memory/blob/main/CHANGELOG.md) - [Commits](rust-vmm/vm-memory@v0.12.2...v0.13.0) --- updated-dependencies: - dependency-name: vm-memory dependency-type: direct:production ... In addition to the dependabot changes, this commit updates usages of deprecated functions. With vm-memory 0.13.0, we introduced versions of the Read and Write traits for operations on guest memory (`VolatileRead` and `VolatileWrite`). These replace various `Read` and `Write` based APIs that had suboptional performance due to requiring to first copy all data from guest memory to hypervisor memory to ensure soundness. See also rust-vmm/vm-memory#217. Signed-off-by: Patrick Roy <[email protected]> Co-authored-by: dependabot[bot] <[email protected]>

roypat requested review from alexandruag, bonzini and jiangliu as code owners February 28, 2023 11:03

roypat force-pushed the no-slices-to-guest-mem branch 2 times, most recently from 90a8280 to 8f8cbf5 Compare February 28, 2023 11:24

andreeaflorescu reviewed Mar 6, 2023

View reviewed changes

roypat added 5 commits March 6, 2023 16:04

roypat force-pushed the no-slices-to-guest-mem branch from 8f8cbf5 to 37a537d Compare March 6, 2023 16:05

Adjust coverage for x86

f84564d

Signed-off-by: Patrick Roy <[email protected]>

jiangliu approved these changes Mar 18, 2023

View reviewed changes

stsquad approved these changes Mar 28, 2023

View reviewed changes

andreeaflorescu approved these changes Mar 29, 2023

View reviewed changes

andreeaflorescu merged commit 1bdb2be into rust-vmm:main Mar 29, 2023

andreeaflorescu mentioned this pull request Mar 30, 2023

Remove as_slice from GuestMemoryRegion #45

Closed

roypat mentioned this pull request Apr 5, 2023

Prepare release 0.11.0 #222

Merged

4 tasks

roypat mentioned this pull request May 22, 2023

CI: Run miri on each pull request #232

Merged

4 tasks

roypat mentioned this pull request Jul 24, 2023

[RFC] ReadVolatile and WriteVolatile traits for efficient data transfer from/to guest memory #247

Merged

4 tasks

roypat mentioned this pull request Dec 22, 2023

VcpuFd::set_kvm_immediate_exit may lead to data races/undefined behavior rust-vmm/kvm#248

Closed

roypat mentioned this pull request Apr 15, 2024

Get a slice of a GuestRegionMmap #286

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use pointers instead of slices when operating on guest memory #217

Use pointers instead of slices when operating on guest memory #217

Uh oh!

roypat commented Feb 28, 2023

Uh oh!

andreeaflorescu commented Mar 6, 2023

Uh oh!

andreeaflorescu left a comment

Uh oh!

andreeaflorescu Mar 6, 2023

Uh oh!

roypat Mar 6, 2023

Uh oh!

Uh oh!

roypat commented Mar 6, 2023

Uh oh!

andreeaflorescu commented Mar 6, 2023

Uh oh!

roypat commented Mar 6, 2023

Uh oh!

stsquad left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Use pointers instead of slices when operating on guest memory #217

Use pointers instead of slices when operating on guest memory #217

Uh oh!

Conversation

roypat commented Feb 28, 2023

Summary of the PR

Requirements

Uh oh!

andreeaflorescu commented Mar 6, 2023

Uh oh!

andreeaflorescu left a comment

Choose a reason for hiding this comment

Uh oh!

andreeaflorescu Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

roypat Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

roypat commented Mar 6, 2023

Uh oh!

andreeaflorescu commented Mar 6, 2023

Uh oh!

roypat commented Mar 6, 2023

Uh oh!

stsquad left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants