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

Code before:
        if used_element.id >= u32::from(self.actual_size()) {
            error!(
                "attempted to add out of bounds descriptor to used ring: {}",
                used_element.id
            );
            return Err(QueueError::DescIndexOutOfBounds(used_element.id));
        }

        // Used ring has layout:
        // struct UsedRing {
        //     flags: u16,
        //     idx: u16,
        //     ring: [UsedElement; <queue size>],
        //     avail_event: u16,
        // }
        // We calculate offset into `ring` field.
        let used_ring_offset = std::mem::size_of::<u16>()
            + std::mem::size_of::<u16>()
            + std::mem::size_of::<UsedElement>() * usize::from(index % self.actual_size());
        let used_element_address = self.used_ring.unchecked_add(usize_to_u64(used_ring_offset));

        // SAFETY:
        // `used_element_address` param is bounded by size of the queue as `index` is
        // modded by `actual_size()`.
        mem.write_obj(used_element, used_element_address).unwrap();
        Ok(())
    }

    /// Fetch the available ring index (`virtq_avail->idx`) from guest memory.
    /// This is written by the driver, to indicate the next slot that will be filled in the avail
    /// ring.
    pub fn avail_idx<M: GuestMemory>(&self, mem: &M) -> Wrapping<u16> {
        // Bound checks for queue inner data have already been performed, at device activation time,
        // via `self.is_valid()`, so it's safe to unwrap and use unchecked offsets here.
        // Note: the `MmioTransport` code ensures that queue addresses cannot be changed by the
        // guest       after device activation, so we can be certain that no change has
        // occurred since the last `self.is_valid()` check.
        let addr = self.avail_ring.unchecked_add(2);
        Wrapping(mem.read_obj::<u16>(addr).unwrap())
    }

    /// Get the value of the used event field of the avail ring.
    #[inline(always)]
    pub fn used_event<M: GuestMemory>(&self, mem: &M) -> Wrapping<u16> {
        debug_assert!(self.is_layout_valid(mem));

        // We need to find the `used_event` field from the avail ring.
        let used_event_addr = self
            .avail_ring
            .unchecked_add(u64::from(4 + 2 * self.actual_size()));


Review comment (bug):
`write_obj` returns an error when the object your are trying to write into the address does not fit in the available memory, so the `SAFETY` comment above here is not accurate; you might have an address pointing inside the used ring but if you try to write something that does not fit in the ring this will fail.

Now, this thing will not happen here because you are actually writing a `UsedElement`. But I still don't understand why we should change the way of handling errors. Why is this needed for your PR (or the subsequent one)? I think it is not.

Code after:
            return Err(QueueError::DescIndexOutOfBounds(used_element.id));
        }

        // Used ring has layout:
        // struct UsedRing {
        //     flags: u16,
        //     idx: u16,
        //     ring: [UsedElement; <queue size>],
        //     avail_event: u16,
        // }
        // We calculate offset into `ring` field.
        let used_ring_offset = std::mem::size_of::<u16>()
            + std::mem::size_of::<u16>()
            + std::mem::size_of::<UsedElement>() * usize::from(index);
        let used_element_address = self.used_ring.unchecked_add(usize_to_u64(used_ring_offset));

        mem.write_obj(used_element, used_element_address)
            .map_err(QueueError::UsedRing)
    }

    /// Fetch the available ring index (`virtq_avail->idx`) from guest memory.
    /// This is written by the driver, to indicate the next slot that will be filled in the avail
    /// ring.
    pub fn avail_idx<M: GuestMemory>(&self, mem: &M) -> Wrapping<u16> {
        // Bound checks for queue inner data have already been performed, at device activation time,
        // via `self.is_valid()`, so it's safe to unwrap and use unchecked offsets here.
        // Note: the `MmioTransport` code ensures that queue addresses cannot be changed by the
        // guest       after device activation, so we can be certain that no change has
        // occurred since the last `self.is_valid()` check.
        let addr = self.avail_ring.unchecked_add(2);
        Wrapping(mem.read_obj::<u16>(addr).unwrap())
    }

    /// Get the value of the used event field of the avail ring.
    #[inline(always)]
    pub fn used_event<M: GuestMemory>(&self, mem: &M) -> Wrapping<u16> {
        debug_assert!(self.is_layout_valid(mem));

        // We need to find the `used_event` field from the avail ring.
        let used_event_addr = self
            .avail_ring
            .unchecked_add(u64::from(4 + 2 * self.actual_size()));

        Wrapping(mem.read_obj::<u16>(used_event_addr).unwrap())
    }

    /// Helper method that writes to the `avail_event` field of the used ring.
    #[inline(always)]
    fn set_used_ring_avail_event<M: GuestMemory>(&mut self, avail_event: u16, mem: &M) {
        debug_assert!(self.is_layout_valid(mem));