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:
firecracker-microvm#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 <[email protected]>
Signed-off-by: Babis Chalios <[email protected]>

Author: Nikita Zakirov

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

src/vmm/src/devices/virtio/net/device.rs

@@ -209,10 +209,6 @@ impl Net {
     ) -> Result<Self, NetError> {
         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);
@@ -980,7 +1015,11 @@ pub mod tests {
             | 1 << VIRTIO_NET_F_HOST_TSO4
             | 1 << VIRTIO_NET_F_HOST_UFO
             | 1 << VIRTIO_F_VERSION_1
-            | 1 << VIRTIO_RING_F_EVENT_IDX;
+            | 1 << VIRTIO_RING_F_EVENT_IDX
+            | 1 << VIRTIO_NET_F_GUEST_TSO6
+            | 1 << VIRTIO_NET_F_HOST_TSO6
+            | 1 << VIRTIO_NET_F_GUEST_ECN
+            | 1 << VIRTIO_NET_F_HOST_ECN;
 
         assert_eq!(
             net.avail_features_by_page(0),
@@ -998,6 +1037,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();

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}