From 49ed5ea4b48ccd98903da037368fa3108f58ac1f Mon Sep 17 00:00:00 2001 From: Nikita Zakirov Date: Fri, 19 Jan 2024 15:48:21 +0000 Subject: [PATCH] fix(net): Apply only supported TAP offloading features Currently, we assume that the guest virtio-net driver acknowledges all the offload features we offer to it and then subsequently set the corresponding TAP features. This might be the case for the Linux guest kernel drivers we are currently using, but it is not necessarily the case. FreeBSD for example, might try to NACK some of them in some cases: https://github.com/firecracker-microvm/firecracker/issues/3905. This commit, changes the Firecracker implementation of VirtIO device to only setup the TAP offload features that correspond to the ones that the guest driver acknowledged. Signed-off-by: Nikita Zakirov Signed-off-by: Babis Chalios --- src/vmm/src/devices/virtio/mod.rs | 4 ++ src/vmm/src/devices/virtio/net/device.rs | 69 ++++++++++++++++++++++-- src/vmm/src/devices/virtio/net/mod.rs | 2 - 3 files changed, 69 insertions(+), 6 deletions(-) diff --git a/src/vmm/src/devices/virtio/mod.rs b/src/vmm/src/devices/virtio/mod.rs index 3575c53f1e2..9edf96514b0 100644 --- a/src/vmm/src/devices/virtio/mod.rs +++ b/src/vmm/src/devices/virtio/mod.rs @@ -9,6 +9,8 @@ use std::any::Any; +use crate::devices::virtio::net::TapError; + pub mod balloon; pub mod block; pub mod device; @@ -66,6 +68,8 @@ pub enum ActivateError { EventFd, /// Vhost user: {0} VhostUser(vhost_user::VhostUserError), + /// Setting tap interface offload flags failed: {0} + TapSetOffload(TapError), } /// Trait that helps in upcasting an object to Any diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index 1e0d2153296..911763fdb7d 100755 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/src/vmm/src/devices/virtio/net/device.rs @@ -209,10 +209,6 @@ impl Net { ) -> Result { let tap = Tap::open_named(tap_if_name).map_err(NetError::TapOpen)?; - // Set offload flags to match the virtio features below. - tap.set_offload(gen::TUN_F_CSUM | gen::TUN_F_UFO | gen::TUN_F_TSO4 | gen::TUN_F_TSO6) - .map_err(NetError::TapSetOffload)?; - let vnet_hdr_size = i32::try_from(vnet_hdr_len()).unwrap(); tap.set_vnet_hdr_size(vnet_hdr_size) .map_err(NetError::TapSetVnetHdrSize)?; @@ -658,6 +654,40 @@ impl Net { } } + /// Builds the offload features we will setup on the TAP device based on the features that the + /// guest supports. + fn build_tap_offload_features(guest_supported_features: u64) -> u32 { + let add_if_supported = + |tap_features: &mut u32, supported_features: u64, tap_flag: u32, virtio_flag: u32| { + if supported_features & (1 << virtio_flag) != 0 { + *tap_features |= tap_flag; + } + }; + + let mut tap_features: u32 = 0; + + add_if_supported( + &mut tap_features, + guest_supported_features, + gen::TUN_F_CSUM, + VIRTIO_NET_F_CSUM, + ); + add_if_supported( + &mut tap_features, + guest_supported_features, + gen::TUN_F_UFO, + VIRTIO_NET_F_GUEST_UFO, + ); + add_if_supported( + &mut tap_features, + guest_supported_features, + gen::TUN_F_TSO4, + VIRTIO_NET_F_GUEST_TSO4, + ); + + tap_features + } + /// Updates the parameters for the rate limiters pub fn patch_rate_limiters( &mut self, @@ -861,6 +891,11 @@ impl VirtioDevice for Net { } } + let supported_flags: u32 = Net::build_tap_offload_features(self.acked_features); + self.tap + .set_offload(supported_flags) + .map_err(super::super::ActivateError::TapSetOffload)?; + if self.activate_evt.write(1).is_err() { self.metrics.activate_fails.inc(); return Err(ActivateError::EventFd); @@ -998,6 +1033,32 @@ pub mod tests { assert_eq!(net.acked_features, features); } + #[test] + // Test that `Net::build_tap_offload_features` creates the TAP offload features that we expect + // it to do, based on the available guest features + fn test_build_tap_offload_features_all() { + let supported_features = + 1 << VIRTIO_NET_F_CSUM | 1 << VIRTIO_NET_F_GUEST_UFO | 1 << VIRTIO_NET_F_GUEST_TSO4; + let expected_tap_features = gen::TUN_F_CSUM | gen::TUN_F_UFO | gen::TUN_F_TSO4; + let supported_flags = Net::build_tap_offload_features(supported_features); + + assert_eq!(supported_flags, expected_tap_features); + } + + #[test] + // Same as before, however, using each supported feature one by one. + fn test_build_tap_offload_features_one_by_one() { + let features = [ + (1 << VIRTIO_NET_F_CSUM, gen::TUN_F_CSUM), + (1 << VIRTIO_NET_F_GUEST_UFO, gen::TUN_F_UFO), + (1 << VIRTIO_NET_F_GUEST_TSO4, gen::TUN_F_TSO4), + ]; + for (virtio_flag, tap_flag) in features { + let supported_flags = Net::build_tap_offload_features(virtio_flag); + assert_eq!(supported_flags, tap_flag); + } + } + #[test] fn test_virtio_device_read_config() { let mut net = default_net(); diff --git a/src/vmm/src/devices/virtio/net/mod.rs b/src/vmm/src/devices/virtio/net/mod.rs index 6754fc89f11..1a7972595ad 100644 --- a/src/vmm/src/devices/virtio/net/mod.rs +++ b/src/vmm/src/devices/virtio/net/mod.rs @@ -44,8 +44,6 @@ pub enum NetQueue { pub enum NetError { /// Open tap device failed: {0} TapOpen(TapError), - /// Setting tap interface offload flags failed: {0} - TapSetOffload(TapError), /// Setting vnet header size failed: {0} TapSetVnetHdrSize(TapError), /// EventFd error: {0}