Skip to content

Commit 6b7dea5

Browse files
committed
refactor: remove actual_size from Queue
The size of queue set by the driver must be always less or equal to the queue size in FC. This is checked before device activation. This removes the need for `actual_size` function. Signed-off-by: Egor Lazarchuk <[email protected]>
1 parent ec27e83 commit 6b7dea5

File tree

2 files changed

+23
-33
lines changed

2 files changed

+23
-33
lines changed

src/vmm/src/devices/virtio/queue.rs

Lines changed: 21 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
// Use of this source code is governed by a BSD-style license that can be
66
// found in the THIRD-PARTY file.
77

8-
use std::cmp::min;
98
use std::num::Wrapping;
109
use std::sync::atomic::{Ordering, fence};
1110

@@ -448,12 +447,6 @@ impl Queue {
448447
}
449448
}
450449

451-
/// Return the actual size of the queue, as the driver may not set up a
452-
/// queue as big as the device allows.
453-
pub fn actual_size(&self) -> u16 {
454-
min(self.size, self.max_size)
455-
}
456-
457450
/// Validates the queue's in-memory layout is correct.
458451
pub fn is_valid<M: GuestMemory>(&self, mem: &M) -> bool {
459452
let desc_table = self.desc_table_address;
@@ -525,13 +518,13 @@ impl Queue {
525518
// once. Checking and reporting such incorrect driver behavior
526519
// can prevent potential hanging and Denial-of-Service from
527520
// happening on the VMM side.
528-
if len > self.actual_size() {
521+
if self.size < len {
529522
// We are choosing to interrupt execution since this could be a potential malicious
530523
// driver scenario. This way we also eliminate the risk of repeatedly
531524
// logging and potentially clogging the microVM through the log system.
532525
panic!(
533526
"The number of available virtio descriptors {len} is greater than queue size: {}!",
534-
self.actual_size()
527+
self.size
535528
);
536529
}
537530

@@ -571,17 +564,15 @@ impl Queue {
571564
//
572565
// We use `self.next_avail` to store the position, in `ring`, of the next available
573566
// descriptor index, with a twist: we always only increment `self.next_avail`, so the
574-
// actual position will be `self.next_avail % self.actual_size()`.
575-
let idx = self.next_avail.0 % self.actual_size();
567+
// actual position will be `self.next_avail % self.size`.
568+
let idx = self.next_avail.0 % self.size;
576569
// SAFETY:
577570
// index is bound by the queue size
578571
let desc_index = unsafe { self.avail_ring_ring_get(usize::from(idx)) };
579572

580-
DescriptorChain::checked_new(self.desc_table_ptr, self.actual_size(), desc_index).inspect(
581-
|_| {
582-
self.next_avail += Wrapping(1);
583-
},
584-
)
573+
DescriptorChain::checked_new(self.desc_table_ptr, self.size, desc_index).inspect(|_| {
574+
self.next_avail += Wrapping(1);
575+
})
585576
}
586577

587578
/// Undo the effects of the last `self.pop()` call.
@@ -599,15 +590,15 @@ impl Queue {
599590
desc_index: u16,
600591
len: u32,
601592
) -> Result<(), QueueError> {
602-
if self.actual_size() <= desc_index {
593+
if self.size <= desc_index {
603594
error!(
604595
"attempted to add out of bounds descriptor to used ring: {}",
605596
desc_index
606597
);
607598
return Err(QueueError::DescIndexOutOfBounds(desc_index));
608599
}
609600

610-
let next_used = (self.next_used + Wrapping(ring_index_offset)).0 % self.actual_size();
601+
let next_used = (self.next_used + Wrapping(ring_index_offset)).0 % self.size;
611602
let used_element = UsedElement {
612603
id: u32::from(desc_index),
613604
len,
@@ -653,15 +644,15 @@ impl Queue {
653644
if len != 0 {
654645
// The number of descriptor chain heads to process should always
655646
// be smaller or equal to the queue size.
656-
if len > self.actual_size() {
647+
if len > self.size {
657648
// We are choosing to interrupt execution since this could be a potential malicious
658649
// driver scenario. This way we also eliminate the risk of
659650
// repeatedly logging and potentially clogging the microVM through
660651
// the log system.
661652
panic!(
662653
"The number of available virtio descriptors {len} is greater than queue size: \
663654
{}!",
664-
self.actual_size()
655+
self.size
665656
);
666657
}
667658
return false;
@@ -1060,7 +1051,7 @@ mod verification {
10601051
// done. This is relying on implementation details of add_used, namely that
10611052
// the check for out-of-bounds descriptor index happens at the very beginning of the
10621053
// function.
1063-
assert!(used_desc_table_index >= queue.actual_size());
1054+
assert!(used_desc_table_index >= queue.size);
10641055
}
10651056
}
10661057

@@ -1097,11 +1088,11 @@ mod verification {
10971088

10981089
#[kani::proof]
10991090
#[kani::unwind(0)]
1100-
fn verify_actual_size() {
1091+
fn verify_size() {
11011092
let ProofContext(queue, _) = kani::any();
11021093

1103-
assert!(queue.actual_size() <= queue.max_size);
1104-
assert!(queue.actual_size() <= queue.size);
1094+
assert!(queue.size <= queue.max_size);
1095+
assert!(queue.size <= queue.size);
11051096
}
11061097

11071098
#[kani::proof]
@@ -1166,7 +1157,7 @@ mod verification {
11661157
// is called when the queue is being initialized, e.g. empty. We compute it using
11671158
// local variables here to make things easier on kani: One less roundtrip through vm-memory.
11681159
let queue_len = queue.len();
1169-
kani::assume(queue_len <= queue.actual_size());
1160+
kani::assume(queue_len <= queue.size);
11701161

11711162
let next_avail = queue.next_avail;
11721163

@@ -1184,7 +1175,7 @@ mod verification {
11841175
let ProofContext(mut queue, _) = kani::any();
11851176

11861177
// See verify_pop for explanation
1187-
kani::assume(queue.len() <= queue.actual_size());
1178+
kani::assume(queue.len() <= queue.size);
11881179

11891180
let queue_clone = queue.clone();
11901181
if let Some(_) = queue.pop() {
@@ -1200,7 +1191,7 @@ mod verification {
12001191
fn verify_try_enable_notification() {
12011192
let ProofContext(mut queue, _) = ProofContext::bounded_queue();
12021193

1203-
kani::assume(queue.len() <= queue.actual_size());
1194+
kani::assume(queue.len() <= queue.size);
12041195

12051196
if queue.try_enable_notification() && queue.uses_notif_suppression {
12061197
// We only require new notifications if the queue is empty (e.g. we've processed
@@ -1218,10 +1209,9 @@ mod verification {
12181209
let ProofContext(queue, mem) = kani::any();
12191210

12201211
let index = kani::any();
1221-
let maybe_chain =
1222-
DescriptorChain::checked_new(queue.desc_table_ptr, queue.actual_size(), index);
1212+
let maybe_chain = DescriptorChain::checked_new(queue.desc_table_ptr, queue.size, index);
12231213

1224-
if index >= queue.actual_size() {
1214+
if index >= queue.size {
12251215
assert!(maybe_chain.is_none())
12261216
} else {
12271217
// If the index was in-bounds for the descriptor table, we at least should be
@@ -1236,7 +1226,7 @@ mod verification {
12361226
match maybe_chain {
12371227
None => {
12381228
// This assert is the negation of the "is_valid" check in checked_new
1239-
assert!(desc.flags & VIRTQ_DESC_F_NEXT == 1 && desc.next >= queue.actual_size())
1229+
assert!(desc.flags & VIRTQ_DESC_F_NEXT == 1 && desc.next >= queue.size)
12401230
}
12411231
Some(head) => {
12421232
assert!(head.is_valid())

src/vmm/src/devices/virtio/vhost_user.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -410,14 +410,14 @@ impl<T: VhostUserHandleBackend> VhostUserHandleImpl<T> {
410410
// at early stage.
411411
for (queue_index, queue, _) in queues.iter() {
412412
self.vu
413-
.set_vring_num(*queue_index, queue.actual_size())
413+
.set_vring_num(*queue_index, queue.size)
414414
.map_err(VhostUserError::VhostUserSetVringNum)?;
415415
}
416416

417417
for (queue_index, queue, queue_evt) in queues.iter() {
418418
let config_data = VringConfigData {
419419
queue_max_size: queue.max_size,
420-
queue_size: queue.actual_size(),
420+
queue_size: queue.size,
421421
flags: 0u32,
422422
desc_table_addr: mem
423423
.get_host_address(queue.desc_table_address)

0 commit comments

Comments
 (0)