add QueueState & mark queue fields as private

To make sure that the queue cannot be misused, we are only allowing
alteration of the Queue by using the setters & other methods that are
part of the public interface.

To be able to save & restore the state of the Queue, we are introducing
a `QueueState` structure. This structure represents the pure state of
the Queue in the sense that it does not hold any information about the
implementation of the queue. If the VMM wants this state to be
serializable/deserializable, the structure needs to be duplicated in
the VMM code and the appropriate ser/deser traits need to be implemented.
Alternatively, VMMs can use directly the virtio-queue-ser crate that
uses serde & versionize for this purpose.

Signed-off-by: Andreea Florescu <[email protected]>

Author: andreeaflorescu

coverage_config_x86_64.json

@@ -1,5 +1,5 @@
 {
-  "coverage_score": 85.5,
+  "coverage_score": 87.2,
   "exclude_path": "crates/virtio-bindings,crates/virtio-queue/src/mock.rs",
   "crate_features": "virtio-blk/backend-stdio"
 }

crates/virtio-queue-ser/src/state.rs

@@ -1,14 +1,10 @@
 // Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 //
 // SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause
-
-use std::num::Wrapping;
-
 use serde::{Deserialize, Serialize};
 use versionize::{VersionMap, Versionize, VersionizeResult};
 use versionize_derive::Versionize;
-use virtio_queue::Queue;
-use vm_memory::GuestAddress;
+use virtio_queue::QueueState;
 
 /// Wrapper over a `QueueState` that has serialization capabilities.
 #[derive(Clone, Debug, Deserialize, PartialEq, Serialize, Versionize)]
@@ -46,78 +42,79 @@ pub struct QueueStateSer {
 // Nevertheless, we don't make any assumptions in the virtio-queue code about the queue's state that
 // would otherwise result in a panic, when initialized with random data, so from this point of view
 // these conversions are safe to use.
-impl From<&QueueStateSer> for Queue {
+impl From<&QueueStateSer> for QueueState {
     fn from(state: &QueueStateSer) -> Self {
-        Queue {
+        QueueState {
             max_size: state.max_size,
-            next_avail: Wrapping(state.next_avail),
-            next_used: Wrapping(state.next_used),
+            next_avail: state.next_avail,
+            next_used: state.next_used,
             event_idx_enabled: state.event_idx_enabled,
-            num_added: Wrapping(0),
             size: state.size,
             ready: state.ready,
-            desc_table: GuestAddress(state.desc_table),
-            avail_ring: GuestAddress(state.avail_ring),
-            used_ring: GuestAddress(state.used_ring),
+            desc_table: state.desc_table,
+            avail_ring: state.avail_ring,
+            used_ring: state.used_ring,
         }
     }
 }
 
-impl From<&Queue> for QueueStateSer {
-    fn from(state: &Queue) -> Self {
+impl From<&QueueState> for QueueStateSer {
+    fn from(state: &QueueState) -> Self {
         QueueStateSer {
             max_size: state.max_size,
-            next_avail: state.next_avail.0,
-            next_used: state.next_used.0,
+            next_avail: state.next_avail,
+            next_used: state.next_used,
             event_idx_enabled: state.event_idx_enabled,
             size: state.size,
             ready: state.ready,
-            desc_table: state.desc_table.0,
-            avail_ring: state.avail_ring.0,
-            used_ring: state.used_ring.0,
+            desc_table: state.desc_table,
+            avail_ring: state.avail_ring,
+            used_ring: state.used_ring,
         }
     }
 }
 
 impl Default for QueueStateSer {
     fn default() -> Self {
-        QueueStateSer::from(&Queue::default())
+        QueueStateSer::from(&QueueState::default())
     }
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
-    use virtio_queue::{mock::MockSplitQueue, Descriptor, QueueT};
-    use vm_memory::GuestMemoryMmap;
+    use std::convert::TryFrom;
+    use virtio_queue::{Error, Queue};
 
     #[test]
     fn test_state_ser() {
         const SOME_VALUE: u16 = 16;
 
-        let state = Queue {
+        let state = QueueState {
             max_size: SOME_VALUE * 2,
-            next_avail: Wrapping(SOME_VALUE - 1),
-            next_used: Wrapping(SOME_VALUE + 1),
+            next_avail: SOME_VALUE - 1,
+            next_used: SOME_VALUE + 1,
             event_idx_enabled: false,
-            num_added: Wrapping(0),
             size: SOME_VALUE,
             ready: true,
-            desc_table: GuestAddress(SOME_VALUE as u64),
-            avail_ring: GuestAddress(SOME_VALUE as u64 * 2),
-            used_ring: GuestAddress(SOME_VALUE as u64 * 4),
+            desc_table: SOME_VALUE as u64,
+            avail_ring: SOME_VALUE as u64 * 2,
+            used_ring: SOME_VALUE as u64 * 4,
         };
 
         let ser_state = QueueStateSer::from(&state);
-        let state_from_ser = Queue::from(&ser_state);
+        let state_from_ser = QueueState::from(&ser_state);
 
         // Check that the old and the new state are identical when using the intermediate
         // `QueueStateSer` object.
         assert_eq!(state, state_from_ser);
 
         // Test the `Default` implementation of `QueueStateSer`.
         let default_queue_state_ser = QueueStateSer::default();
-        assert_eq!(Queue::from(&default_queue_state_ser), Queue::default());
+        assert_eq!(
+            QueueState::from(&default_queue_state_ser),
+            QueueState::default()
+        );
     }
 
     #[test]
@@ -129,8 +126,6 @@ mod tests {
         // public, and the way to obtain a Queue from a serialized Queue is by using a `try_from`
         // function which then makes sure that the deserialized values are valid before creating
         // a queue that might be invalid.
-        let m = &GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0), 0x10000)]).unwrap();
-        let vq = MockSplitQueue::new(m, 16);
         let queue_ser = QueueStateSer {
             max_size: 16,
             next_avail: 0,
@@ -143,9 +138,8 @@ mod tests {
             used_ring: 276,
         };
 
-        let mut queue = Queue::from(&queue_ser);
-        let desc_chain = vec![Descriptor::new(0x0, 0x100, 0, 0)];
-        vq.build_desc_chain(&desc_chain).unwrap();
-        assert!(queue.pop_descriptor_chain(m).is_none());
+        let queue_state = QueueState::from(&queue_ser);
+        let err = Queue::try_from(queue_state).unwrap_err();
+        assert_eq!(err.to_string(), Error::InvalidSize.to_string());
     }
 }

crates/virtio-queue/src/lib.rs

@@ -26,6 +26,7 @@ pub use self::chain::{DescriptorChain, DescriptorChainRwIter};
 pub use self::descriptor::{Descriptor, VirtqUsedElem};
 pub use self::queue::{AvailIter, Queue};
 pub use self::queue_sync::QueueSync;
+pub use self::state::QueueState;
 
 pub mod defs;
 #[cfg(any(test, feature = "test-utils"))]
@@ -35,6 +36,7 @@ mod chain;
 mod descriptor;
 mod queue;
 mod queue_sync;
+mod state;
 
 /// Virtio Queue related errors.
 #[derive(Debug)]

crates/virtio-queue/src/queue.rs

@@ -20,7 +20,8 @@ use crate::defs::{
     VIRTQ_USED_ELEMENT_SIZE, VIRTQ_USED_RING_HEADER_SIZE, VIRTQ_USED_RING_META_SIZE,
 };
 use crate::{
-    error, Descriptor, DescriptorChain, Error, QueueGuard, QueueOwnedT, QueueT, VirtqUsedElem,
+    error, Descriptor, DescriptorChain, Error, QueueGuard, QueueOwnedT, QueueState, QueueT,
+    VirtqUsedElem,
 };
 use virtio_bindings::bindings::virtio_ring::VRING_USED_F_NO_NOTIFY;
 
@@ -75,45 +76,38 @@ pub const MAX_QUEUE_SIZE: u16 = 32768;
 /// // The queue should not be ready after reset.
 /// assert!(!queue.ready());
 /// ```
-///
-/// WARNING: The current implementation allows setting up and using an invalid queue
-/// (initialized with random data since the `Queue`'s fields are public). When fixing
-/// <https://github.com/rust-vmm/vm-virtio/issues/172>, we plan to define a `QueueState` that
-/// represents the actual state of the queue (no `Wrapping`s in it, for example). This way, we
-/// will also be able to do the checks that we normally do in the queue's field setters when
-/// starting from scratch, when trying to create a `Queue` from a `QueueState`.
 #[derive(Debug, Default, PartialEq)]
 pub struct Queue {
     /// The maximum size in elements offered by the device.
-    pub max_size: u16,
+    max_size: u16,
 
     /// Tail position of the available ring.
-    pub next_avail: Wrapping<u16>,
+    next_avail: Wrapping<u16>,
 
     /// Head position of the used ring.
-    pub next_used: Wrapping<u16>,
+    next_used: Wrapping<u16>,
 
     /// VIRTIO_F_RING_EVENT_IDX negotiated.
-    pub event_idx_enabled: bool,
+    event_idx_enabled: bool,
 
     /// The number of descriptor chains placed in the used ring via `add_used`
     /// since the last time `needs_notification` was called on the associated queue.
-    pub num_added: Wrapping<u16>,
+    num_added: Wrapping<u16>,
 
     /// The queue size in elements the driver selected.
-    pub size: u16,
+    size: u16,
 
     /// Indicates if the queue is finished with configuration.
-    pub ready: bool,
+    ready: bool,
 
     /// Guest physical address of the descriptor table.
-    pub desc_table: GuestAddress,
+    desc_table: GuestAddress,
 
     /// Guest physical address of the available ring.
-    pub avail_ring: GuestAddress,
+    avail_ring: GuestAddress,
 
     /// Guest physical address of the used ring.
-    pub used_ring: GuestAddress,
+    used_ring: GuestAddress,
 }
 
 impl Queue {
@@ -172,6 +166,29 @@ impl Queue {
         Ok(())
     }
 
+    /// Returns the state of the `Queue`.
+    ///
+    /// This is useful for implementing save/restore capabilities.
+    /// The state does not have support for serialization, but this can be
+    /// added by VMMs locally through the use of a
+    /// [remote type](https://serde.rs/remote-derive.html).
+    ///
+    /// Alternatively, a version aware and serializable/deserializable QueueState
+    /// is available in the `virtio-queue-ser` crate.
+    pub fn state(&self) -> QueueState {
+        QueueState {
+            max_size: self.max_size,
+            next_avail: self.next_avail(),
+            next_used: self.next_used(),
+            event_idx_enabled: self.event_idx_enabled,
+            size: self.size,
+            ready: self.ready,
+            desc_table: self.desc_table(),
+            avail_ring: self.avail_ring(),
+            used_ring: self.used_ring(),
+        }
+    }
+
     // Helper method that writes `val` to the `avail_event` field of the used ring, using
     // the provided ordering.
     fn set_avail_event<M: GuestMemory>(

crates/virtio-queue/src/state.rs

@@ -0,0 +1,120 @@
+use crate::{Error, Queue, QueueT};
+use std::convert::TryFrom;
+use vm_memory::GuestAddress;
+
+/// Representation of the `Queue` state.
+///
+/// The `QueueState` represents the pure state of the `queue` without tracking any implementation
+/// details of the queue. The goal with this design is to minimize the changes required to the
+/// state, and thus the required transitions between states when upgrading or downgrading.
+///
+/// In practice this means that the `QueueState` consists solely of POD (Plain Old Data).
+///
+/// As this structure has all the fields public it is consider to be untrusted. A validated
+/// queue can be created from the state by calling the associated `try_from` function.
+#[derive(Clone, Copy, Debug, Default, PartialEq)]
+pub struct QueueState {
+    /// The maximum size in elements offered by the device.
+    pub max_size: u16,
+    /// Tail position of the available ring.
+    pub next_avail: u16,
+    /// Head position of the used ring.
+    pub next_used: u16,
+    /// VIRTIO_F_RING_EVENT_IDX negotiated.
+    pub event_idx_enabled: bool,
+    /// The queue size in elements the driver selected.
+    pub size: u16,
+    /// Indicates if the queue is finished with configuration.
+    pub ready: bool,
+    /// Guest physical address of the descriptor table.
+    pub desc_table: u64,
+    /// Guest physical address of the available ring.
+    pub avail_ring: u64,
+    /// Guest physical address of the used ring.
+    pub used_ring: u64,
+}
+
+impl TryFrom<QueueState> for Queue {
+    type Error = Error;
+
+    fn try_from(q_state: QueueState) -> Result<Self, Self::Error> {
+        let mut q = Queue::new(q_state.max_size)?;
+
+        q.set_next_avail(q_state.next_avail);
+        q.set_next_used(q_state.next_used);
+        q.set_event_idx(q_state.event_idx_enabled);
+        q.try_set_size(q_state.size)?;
+        q.set_ready(q_state.ready);
+        q.try_set_desc_table_address(GuestAddress(q_state.desc_table))?;
+        q.try_set_avail_ring_address(GuestAddress(q_state.avail_ring))?;
+        q.try_set_used_ring_address(GuestAddress(q_state.used_ring))?;
+
+        Ok(q)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn create_valid_queue_state() -> QueueState {
+        let queue = Queue::new(16).unwrap();
+        queue.state()
+    }
+
+    #[test]
+    fn test_empty_queue_state() {
+        let max_size = 16;
+        let queue = Queue::new(max_size).unwrap();
+
+        // Saving the state of a queue on which we didn't do any operation is ok.
+        // Same for restore.
+        let queue_state = queue.state();
+        let restored_q = Queue::try_from(queue_state).unwrap();
+        assert_eq!(queue, restored_q);
+    }
+
+    #[test]
+    fn test_invalid_queue_state() {
+        // Let's generate a state that we know is valid so we can just alter one field at a time.
+        let mut q_state = create_valid_queue_state();
+
+        // Test invalid max_size.
+        // Size too small.
+        q_state.max_size = 0;
+        assert!(Queue::try_from(q_state).is_err());
+        // Size too big.
+        q_state.max_size = u16::MAX;
+        assert!(Queue::try_from(q_state).is_err());
+        // Size not a power of 2.
+        q_state.max_size = 15;
+        assert!(Queue::try_from(q_state).is_err());
+
+        // Test invalid size.
+        let mut q_state = create_valid_queue_state();
+        // Size too small.
+        q_state.size = 0;
+        assert!(Queue::try_from(q_state).is_err());
+        // Size too big.
+        q_state.size = u16::MAX;
+        assert!(Queue::try_from(q_state).is_err());
+        // Size not a power of 2.
+        q_state.size = 15;
+        assert!(Queue::try_from(q_state).is_err());
+
+        // Test invalid desc_table.
+        let mut q_state = create_valid_queue_state();
+        q_state.desc_table = 0xf;
+        assert!(Queue::try_from(q_state).is_err());
+
+        // Test invalid avail_ring.
+        let mut q_state = create_valid_queue_state();
+        q_state.avail_ring = 0x1;
+        assert!(Queue::try_from(q_state).is_err());
+
+        // Test invalid used_ring.
+        let mut q_state = create_valid_queue_state();
+        q_state.used_ring = 0x3;
+        assert!(Queue::try_from(q_state).is_err());
+    }
+}