virtio-queue: add verify_add_used and stub memory region #346
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
This comment was marked as outdated.
This comment was marked as outdated.
priyasiddharth
force-pushed
the
verify_add_used
branch
from
959d811 to
7113f45
Compare
priyasiddharth
commented
Jun 19, 2025
MatiasVara
reviewed
Jun 26, 2025
priyasiddharth
force-pushed
the
verify_add_used
branch
4 times, most recently
from
96d3228 to
89e9614
Compare
MatiasVara
reviewed
Jul 31, 2025
priyasiddharth
force-pushed
the
verify_add_used
branch
from
89e9614 to
7f83b55
Compare
priyasiddharth
changed the title
priyasiddharth
force-pushed
the
verify_add_used
branch
from
7f83b55 to
f8c3526
Compare
priyasiddharth
requested review from
alexandruag,
andreeaflorescu,
jiangliu,
slp,
stsquad,
epilys and
stefano-garzarella
as code owners
MatiasVara
reviewed
Aug 6, 2025
|
LGTM! |
MatiasVara
reviewed
Aug 6, 2025
MatiasVara
reviewed
Aug 6, 2025
MatiasVara
reviewed
Aug 7, 2025
MatiasVara
reviewed
Aug 7, 2025
priyasiddharth
force-pushed
the
verify_add_used
branch
from
f8c3526 to
433129f
Compare
priyasiddharth
changed the title
priyasiddharth
changed the title
priyasiddharth
force-pushed
the
verify_add_used
branch
3 times, most recently
from
7f83b55 to
6451a84
Compare
Kani proofs do not finish in practical time if we use the production memory region. So we implement a stub region with a simple vector backing it. This will help subsequent proofs work and also enable stateful proofs. Signed-off-by: Siddharth Priya <[email protected]>
priyasiddharth
force-pushed
the
verify_add_used
branch
from
6451a84 to
a0bdc34
Compare
There was a problem hiding this comment.
minor comments from my side. LGTM but I'd like an approval from @MatiasVara and @roypat
and @epilys |
Signed-off-by: Siddharth Priya <[email protected]>
Signed-off-by: Siddharth Priya <[email protected]>
priyasiddharth
force-pushed
the
verify_add_used
branch
from
a0bdc34 to
2bda53f
Compare
| @@ -458,19 +458,22 @@ impl kani::Arbitrary for ProofContext { | |||
| } | |||
| } | |||
|
|
|||
| /// # Specification (VirtIO 1.3, Section 2.7.7.2: "Device-to-driver notification suppression") | |||
There was a problem hiding this comment.
In virtio 1.3 (https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-510007) the section 2.7.7.2 is "2.7.7.2 Device Requirements: Used Buffer Notification Suppression", so I guess here we should update the title. (Is the 2.7.7.2 correct?)
epilys
reviewed
Aug 28, 2025
Comment on lines
+13
to
+17
| pub struct StubRegion { | ||
| buffer: *mut u8, | ||
| region_len: usize, | ||
| region_start: GuestAddress, | ||
| } |
There was a problem hiding this comment.
Nitpick: does this need to be pub?
| // between guest addresses and memory region addresses, and to check offsets | ||
| // within the region. | ||
| impl StubRegion { | ||
| pub fn new(buf_ptr: *mut u8, buf_len: usize, start_offset: u64) -> Self { |
Comment on lines
+513
to
+514
| /// Reads the idx field from the used ring in guest memory, as stored by Queue::add_used. | ||
| /// Returns the value as u16. |
There was a problem hiding this comment.
Nitpick:
Suggested change
| /// Reads the idx field from the used ring in guest memory, as stored by Queue::add_used. | |
| /// Returns the value as u16. | |
| /// Returns the `idx` field from the used ring in guest memory, as stored by [`Queue::add_used`]. |
| // On failure, we expect the next_used to remain unchanged | ||
| // and the used_desc_table_index to be out of bounds. | ||
| assert_eq!(queue.next_used, used_idx); | ||
| assert!(used_desc_table_index >= queue.size()); |
There was a problem hiding this comment.
Suggestion (not a demand)
Suggested change
| assert!(used_desc_table_index >= queue.size()); | |
| assert!(used_desc_table_index >= queue.size(), "{used_desc_table_index:?}, {:?}", queue.size()); |
Comment on lines
+350
to
+354
| let size = GUEST_MEMORY_SIZE; | ||
| let start = GUEST_MEMORY_BASE; | ||
| Self { | ||
| the_region: StubRegion::new(memory, size, start), | ||
| } |
There was a problem hiding this comment.
Suggested change
| let size = GUEST_MEMORY_SIZE; | |
| let start = GUEST_MEMORY_BASE; | |
| Self { | |
| the_region: StubRegion::new(memory, size, start), | |
| } | |
| Self { | |
| the_region: StubRegion::new(memory, GUEST_MEMORY_SIZE, GUEST_MEMORY_BASE), | |
| } |
Comment on lines
+274
to
+282
| assert!(region | ||
| .write(&bytes, MemoryRegionAddress(write_offset as u64)) | ||
| .is_ok()); | ||
|
|
||
| // Read back into a new buffer | ||
| let mut readback = kani::vec::exact_vec::<u8, 16>(); | ||
| assert!(region | ||
| .read(&mut readback, MemoryRegionAddress(write_offset as u64)) | ||
| .is_ok()); |
There was a problem hiding this comment.
Shouldn't we unwrap errors instead? Otherwise the error message is not shown.
| dst: &mut F, | ||
| count: usize, | ||
| ) -> Result<(), Self::E> { | ||
| self.write_to(addr, dst, count).map(|_| ()) |
There was a problem hiding this comment.
Suggested change
| self.write_to(addr, dst, count).map(|_| ()) | |
| self.write_to(addr, dst, count)?; | |
| Ok(()) |
| unsafe { | ||
| std::ptr::copy_nonoverlapping( | ||
| self.buffer.add(offset), | ||
| (&mut result as *mut T) as *mut u8, |
There was a problem hiding this comment.
Use ByteValued method:
Suggested change
| (&mut result as *mut T) as *mut u8, | |
| result.as_mut_slice().as_mut_ptr(), |
| } | ||
|
|
||
| fn read_slice(&self, buf: &mut [u8], addr: MemoryRegionAddress) -> Result<(), Self::E> { | ||
| self.read(buf, addr).map(|_| ()) |
There was a problem hiding this comment.
Suggested change
| self.read(buf, addr).map(|_| ()) | |
| self.read(buf, addr)?; | |
| Ok(()) |
| } | ||
|
|
||
| fn write_slice(&self, buf: &[u8], addr: MemoryRegionAddress) -> Result<(), Self::E> { | ||
| self.write(buf, addr).map(|_| ()) |
There was a problem hiding this comment.
Suggested change
| self.write(buf, addr).map(|_| ()) | |
| self.write(buf, addr)?; | |
| Ok(()) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary of the PR
virtio-queue: add verification for
add_usedoperationKani proofs like
verify_add_useddo not finish in practical time if we use the production memory region. So we implement a stub region with a simple vector backing it. This will help subsequent proofs work and also enable stateful proofs.Note that
unsafecode is added only for StubRegion that will run in Kani.Kani verifies all
unsafeaccesses do not cause undefined behaviour (in the context of unit proof execution).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.