Merge pull request #931 from dgibson/bug703

Forward port device conflict fixes from Kata 1 / Go agent

src/agent/src/device.rs

@@ -28,8 +28,15 @@ macro_rules! sl {
 
 const VM_ROOTFS: &str = "/";
 
+struct DevIndexEntry {
+    idx: usize,
+    residx: Vec<usize>,
+}
+
+struct DevIndex(HashMap<String, DevIndexEntry>);
+
 // DeviceHandler is the type of callback to be defined to handle every type of device driver.
-type DeviceHandler = fn(&Device, &mut Spec, &Arc<Mutex<Sandbox>>) -> Result<()>;
+type DeviceHandler = fn(&Device, &mut Spec, &Arc<Mutex<Sandbox>>, &DevIndex) -> Result<()>;
 
 // DeviceHandlerList lists the supported drivers.
 #[cfg_attr(rustfmt, rustfmt_skip)]
@@ -194,7 +201,7 @@ fn scan_scsi_bus(scsi_addr: &str) -> Result<()> {
 // the same device in the list of devices provided through the OCI spec.
 // This is needed to update information about minor/major numbers that cannot
 // be predicted from the caller.
-fn update_spec_device_list(device: &Device, spec: &mut Spec) -> Result<()> {
+fn update_spec_device_list(device: &Device, spec: &mut Spec, devidx: &DevIndex) -> Result<()> {
     let major_id: c_uint;
     let minor_id: c_uint;
 
@@ -228,44 +235,44 @@ fn update_spec_device_list(device: &Device, spec: &mut Spec) -> Result<()> {
         "got the device: dev_path: {}, major: {}, minor: {}\n", &device.vm_path, major_id, minor_id
     );
 
-    let devices = linux.devices.as_mut_slice();
-    for dev in devices.iter_mut() {
-        if dev.path == device.container_path {
-            let host_major = dev.major;
-            let host_minor = dev.minor;
+    if let Some(idxdata) = devidx.0.get(device.container_path.as_str()) {
+        let dev = &mut linux.devices[idxdata.idx];
+        let host_major = dev.major;
+        let host_minor = dev.minor;
 
-            dev.major = major_id as i64;
-            dev.minor = minor_id as i64;
+        dev.major = major_id as i64;
+        dev.minor = minor_id as i64;
+
+        info!(
+            sl!(),
+            "change the device from major: {} minor: {} to vm device major: {} minor: {}",
+            host_major,
+            host_minor,
+            major_id,
+            minor_id
+        );
+
+        // Resources must be updated since they are used to identify
+        // the device in the devices cgroup.
+        for ridx in &idxdata.residx {
+            // unwrap is safe, because residx would be empty if there
+            // were no resources
+            let res = &mut linux.resources.as_mut().unwrap().devices[*ridx];
+            res.major = Some(major_id as i64);
+            res.minor = Some(minor_id as i64);
 
             info!(
                 sl!(),
-                "change the device from major: {} minor: {} to vm device major: {} minor: {}",
-                host_major,
-                host_minor,
-                major_id,
-                minor_id
+                "set resources for device major: {} minor: {}\n", major_id, minor_id
             );
-
-            // Resources must be updated since they are used to identify the
-            // device in the devices cgroup.
-            if let Some(res) = linux.resources.as_mut() {
-                let ds = res.devices.as_mut_slice();
-                for d in ds.iter_mut() {
-                    if d.major == Some(host_major) && d.minor == Some(host_minor) {
-                        d.major = Some(major_id as i64);
-                        d.minor = Some(minor_id as i64);
-
-                        info!(
-                            sl!(),
-                            "set resources for device major: {} minor: {}\n", major_id, minor_id
-                        );
-                    }
-                }
-            }
         }
+        Ok(())
+    } else {
+        Err(anyhow!(
+            "Should have found a matching device {} in the spec",
+            device.vm_path
+        ))
     }
-
-    Ok(())
 }
 
 // device.Id should be the predicted device name (vda, vdb, ...)
@@ -274,12 +281,13 @@ fn virtiommio_blk_device_handler(
     device: &Device,
     spec: &mut Spec,
     _sandbox: &Arc<Mutex<Sandbox>>,
+    devidx: &DevIndex,
 ) -> Result<()> {
     if device.vm_path == "" {
         return Err(anyhow!("Invalid path for virtio mmio blk device"));
     }
 
-    update_spec_device_list(device, spec)
+    update_spec_device_list(device, spec, devidx)
 }
 
 // device.Id should be the PCI address in the format  "bridgeAddr/deviceAddr".
@@ -289,6 +297,7 @@ fn virtio_blk_device_handler(
     device: &Device,
     spec: &mut Spec,
     sandbox: &Arc<Mutex<Sandbox>>,
+    devidx: &DevIndex,
 ) -> Result<()> {
     let mut dev = device.clone();
 
@@ -298,7 +307,7 @@ fn virtio_blk_device_handler(
         dev.vm_path = get_pci_device_name(sandbox, &device.id)?;
     }
 
-    update_spec_device_list(&dev, spec)
+    update_spec_device_list(&dev, spec, devidx)
 }
 
 // device.Id should be the SCSI address of the disk in the format "scsiID:lunID"
@@ -306,22 +315,49 @@ fn virtio_scsi_device_handler(
     device: &Device,
     spec: &mut Spec,
     sandbox: &Arc<Mutex<Sandbox>>,
+    devidx: &DevIndex,
 ) -> Result<()> {
     let mut dev = device.clone();
     dev.vm_path = get_scsi_device_name(sandbox, &device.id)?;
-    update_spec_device_list(&dev, spec)
+    update_spec_device_list(&dev, spec, devidx)
 }
 
 fn virtio_nvdimm_device_handler(
     device: &Device,
     spec: &mut Spec,
     _sandbox: &Arc<Mutex<Sandbox>>,
+    devidx: &DevIndex,
 ) -> Result<()> {
     if device.vm_path == "" {
         return Err(anyhow!("Invalid path for nvdimm device"));
     }
 
-    update_spec_device_list(device, spec)
+    update_spec_device_list(device, spec, devidx)
+}
+
+impl DevIndex {
+    fn new(spec: &Spec) -> DevIndex {
+        let mut map = HashMap::new();
+
+        for linux in spec.linux.as_ref() {
+            for (i, d) in linux.devices.iter().enumerate() {
+                let mut residx = Vec::new();
+
+                for linuxres in linux.resources.as_ref() {
+                    for (j, r) in linuxres.devices.iter().enumerate() {
+                        if r.r#type == d.r#type
+                            && r.major == Some(d.major)
+                            && r.minor == Some(d.minor)
+                        {
+                            residx.push(j);
+                        }
+                    }
+                }
+                map.insert(d.path.clone(), DevIndexEntry { idx: i, residx });
+            }
+        }
+        DevIndex(map)
+    }
 }
 
 pub fn add_devices(
@@ -329,14 +365,21 @@ pub fn add_devices(
     spec: &mut Spec,
     sandbox: &Arc<Mutex<Sandbox>>,
 ) -> Result<()> {
+    let devidx = DevIndex::new(spec);
+
     for device in devices.iter() {
-        add_device(device, spec, sandbox)?;
+        add_device(device, spec, sandbox, &devidx)?;
     }
 
     Ok(())
 }
 
-fn add_device(device: &Device, spec: &mut Spec, sandbox: &Arc<Mutex<Sandbox>>) -> Result<()> {
+fn add_device(
+    device: &Device,
+    spec: &mut Spec,
+    sandbox: &Arc<Mutex<Sandbox>>,
+    devidx: &DevIndex,
+) -> Result<()> {
     // log before validation to help with debugging gRPC protocol version differences.
     info!(sl!(), "device-id: {}, device-type: {}, device-vm-path: {}, device-container-path: {}, device-options: {:?}",
           device.id, device.field_type, device.vm_path, device.container_path, device.options);
@@ -355,7 +398,7 @@ fn add_device(device: &Device, spec: &mut Spec, sandbox: &Arc<Mutex<Sandbox>>) -
 
     match DEVICEHANDLERLIST.get(device.field_type.as_str()) {
         None => Err(anyhow!("Unknown device type {}", device.field_type)),
-        Some(dev_handler) => dev_handler(device, spec, sandbox),
+        Some(dev_handler) => dev_handler(device, spec, sandbox, devidx),
     }
 }
 
@@ -413,4 +456,263 @@ mod tests {
         assert_eq!(devices[0].major, Some(major));
         assert_eq!(devices[0].minor, Some(minor));
     }
+
+    #[test]
+    fn test_update_spec_device_list() {
+        let (major, minor) = (7, 2);
+        let mut device = Device::default();
+        let mut spec = Spec::default();
+
+        // container_path empty
+        let devidx = DevIndex::new(&spec);
+        let res = update_spec_device_list(&device, &mut spec, &devidx);
+        assert!(res.is_err());
+
+        device.container_path = "/dev/null".to_string();
+
+        // linux is empty
+        let devidx = DevIndex::new(&spec);
+        let res = update_spec_device_list(&device, &mut spec, &devidx);
+        assert!(res.is_err());
+
+        spec.linux = Some(Linux::default());
+
+        // linux.devices is empty
+        let devidx = DevIndex::new(&spec);
+        let res = update_spec_device_list(&device, &mut spec, &devidx);
+        assert!(res.is_err());
+
+        spec.linux.as_mut().unwrap().devices = vec![oci::LinuxDevice {
+            path: "/dev/null2".to_string(),
+            major,
+            minor,
+            ..oci::LinuxDevice::default()
+        }];
+
+        // vm_path empty
+        let devidx = DevIndex::new(&spec);
+        let res = update_spec_device_list(&device, &mut spec, &devidx);
+        assert!(res.is_err());
+
+        device.vm_path = "/dev/null".to_string();
+
+        // guest and host path are not the same
+        let devidx = DevIndex::new(&spec);
+        let res = update_spec_device_list(&device, &mut spec, &devidx);
+        assert!(res.is_err(), "device={:?} spec={:?}", device, spec);
+
+        spec.linux.as_mut().unwrap().devices[0].path = device.container_path.clone();
+
+        // spec.linux.resources is empty
+        let devidx = DevIndex::new(&spec);
+        let res = update_spec_device_list(&device, &mut spec, &devidx);
+        assert!(res.is_ok());
+
+        // update both devices and cgroup lists
+        spec.linux.as_mut().unwrap().devices = vec![oci::LinuxDevice {
+            path: device.container_path.clone(),
+            major,
+            minor,
+            ..oci::LinuxDevice::default()
+        }];
+
+        spec.linux.as_mut().unwrap().resources = Some(oci::LinuxResources {
+            devices: vec![oci::LinuxDeviceCgroup {
+                major: Some(major),
+                minor: Some(minor),
+                ..oci::LinuxDeviceCgroup::default()
+            }],
+            ..oci::LinuxResources::default()
+        });
+
+        let devidx = DevIndex::new(&spec);
+        let res = update_spec_device_list(&device, &mut spec, &devidx);
+        assert!(res.is_ok());
+    }
+
+    #[test]
+    fn test_update_spec_device_list_guest_host_conflict() {
+        let null_rdev = fs::metadata("/dev/null").unwrap().rdev();
+        let zero_rdev = fs::metadata("/dev/zero").unwrap().rdev();
+        let full_rdev = fs::metadata("/dev/full").unwrap().rdev();
+
+        let host_major_a = stat::major(null_rdev) as i64;
+        let host_minor_a = stat::minor(null_rdev) as i64;
+        let host_major_b = stat::major(zero_rdev) as i64;
+        let host_minor_b = stat::minor(zero_rdev) as i64;
+
+        let mut spec = Spec {
+            linux: Some(Linux {
+                devices: vec![
+                    oci::LinuxDevice {
+                        path: "/dev/a".to_string(),
+                        r#type: "c".to_string(),
+                        major: host_major_a,
+                        minor: host_minor_a,
+                        ..oci::LinuxDevice::default()
+                    },
+                    oci::LinuxDevice {
+                        path: "/dev/b".to_string(),
+                        r#type: "c".to_string(),
+                        major: host_major_b,
+                        minor: host_minor_b,
+                        ..oci::LinuxDevice::default()
+                    },
+                ],
+                resources: Some(LinuxResources {
+                    devices: vec![
+                        oci::LinuxDeviceCgroup {
+                            r#type: "c".to_string(),
+                            major: Some(host_major_a),
+                            minor: Some(host_minor_a),
+                            ..oci::LinuxDeviceCgroup::default()
+                        },
+                        oci::LinuxDeviceCgroup {
+                            r#type: "c".to_string(),
+                            major: Some(host_major_b),
+                            minor: Some(host_minor_b),
+                            ..oci::LinuxDeviceCgroup::default()
+                        },
+                    ],
+                    ..LinuxResources::default()
+                }),
+                ..Linux::default()
+            }),
+            ..Spec::default()
+        };
+        let devidx = DevIndex::new(&spec);
+
+        let dev_a = Device {
+            container_path: "/dev/a".to_string(),
+            vm_path: "/dev/zero".to_string(),
+            ..Device::default()
+        };
+
+        let guest_major_a = stat::major(zero_rdev) as i64;
+        let guest_minor_a = stat::minor(zero_rdev) as i64;
+
+        let dev_b = Device {
+            container_path: "/dev/b".to_string(),
+            vm_path: "/dev/full".to_string(),
+            ..Device::default()
+        };
+
+        let guest_major_b = stat::major(full_rdev) as i64;
+        let guest_minor_b = stat::minor(full_rdev) as i64;
+
+        let specdevices = &spec.linux.as_ref().unwrap().devices;
+        assert_eq!(host_major_a, specdevices[0].major);
+        assert_eq!(host_minor_a, specdevices[0].minor);
+        assert_eq!(host_major_b, specdevices[1].major);
+        assert_eq!(host_minor_b, specdevices[1].minor);
+
+        let specresources = spec.linux.as_ref().unwrap().resources.as_ref().unwrap();
+        assert_eq!(Some(host_major_a), specresources.devices[0].major);
+        assert_eq!(Some(host_minor_a), specresources.devices[0].minor);
+        assert_eq!(Some(host_major_b), specresources.devices[1].major);
+        assert_eq!(Some(host_minor_b), specresources.devices[1].minor);
+
+        let res = update_spec_device_list(&dev_a, &mut spec, &devidx);
+        assert!(res.is_ok());
+
+        let specdevices = &spec.linux.as_ref().unwrap().devices;
+        assert_eq!(guest_major_a, specdevices[0].major);
+        assert_eq!(guest_minor_a, specdevices[0].minor);
+        assert_eq!(host_major_b, specdevices[1].major);
+        assert_eq!(host_minor_b, specdevices[1].minor);
+
+        let specresources = spec.linux.as_ref().unwrap().resources.as_ref().unwrap();
+        assert_eq!(Some(guest_major_a), specresources.devices[0].major);
+        assert_eq!(Some(guest_minor_a), specresources.devices[0].minor);
+        assert_eq!(Some(host_major_b), specresources.devices[1].major);
+        assert_eq!(Some(host_minor_b), specresources.devices[1].minor);
+
+        let res = update_spec_device_list(&dev_b, &mut spec, &devidx);
+        assert!(res.is_ok());
+
+        let specdevices = &spec.linux.as_ref().unwrap().devices;
+        assert_eq!(guest_major_a, specdevices[0].major);
+        assert_eq!(guest_minor_a, specdevices[0].minor);
+        assert_eq!(guest_major_b, specdevices[1].major);
+        assert_eq!(guest_minor_b, specdevices[1].minor);
+
+        let specresources = spec.linux.as_ref().unwrap().resources.as_ref().unwrap();
+        assert_eq!(Some(guest_major_a), specresources.devices[0].major);
+        assert_eq!(Some(guest_minor_a), specresources.devices[0].minor);
+        assert_eq!(Some(guest_major_b), specresources.devices[1].major);
+        assert_eq!(Some(guest_minor_b), specresources.devices[1].minor);
+    }
+
+    #[test]
+    fn test_update_spec_device_list_char_block_conflict() {
+        let null_rdev = fs::metadata("/dev/null").unwrap().rdev();
+
+        let guest_major = stat::major(null_rdev) as i64;
+        let guest_minor = stat::minor(null_rdev) as i64;
+        let host_major: i64 = 99;
+        let host_minor: i64 = 99;
+
+        let mut spec = Spec {
+            linux: Some(Linux {
+                devices: vec![
+                    oci::LinuxDevice {
+                        path: "/dev/char".to_string(),
+                        r#type: "c".to_string(),
+                        major: host_major,
+                        minor: host_minor,
+                        ..oci::LinuxDevice::default()
+                    },
+                    oci::LinuxDevice {
+                        path: "/dev/block".to_string(),
+                        r#type: "b".to_string(),
+                        major: host_major,
+                        minor: host_minor,
+                        ..oci::LinuxDevice::default()
+                    },
+                ],
+                resources: Some(LinuxResources {
+                    devices: vec![
+                        LinuxDeviceCgroup {
+                            r#type: "c".to_string(),
+                            major: Some(host_major),
+                            minor: Some(host_minor),
+                            ..LinuxDeviceCgroup::default()
+                        },
+                        LinuxDeviceCgroup {
+                            r#type: "b".to_string(),
+                            major: Some(host_major),
+                            minor: Some(host_minor),
+                            ..LinuxDeviceCgroup::default()
+                        },
+                    ],
+                    ..LinuxResources::default()
+                }),
+                ..Linux::default()
+            }),
+            ..Spec::default()
+        };
+        let devidx = DevIndex::new(&spec);
+
+        let dev = Device {
+            container_path: "/dev/char".to_string(),
+            vm_path: "/dev/null".to_string(),
+            ..Device::default()
+        };
+
+        let specresources = spec.linux.as_ref().unwrap().resources.as_ref().unwrap();
+        assert_eq!(Some(host_major), specresources.devices[0].major);
+        assert_eq!(Some(host_minor), specresources.devices[0].minor);
+        assert_eq!(Some(host_major), specresources.devices[1].major);
+        assert_eq!(Some(host_minor), specresources.devices[1].minor);
+
+        let res = update_spec_device_list(&dev, &mut spec, &devidx);
+        assert!(res.is_ok());
+
+        // Only the char device, not the block device should be updated
+        let specresources = spec.linux.as_ref().unwrap().resources.as_ref().unwrap();
+        assert_eq!(Some(guest_major), specresources.devices[0].major);
+        assert_eq!(Some(guest_minor), specresources.devices[0].minor);
+        assert_eq!(Some(host_major), specresources.devices[1].major);
+        assert_eq!(Some(host_minor), specresources.devices[1].minor);
+    }
 }