Repository: firecracker-microvm/firecracker
File: src/vmm/src/devices/virtio/iovec.rs (Rust)

Code before:
#[cfg(kani)]
type IoVecVec = Vec<iovec>;
#[cfg(not(kani))]
type IoVecVec = SmallVec<[iovec; 4]>;

/// This is essentially a wrapper of a `Vec<libc::iovec>` which can be passed to `libc::writev`.
///
/// It describes a buffer passed to us by the guest that is scattered across multiple
/// memory regions. Additionally, this wrapper provides methods that allow reading arbitrary ranges
/// of data from that buffer.
#[derive(Debug, Default)]
pub struct IoVecBuffer {
    // container of the memory regions included in this IO vector
    vecs: IoVecVec,
    // Total length of the IoVecBuffer
    len: u32,
}

// SAFETY: `IoVecBuffer` doesn't allow for interior mutability and no shared ownership is possible
// as it doesn't implement clone
unsafe impl Send for IoVecBuffer {}

impl IoVecBuffer {
    /// Create an `IoVecBuffer` from a `DescriptorChain`
    pub unsafe fn load_descriptor_chain(&mut self, head: DescriptorChain) -> Result<(), IoVecError> {
        self.vecs.clear();
        self.len = 0u32;

        let mut next_descriptor = Some(head);
        while let Some(desc) = next_descriptor {
            if desc.is_write_only() {
                return Err(IoVecError::WriteOnlyDescriptor);
            }

            // We use get_slice instead of `get_host_address` here in order to have the whole
            // range of the descriptor chain checked, i.e. [addr, addr + len) is a valid memory
            // region in the GuestMemoryMmap.
            let iov_base = desc
                .mem
                .get_slice(desc.addr, desc.len as usize)?
                .ptr_guard_mut()
                .as_ptr()
                .cast::<c_void>();
            self.vecs.push(iovec {
                iov_base,
                iov_len: desc.len as size_t,
            });
            self.len = self.len
                .checked_add(desc.len)
                .ok_or(IoVecError::OverflowedDescriptor)?;


Review comment (performance):
I think we really need an additional `pub fn clear(&mut self) { self.vecs.clear() }` function in these iovecbuffer types that call inside `process_tx` at the very end of the function. That's because for safety, we rely on no two buffers pointing towards the same slices of guest memory. We know this will be true because all buffers across all devices are processed sequentially, but we need to make sure that some cached buffer (such as the one in the net device) does not contain "stale" points while we're processing another buffer. Tbh, this is mainly just me being paranoid, I don't think anything bad can happen from it (because the only thing we do with those stale pointers is overwrite them later), but let's be precise here :) 

Code after:
#[cfg(kani)]
type IoVecVec = Vec<iovec>;
#[cfg(not(kani))]
type IoVecVec = SmallVec<[iovec; 4]>;

/// This is essentially a wrapper of a `Vec<libc::iovec>` which can be passed to `libc::writev`.
///
/// It describes a buffer passed to us by the guest that is scattered across multiple
/// memory regions. Additionally, this wrapper provides methods that allow reading arbitrary ranges
/// of data from that buffer.
#[derive(Debug, Default)]
pub struct IoVecBuffer {
    // container of the memory regions included in this IO vector
    vecs: IoVecVec,
    // Total length of the IoVecBuffer
    len: u32,
}

// SAFETY: `IoVecBuffer` doesn't allow for interior mutability and no shared ownership is possible
// as it doesn't implement clone
unsafe impl Send for IoVecBuffer {}

impl IoVecBuffer {
    /// Create an `IoVecBuffer` from a `DescriptorChain`
    ///
    /// # Safety
    ///
    /// The descriptor chain cannot be referencing the same memory location as another chain
    pub unsafe fn load_descriptor_chain(
        &mut self,
        head: DescriptorChain,
    ) -> Result<(), IoVecError> {
        self.clear();

        let mut next_descriptor = Some(head);
        while let Some(desc) = next_descriptor {
            if desc.is_write_only() {
                return Err(IoVecError::WriteOnlyDescriptor);
            }

            // We use get_slice instead of `get_host_address` here in order to have the whole
            // range of the descriptor chain checked, i.e. [addr, addr + len) is a valid memory
            // region in the GuestMemoryMmap.
            let iov_base = desc
                .mem
                .get_slice(desc.addr, desc.len as usize)?
                .ptr_guard_mut()
                .as_ptr()
                .cast::<c_void>();
            self.vecs.push(iovec {
                iov_base,