bonzini
commented
7 months ago Indeed, the purpose of the whole VolatileMemory/ByteValued mechanism is to provide safe mechanisms that very closely skirt undefined behavior.
The problem is that a VMM's memory accesses (both reads and writes) are driven by whatever configuration the guests puts in the device registers, and it's not really possible to enforce that the configuration makes sense. For example you could program a device to operate from address 0x12345000, while another unrelated device DMAs from 0x12300000 to 0x123FFFFFF.
VolatileMemory/ByteValued therefore have to make some assumptions, with varying degrees of soundness. From definitely sound to least sound:
-
that VMM code using volatile (non-atomic) accesses is appropriately synchronized with guest code. For example in the case of writes, this can be done by performing volatile accesses in the CPU thread, or by following the volatile access with an atomic access or a fence. In the case of volatile reads, they can again be performed in the CPU thread, or they can be preceded by an atomic access or fence.
-
that atomics limit the compiler's choices in terms of optimization and code generation. Note that some optimization of atomics is possible[2]. But, for example, unlike regular references the compiler cannot reload a value from an AtomicXYZ. It has to keep the loaded value in a register or spill it to the stack. This is why, for example,
aligned_as_ref<T: ByteValued>is unsafe butget_atomic_ref<T: AtomicInteger>is considered safe. -
that the compiler in general cannot optimize based on seeing all possible accesses to AtomicXYZ (it clearly does not, since some accesses happen in the guest). This is important because client code will be written to validate data read from atomic references shared with the guest. This data is in general an integer index or a guest address; all conversions to host pointers happen in unsafe code. The code performs bounds-checking before reaching the unsafe-safe boundary. If the compiler tried to infer that these bounds checks are dead, the whole castle of cards falls apart.
-
that in the case of data races[1] involving integer atomics (as opposed to pointers), the undefined behavior is limited to seeing data that neither side has ever written, for example leaving a mix of the old and the new value in memory. This is related to the previous point, because validation in the end will catch the invalid data before reaching the unsafe-safe boundary.
And indeed, this last point is not entirely sound. Because only very specific synchronization patterns are employed between host and guest, there should be no undefined behavior for well-behaving guests.
However, it simply is not possible to write a data-driven program such as a VMM in a way that is 100% compliant with the Rust memory model. Even for requests serviced in the CPU thread, there's always the possibility of a multi-processor guest writing to memory at the same time. Again, vm-memory tries to protect against that by making methods like aligned_as_ref<T: ByteValued> unsafe. Whenever atomics cannot be used, safe code can use volatile accesses because volatile has similar optimization guarantees to atomics; either VolatileRef (get_ref<T: ByteValued> is likewise safe), or the Bytes trait (all implementations Bytes ultimately call into VolatileSlice's implementation of the trait).
So, for an ill-behaved guest that tries actively to cause data races, we make an assumption that is true at the processor level: that this is no different from dealing with a guest that writes random data to guest memory without causing data races. And that this constrains the kind of optimization that the compiler can perform.
In fact this is not specific to virtualization, it can happen with cross-process atomics, or it can happen at the kernel-userspace boundary for Rust code that is part of Linux or another OS.
[1] By the way, "it is not allowed to mix atomic and non-atomic accesses" is not really true: it is not allowed to mix atomic and non-atomic accesses, or atomic accesses of different sizes, if at least one is a write. If all of the accesses are reads there are no data races in either C, C++ or Rust.
[2] https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4455.html
cblichmann
Hi everyone!
During an internal "
unsafe" review, we stumbled across this line: https://github.com/rust-vmm/vm-memory/blob/c1b23a2a82eddcbc049f434ed9466f137a9623ae/src/volatile_memory.rs#L274(with
TbeingAtomicInteger).Now the std docs say:
This doesn't appear to be either enforced or documented as a safety invariant.
So this should either be clearly stated in the SAFETY comment or the code should make sure that memory accesses are not mixed.