Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
[workspace]
members = [
"crates/virtio-queue",
"crates/virtio-queue-ser",
"crates/virtio-device",
"crates/devices/*",
]
Expand Down
17 changes: 17 additions & 0 deletions crates/virtio-queue-ser/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
[package]
name = "virtio-queue-ser"
version = "0.1.0"
authors = ["rust-vmm AWS maintainers <[email protected]>"]
description = "Serialization for virtio queue state"
repository = "https://github.com/rust-vmm/vm-virtio"
keywords = ["queue", "serialization", "versioning"]
readme = "README.md"
license = "Apache-2.0 OR BSD-3-Clause"
edition = "2018"

[dependencies]
serde = { version = ">=1.0.27", features = ["derive"] }
versionize = ">=0.1.6"
versionize_derive = ">=0.1.3"
virtio-queue = { path = "../../crates/virtio-queue" }
vm-memory = "0.7.0"
14 changes: 14 additions & 0 deletions crates/virtio-queue-ser/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
//
// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause

//! Adds serialization capabilities to the state objects from `virtio-queue`.
//!
//! Provides wrappers over the state objects from `virtio-queue` crate that
//! implement the `Serialize`, `Deserialize` and `Versionize` traits.

#![deny(missing_docs)]

mod state;

pub use state::QueueStateSer;
126 changes: 126 additions & 0 deletions crates/virtio-queue-ser/src/state.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
// 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::QueueState;
use vm_memory::GuestAddress;

/// Wrapper over a `QueueState` that has serialization capabilities.
#[derive(Clone, Debug, Deserialize, PartialEq, Serialize, Versionize)]
pub struct QueueStateSer {
/// 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 last used value when using VIRTIO_F_EVENT_IDX.
pub signalled_used: Option<u16>,
/// The queue size in elements the driver selected.
pub size: u16,
/// Indicates if the queue finished with the 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,
}

// The following `From` implementations can be used to convert from a `QueueStateSer` to the
// `QueueState` from the base crate and vice versa.
// WARNING: They don't check for any invalid data, that would otherwise be checked when initializing
// a queue object from scratch (for example setting a queue size greater than its max possible
// size). The problem with the current `QueueState` implementation is that it can be used as the
// queue object itself. `QueueState`'s fields are public, which allows the user to set up and use an
// invalid queue. Once we fix https://github.com/rust-vmm/vm-virtio/issues/143, `QueueState` will be
// renamed to `Queue`, its fields will no longer be public, and `QueueState` will consist of the
// actual state. This way we can also check against any invalid data when trying to get a `Queue`
// from the state object.
// 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 QueueState {
fn from(state: &QueueStateSer) -> Self {
QueueState {
max_size: state.max_size,
next_avail: Wrapping(state.next_avail),
next_used: Wrapping(state.next_used),
event_idx_enabled: state.event_idx_enabled,
signalled_used: state.signalled_used.map(Wrapping),
size: state.size,
ready: state.ready,
desc_table: GuestAddress(state.desc_table),
avail_ring: GuestAddress(state.avail_ring),
used_ring: GuestAddress(state.used_ring),
}
}
}

impl From<&QueueState> for QueueStateSer {
fn from(state: &QueueState) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is anything that we need to check here. Is there any invalid state that would otherwise not be able to get into when just initializing a Queue from scratch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When initializing a Queue from scratch, there are some checks that are done on the queue size, on the addresses of the queue components (in their corresponding setters). So yes, the state here can be invalid if the user is initializing it with random values.
This is actually a problem at the QueueState level and related to all that confusion around the queue interfaces. The QueueStates fields are public, which is okay for a state object, but since the QueueState is actually the Queue, the fact that its fields are public is no longer good.
I guess for now I can bring the logic from those setters here as well, but that won't fix the entire problem, and is not a clean fix either. I think the clean fix would be to have the QueueState -> Queue interface update (and not having the queue fields public), and when trying to get a Queue from a QueueState, do all of the checks from those setters. That way, the code in this file wouldn't require any checks.

Copy link

@acatangiu acatangiu Mar 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why just wonder and not reason about it?

For example, my thinking is even if some of the fields could be sanity checked (size <= max_size for example), I suggest deferring validations to when the actual Queue is built, since without GuestMemory most of the state doesn't make any sense.

Once you have the Queue, you can rightly call its is_valid() method to make sure all is kosher.

I too am lazy to point out exactly what extra checks is_valid() would need when also considering deserialization, but once you get there you can look at Firecracker impl to get an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why just wonder and not reason about it?

For example, my thinking is even if some of the fields could be sanity checked (size <= max_size for example), I suggest deferring validations to when the actual Queue is built, since without GuestMemory most of the state doesn't make any sense.

That is sort of my suggestion as well. The thing here is the way the queue interfaces are right now, you can more easily use QueueState as the Queue object. The queue object doesn't need to have a memory handle in its composition. This is basically one of the things we want to update, and after that the where should I check stuff discussion would be much easier to have.

Then you can rightly call its is_valid() method to make sure all is kosher.

I too am lazy to point out exactly what extra checks is_valid() would need when also considering deserialization, but once you get there you can look at Firecracker impl to get an idea.

In virtio-queue, is_valid is no longer doing all those checks that are done in Firecracker. The checks that are part of the spec were moved to the corresponding setters of those fields, because as opposed to is_valid, which is optional for the user to call, the setters are not (when starting from scratch). But yes, this is the way to do the validations after deserialization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how I imagine the conversion would look like after the interface change that was mentioned:

impl Queue {
    pub fn from_state(state: &QueueState) -> Self {
        let mut queue = Queue {
            max_size: state.max_size,
            next_avail: state.next_avail,
            next_used: state.next_used,
            event_idx_enabled: state.event_idx_enabled,
            signalled_used: state.signalled_used,
            size: state.max_size,
            ready: state.ready,
            desc_table: GuestAddress(DEFAULT_DESC_TABLE_ADDR),
            avail_ring: GuestAddress(DEFAULT_AVAIL_RING_ADDR),
            used_ring: GuestAddress(DEFAULT_USED_RING_ADDR),
        };
        queue.set_size(state.size);
        queue.set_desc_table_address(..);
         ...
        queue
    }
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm! But as a heads-up, you might have an easier time if you created a QueueBuilder object where you'd have fn with_x(self, x) -> Self instead of setters you mention above.
The advantage is that ultimately you have fn build(self) -> Result<Queue, Error<Self>> that can do more complex validations that cannot be done in simple setters.

Validations depending on X and Y cannot be done in set_x or set_y (for example this check can't be done in a setter, but can be done in qbuilder.build() or queue.is_valid()).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm! But as a heads-up, you might have an easier time if you created a QueueBuilder object where you'd have fn with_x(self, x) -> Self instead of setters you mention above. The advantage is that ultimately you have fn build(self) -> Result<Queue, Error<Self>> that can do more complex validations that cannot be done in simple setters.

We thought about using the builder pattern before 0.1.0, but it's a bit incompatible with how the queue is constructed when using for example MMIO writes (and there won't be any build() in this case; calling build() will still be optional).

Validations depending on X and Y cannot be done in set_x or set_y (for example this check can't be done in a setter, but can be done in qbuilder.build() or queue.is_valid()).

Not all de validations are supposed to happen in the setters, but what we are aiming with this is not having the MUSTs from the spec as optional.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all de validations are supposed to happen in the setters, but what we are aiming with this is not having the MUSTs from the spec as optional.

Sure! Validating in the setters against the spec is great!
I'm just saying as a heads-up that, on the slightly orthogonal subject of deserialization, you will need some extra validations that you can't do in setters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally dislike the pattern with is_valid. The callers of the interface should not know that they have to call is_valid. But this should be part of a bigger refactoring effort.

If you're using the current interface correctly (i.e. you don't set arbitrary values to the queue fields, but just use the setters), then the Queue that you are getting is a valid one. From my point of view, this is something that we need to check when initializing from a saved state as well. We can split the checks in 2 categories:

  • what can malicious actors do
  • what can happen because of programming errors

Right now with the current interface is really hard to guard against programming errors because the interface is too open (i.e. with the public fields). Nevertheless, we should have checks for things that can be initialized by malicious actors. As per the threat model the deserialized state is not trusted because we cannot know where it originates from. If we cannot address this in this PR it's fine, but we should know about this shortcoming and at least document it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some more clarifications in code comments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is anything that we need to check here. Is there any invalid state that would otherwise not be able to get into when just initializing a Queue from scratch?

QueueStateSer {
max_size: state.max_size,
next_avail: state.next_avail.0,
next_used: state.next_used.0,
event_idx_enabled: state.event_idx_enabled,
signalled_used: state.signalled_used.map(|value| value.0),
size: state.size,
ready: state.ready,
desc_table: state.desc_table.0,
avail_ring: state.avail_ring.0,
used_ring: state.used_ring.0,
}
}
}

impl Default for QueueStateSer {
fn default() -> Self {
QueueStateSer::from(&QueueState::default())
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_state_ser() {
const SOME_VALUE: u16 = 16;

let state = QueueState {
max_size: SOME_VALUE * 2,
next_avail: Wrapping(SOME_VALUE - 1),
next_used: Wrapping(SOME_VALUE + 1),
event_idx_enabled: false,
signalled_used: None,
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),
};

let ser_state = QueueStateSer::from(&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!(
QueueState::from(&default_queue_state_ser),
QueueState::default()
);
}
}
10 changes: 9 additions & 1 deletion crates/virtio-queue/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,15 @@ use crate::defs::{
use crate::{error, AvailIter, Descriptor, Error, QueueStateGuard, QueueStateT, VirtqUsedElem};

/// Struct to maintain information and manipulate state of a virtio queue.
#[derive(Debug)]
///
/// WARNING: The way the `QueueState` is defined now, it can be used as the queue object, therefore
/// it is allowed to set up and use an invalid queue (initialized with random data since the
/// `QueueState`'s fields are public). When fixing https://github.com/rust-vmm/vm-virtio/issues/143,
/// we plan to rename `QueueState` to `Queue`, and define a new `QueueState` that would be 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 QueueState {
/// The maximum size in elements offered by the device.
pub max_size: u16,
Expand Down