verify add_used operation using guestmemory stubs #346
Conversation
Run kani as a part of the CI pipeline. In particular, run the proofs for virtio-queue. In some cases, kani may not finish so set a twenty minutes timeout. Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]> Signed-off-by: Siddharth Priya <[email protected]>
Add the verify_spec_2_7_7_2() proof to verify that the implementation of queue satisfies 2.7.7.2 requirement. The proof relies on whether the EVENT_IDX feature has been negotiated. Conversely with `test_needs_notification()` test, this proof `tests` for all possible values of the queue structure. Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]> Signed-off-by: Siddharth Priya <[email protected]>
|
@roypat Hi Patrick, thanks |
Add the verify_spec_2_7_7_2() proof to verify that the implementation of queue satisfies 2.7.7.2 requirement. The proof relies on whether the EVENT_IDX feature has been negotiated. Conversely with `test_needs_notification()` test, this proof `tests` for all possible values of the queue structure. Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
GuestMemory trait brings in Bytes trait and therefore hardcodes impl for write_obj and store methods. This does not work well with Kani which now has to process these methods symbolically. The solution is to provide NOP impl for these methods. My current solution is to use kani stub macro to replace these methods by local stubs. Another solution can be to provide the proof writer a way to construct GuestMemory such that they can stub out methods as needed. Signed-off-by: Siddharth Priya <[email protected]>
priyasiddharth
force-pushed
the
verify_add_used
branch
from
959d811 to
7113f45
Compare
| #[kani::unwind(0)] | ||
| #[kani::stub(write_elem, stub_write_elem)] | ||
| #[kani::stub(store_elem, stub_store_elem)] | ||
| fn verify_add_used() { |
| @@ -466,20 +774,12 @@ impl QueueT for Queue { | |||
| .used_ring | |||
| .checked_add(offset) | |||
| .ok_or(Error::AddressOverflow)?; | |||
| mem.write_obj(VirtqUsedElem::new(head_index.into(), len), addr) | |||
| .map_err(Error::GuestMemory)?; | |||
| write_elem(mem, addr, head_index, len)?; | |||
There was a problem hiding this comment.
delegating to local functions that can be stubbed by kani
There was a problem hiding this comment.
I wonder if the only way to make this proof passes is by stubbing. I would like to understand better why write_obj() is taking so long.
There was a problem hiding this comment.
There can be multiple problems:
- State space can explode
- There can be implicit loops (depending on semantics of copy) ... since we are writing a many-byte buffer .. and in the proof we say don't unroll/unroll once so all bytes may not be written, or other problems
Likely Flow:
The write_obj method delegates to write_slice :https://sourcegraph.com/github.com/rust-vmm/vm-memory/-/blob/src/mmap/mod.rs?L190-192
This copies bytes from src to dst: https://sourcegraph.com/github.com/rust-vmm/vm-memory/-/blob/src/bytes.rs?L538
Both problems can occur with this code.
See the commit from firecracker: https://sourcegraph.com/github.com/firecracker-microvm/firecracker/-/commit/1e680c64ae6a6f86f207d3b4e7be3a72fb95fc76
In firecracker it looks like problem 2 does not occur since we still have loops/array-copy in the fake implementation.
| Ordering::Release, | ||
| ) | ||
| .map_err(Error::GuestMemory) | ||
| store_elem(mem, self.used_ring, self.next_used.0) |
| } | ||
|
|
||
| impl GuestMemory for ProofGuestMemory { | ||
| type R = vm_memory::GuestRegionMmap; |
There was a problem hiding this comment.
I replaced this with something like type R = MyOptimizedRegion; and then this leaded me to implement Bytes<MemoryRegionAddress> and GuestMemoryRegion for MyOptimizedRegion. I also struggled to implemented new(), which looks like:
impl MyOptimizedRegion {
pub fn new(start: GuestAddress, len: u64) -> Self {
// Allocate the memory region and bitmap
Self {
region_start: start,
region_len: len,
data: vec![0; len as usize],
region_bitmap: AtomicBitmap::new(len as usize, NonZero::new(4096).unwrap()),
}
}
}I am worried that this approach would add a lot of boiler plate code and thus code duplication. Any comments @roypat ? Thanks
There was a problem hiding this comment.
FYI the error during cargo kani is:
Check 4516: <std::ops::Range<usize> as std::iter::Iterator>::fold::<(), {closure@std::iter::adapters::map::map_fold<usize, std::sync::atomic::AtomicU64, (), {closure@vm_memory::bitmap::AtomicBitmap::new::{closure#0}}, {closure@std::iter::Iterator::for_each::call<std::sync::atomic::AtomicU64, {closure@std::vec::Vec<std::sync::atomic::AtomicU64>::extend_trusted<std::iter::Map<std::ops::Range<usize>, {closure@vm_memory::bitmap::AtomicBitmap::new::{closure#0}}>>::{closure#0}}>::{closure#0}}>::{closure#0}}>.unwind.0
- Status: FAILURE
- Description: "unwinding assertion loop 0"
- Location: ../../../../../github/home/.rustup/toolchains/nightly-2024-10-03-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:2583:9 in function <std::ops::Range<usize> as std::iter::Iterator>::fold::<(), {closure@std::iter::adapters::map::map_fold<usize, std::sync::atomic::AtomicU64, (), {closure@vm_memory::bitmap::AtomicBitmap::new::{closure#0}}, {closure@std::iter::Iterator::for_each::call<std::sync::atomic::AtomicU64, {closure@std::vec::Vec<std::sync::atomic::AtomicU64>::extend_trusted<std::iter::Map<std::ops::Range<usize>, {closure@vm_memory::bitmap::AtomicBitmap::new::{closure#0}}>>::{closure#0}}>::{closure#0}}>::{closure#0}}>
Summary of the PR
We need to stub out or provide fake implementation for
GuestMemoryread/write operations since they are too expensive forkanito execute as-is.This problem was faces by
firekrackeralso and solved by using a fake impl (link).in
vmm-virtioGuestMemory is hardcoded to use the default impl (link) so it is not easy to fake it. There are a few options i see.Bytestrait forGuestMemoryfor kani - needs changes tovm-memorycratevm-virtiofunctions useGuestMemoryin production andFakeGuestMemoryin kani - needs changes tovm-virtioto conditionally compile and use the rightGuestMemoryThis PR provides the third option. However, 1 or 2 maybe a better engineering solution. Looking for inputs.
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafecode is properly documented.