From 489cdb7aeaa5be76dd7ac1fa5d291ac57b04d084 Mon Sep 17 00:00:00 2001 From: Kornel Date: Fri, 16 Feb 2024 15:44:53 +0000 Subject: [PATCH 1/2] fix: Ensure ConfigSpace has stable layout Rust doesn't guarantee struct layout by default, so `repr(C)` or `repr(transparent)` are necessary for safety. Signed-off-by: Kornel --- src/utils/src/net/mac.rs | 1 + src/vmm/src/devices/virtio/net/device.rs | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/utils/src/net/mac.rs b/src/utils/src/net/mac.rs index d0f6e3057c4..3ffba73874d 100644 --- a/src/utils/src/net/mac.rs +++ b/src/utils/src/net/mac.rs @@ -22,6 +22,7 @@ pub const MAC_ADDR_LEN: u8 = 6; /// Represents a MAC address #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +#[repr(transparent)] /// Representation of a MAC address. pub struct MacAddr { bytes: [u8; MAC_ADDR_LEN as usize], diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index 0e7a77aadb6..7a018e47770 100755 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/src/vmm/src/devices/virtio/net/device.rs @@ -96,11 +96,12 @@ fn init_vnet_hdr(buf: &mut [u8]) { } #[derive(Debug, Default, Clone, Copy)] +#[repr(C)] pub struct ConfigSpace { pub guest_mac: MacAddr, } -// SAFETY: `ConfigSpace` contains only PODs. +// SAFETY: `ConfigSpace` contains only PODs in `repr(C)` or `repr(transparent)`, without padding. unsafe impl ByteValued for ConfigSpace {} /// VirtIO network device. From 68cdc71bcfa890479ae13a0ce472dffb6a1e085c Mon Sep 17 00:00:00 2001 From: Kornel Date: Fri, 16 Feb 2024 15:45:05 +0000 Subject: [PATCH 2/2] refactor(virtio): Simplify read_config From: Kornel `io::Write` has higher overhead and unnecessary error handling. Slices can be copied without it. Signed-off-by: Kornel Signed-off-by: Patrick Roy Co-authored-by: Patrick Roy --- src/vmm/src/devices/virtio/balloon/device.rs | 21 ++++++------------- .../devices/virtio/block/vhost_user/device.rs | 18 +++++----------- src/vmm/src/devices/virtio/net/device.rs | 20 ++++++------------ 3 files changed, 17 insertions(+), 42 deletions(-) diff --git a/src/vmm/src/devices/virtio/balloon/device.rs b/src/vmm/src/devices/virtio/balloon/device.rs index 9a69312729f..77a13902cc2 100644 --- a/src/vmm/src/devices/virtio/balloon/device.rs +++ b/src/vmm/src/devices/virtio/balloon/device.rs @@ -1,11 +1,10 @@ // Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -use std::io::Write; +use std::fmt; use std::sync::atomic::AtomicU32; use std::sync::Arc; use std::time::Duration; -use std::{cmp, fmt}; use log::error; use serde::Serialize; @@ -593,20 +592,12 @@ impl VirtioDevice for Balloon { self.irq_trigger.irq_status.clone() } - fn read_config(&self, offset: u64, mut data: &mut [u8]) { - let config_space_bytes = self.config_space.as_slice(); - let config_len = config_space_bytes.len() as u64; - if offset >= config_len { + fn read_config(&self, offset: u64, data: &mut [u8]) { + if let Some(config_space_bytes) = self.config_space.as_slice().get(u64_to_usize(offset)..) { + let len = config_space_bytes.len().min(data.len()); + data[..len].copy_from_slice(&config_space_bytes[..len]); + } else { error!("Failed to read config space"); - return; - } - - if let Some(end) = offset.checked_add(data.len() as u64) { - // This write can't fail, offset and end are checked against config_len. - data.write_all( - &config_space_bytes[u64_to_usize(offset)..u64_to_usize(cmp::min(end, config_len))], - ) - .unwrap(); } } diff --git a/src/vmm/src/devices/virtio/block/vhost_user/device.rs b/src/vmm/src/devices/virtio/block/vhost_user/device.rs index a9cb8523f0f..2304262c26e 100644 --- a/src/vmm/src/devices/virtio/block/vhost_user/device.rs +++ b/src/vmm/src/devices/virtio/block/vhost_user/device.rs @@ -4,8 +4,6 @@ // Portions Copyright 2019 Intel Corporation. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -use std::cmp; -use std::io::Write; use std::sync::atomic::AtomicU32; use std::sync::Arc; @@ -322,19 +320,13 @@ impl VirtioDevice for VhostUserBlock self.irq_trigger.irq_status.clone() } - fn read_config(&self, offset: u64, mut data: &mut [u8]) { - let config_len = self.config_space.len() as u64; - if offset >= config_len { + fn read_config(&self, offset: u64, data: &mut [u8]) { + if let Some(config_space_bytes) = self.config_space.as_slice().get(u64_to_usize(offset)..) { + let len = config_space_bytes.len().min(data.len()); + data[..len].copy_from_slice(&config_space_bytes[..len]); + } else { error!("Failed to read config space"); self.metrics.cfg_fails.inc(); - return; - } - if let Some(end) = offset.checked_add(data.len() as u64) { - // This write can't fail, offset and end are checked against config_len. - data.write_all( - &self.config_space[u64_to_usize(offset)..u64_to_usize(cmp::min(end, config_len))], - ) - .unwrap(); } } diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index 7a018e47770..07336ec4165 100755 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/src/vmm/src/devices/virtio/net/device.rs @@ -7,11 +7,10 @@ #[cfg(not(test))] use std::io::Read; -use std::io::Write; +use std::mem; use std::net::Ipv4Addr; use std::sync::atomic::AtomicU32; use std::sync::{Arc, Mutex}; -use std::{cmp, mem}; use libc::EAGAIN; use log::{error, warn}; @@ -832,20 +831,13 @@ impl VirtioDevice for Net { self.irq_trigger.irq_status.clone() } - fn read_config(&self, offset: u64, mut data: &mut [u8]) { - let config_space_bytes = self.config_space.as_slice(); - let config_len = config_space_bytes.len() as u64; - if offset >= config_len { + fn read_config(&self, offset: u64, data: &mut [u8]) { + if let Some(config_space_bytes) = self.config_space.as_slice().get(u64_to_usize(offset)..) { + let len = config_space_bytes.len().min(data.len()); + data[..len].copy_from_slice(&config_space_bytes[..len]); + } else { error!("Failed to read config space"); self.metrics.cfg_fails.inc(); - return; - } - if let Some(end) = offset.checked_add(data.len() as u64) { - // This write can't fail, offset and end are checked against config_len. - data.write_all( - &config_space_bytes[u64_to_usize(offset)..u64_to_usize(cmp::min(end, config_len))], - ) - .unwrap(); } }