fix(vmm): reject vhost-user hot-add when memory cannot be shared

src/vmm/src/device_manager/mod.rs

@@ -472,6 +472,15 @@ impl DeviceManager {
             VirtioDevices::Mmio(_) => return Err(VmmActionError::PciNotEnabled),
         }
 
+        // After the transport check, so a VM that cannot hotplug at all says so
+        // first. Without this the request succeeds and fails only at DRIVER_OK.
+        if config.is_vhost_user() && !vm.vhost_user_memory_shareable() {
+            return Err(VmmActionError::NotSupported(
+                "vhost-user hot-add requires guest memory that a backend can map shared"
+                    .to_string(),
+            ));
+        }
+
         let device = match config {
             HotplugDeviceConfig::Block(cfg) => Self::hotplug_make_block(cfg)?,
             HotplugDeviceConfig::Pmem(cfg) => Self::hotplug_make_pmem(vm.clone(), cfg)?,

src/vmm/src/devices/virtio/vhost_user.rs

@@ -51,8 +51,8 @@ pub enum VhostUserError {
     VhostUserSetVringKick(VhostError),
     /// Set vring enable failed: {0}
     VhostUserSetVringEnable(VhostError),
-    /// Failed to read vhost eventfd: No memory region found
-    VhostUserNoMemoryRegion,
+    /// Guest memory is not a shared file mapping, so the backend cannot map it
+    VhostUserMemoryNotShareable,
     /// Invalid used address
     UsedAddress(GuestMemoryError),
 }
@@ -368,12 +368,10 @@ impl<T: VhostUserHandleBackend> VhostUserHandleImpl<T> {
         let mut regions: Vec<VhostUserMemoryRegionInfo> = Vec::new();
 
         for region in mem.iter() {
-            let (mmap_handle, mmap_offset) = match region.file_offset() {
-                Some(_file_offset) => (_file_offset.file().as_raw_fd(), _file_offset.start()),
-                None => {
-                    return Err(VhostUserError::VhostUserNoMemoryRegion);
-                }
-            };
+            let file_offset = region
+                .shared_file_offset()
+                .ok_or(VhostUserError::VhostUserMemoryNotShareable)?;
+            let (mmap_handle, mmap_offset) = (file_offset.file().as_raw_fd(), file_offset.start());
 
             let vhost_user_net_reg = VhostUserMemoryRegionInfo {
                 guest_phys_addr: region.start_addr().raw_value(),
@@ -484,7 +482,7 @@ pub(crate) mod tests {
         GuestMemoryMmap::from_regions(
             memory::create(
                 regions.iter().copied(),
-                libc::MAP_PRIVATE,
+                libc::MAP_SHARED,
                 Some(file),
                 false,
                 libc::MADV_NORMAL,
@@ -760,6 +758,48 @@ pub(crate) mod tests {
         );
     }
 
+    #[test]
+    fn test_update_mem_table_rejects_unshareable_memory() {
+        struct NoBackend;
+        impl VhostUserHandleBackend for NoBackend {}
+
+        let vuh = VhostUserHandleImpl {
+            vu: NoBackend,
+            socket_path: "".to_string(),
+        };
+        let region_size = 0x10000;
+        let regions = [(GuestAddress(0), region_size)];
+
+        // Anonymous memory has no file to hand over.
+        let anon = crate::test_utils::single_region_mem(region_size);
+        assert!(matches!(
+            vuh.update_mem_table(&anon),
+            Err(VhostUserError::VhostUserMemoryNotShareable)
+        ));
+
+        // A private file mapping has one, but the backend would map its own copy.
+        let file = TempFile::new().unwrap().into_file();
+        file.set_len(region_size as u64).unwrap();
+        let private = GuestMemoryMmap::from_regions(
+            memory::create(
+                regions.iter().copied(),
+                libc::MAP_PRIVATE,
+                Some(file),
+                false,
+                libc::MADV_NORMAL,
+            )
+            .unwrap()
+            .into_iter()
+            .map(|region| GuestRegionMmapExt::dram_from_mmap_region(region, 0))
+            .collect(),
+        )
+        .unwrap();
+        assert!(matches!(
+            vuh.update_mem_table(&private),
+            Err(VhostUserError::VhostUserMemoryNotShareable)
+        ));
+    }
+
     #[test]
     fn test_update_mem_table() {
         struct MockFrontend {

src/vmm/src/vmm_config/drive.rs

@@ -74,6 +74,14 @@ pub struct BlockDeviceConfig {
     pub socket: Option<String>,
 }
 
+impl BlockDeviceConfig {
+    /// The config-level counterpart of `Block::is_vhost_user`, for before the
+    /// device is built. `socket` is the field `Block::new` selects the backend on.
+    pub fn is_vhost_user(&self) -> bool {
+        self.socket.is_some()
+    }
+}
+
 /// Only provided fields will be updated. I.e. if any optional fields
 /// are missing, they will not be updated.
 #[derive(Debug, Default, PartialEq, Eq, Deserialize)]
@@ -235,6 +243,21 @@ mod tests {
         assert_eq!(block_devs.devices.len(), 0);
     }
 
+    #[test]
+    fn test_is_vhost_user() {
+        let virtio = BlockDeviceConfig {
+            path_on_host: Some(String::from("/dev/null")),
+            ..Default::default()
+        };
+        assert!(!virtio.is_vhost_user());
+
+        let vhost_user = BlockDeviceConfig {
+            socket: Some(String::from("/tmp/vhost.sock")),
+            ..Default::default()
+        };
+        assert!(vhost_user.is_vhost_user());
+    }
+
     #[test]
     fn test_add_non_root_block_device() {
         let dummy_file = TempFile::new().unwrap();

src/vmm/src/vmm_config/mod.rs

@@ -61,6 +61,15 @@ impl HotplugDeviceConfig {
             Self::Net(_) => VirtioDeviceType::Net,
         }
     }
+
+    /// Whether this device is served by a vhost-user backend, which needs guest
+    /// memory it can map by fd.
+    pub(crate) fn is_vhost_user(&self) -> bool {
+        match self {
+            Self::Block(cfg) => cfg.is_vhost_user(),
+            Self::Pmem(_) | Self::Net(_) => false,
+        }
+    }
 }
 
 /// A public-facing, stateless structure, holding all the data we need to create a TokenBucket
@@ -189,6 +198,28 @@ mod tests {
     const ONE_TIME_BURST: u64 = 1024;
     const REFILL_TIME: u64 = 1000;
 
+    #[test]
+    fn test_hotplug_is_vhost_user() {
+        let block = |socket: Option<&str>| crate::vmm_config::drive::BlockDeviceConfig {
+            path_on_host: socket.is_none().then(|| String::from("/dev/null")),
+            socket: socket.map(String::from),
+            ..Default::default()
+        };
+
+        assert!(HotplugDeviceConfig::Block(block(Some("/tmp/vhost.sock"))).is_vhost_user());
+        assert!(!HotplugDeviceConfig::Block(block(None)).is_vhost_user());
+        // Only block has a vhost-user variant today.
+        let net = crate::vmm_config::net::NetworkInterfaceConfig {
+            iface_id: String::from("eth0"),
+            host_dev_name: String::from("tap0"),
+            guest_mac: None,
+            mtu: None,
+            rx_rate_limiter: None,
+            tx_rate_limiter: None,
+        };
+        assert!(!HotplugDeviceConfig::Net(net).is_vhost_user());
+    }
+
     #[test]
     fn test_rate_limiter_configs() {
         let rlconf = RateLimiterConfig {

src/vmm/src/vstate/memory.rs

@@ -647,6 +647,15 @@ impl<'a> GuestMemorySlot<'a> {
 }
 
 impl GuestRegionMmapExt {
+    /// The backing file, if another process could map the same pages from it:
+    /// file-backed and `MAP_SHARED`. A private mapping copies on write, so a
+    /// second mapping of the file diverges from this one.
+    pub fn shared_file_offset(&self) -> Option<&FileOffset> {
+        self.inner
+            .file_offset()
+            .filter(|_| self.inner.flags() & libc::MAP_SHARED != 0)
+    }
+
     /// Adds a DRAM region which only contains a single plugged slot
     pub(crate) fn dram_from_mmap_region(region: GuestRegionMmap, slot: u32) -> Self {
         let slot_size = u64_to_usize(region.len());

src/vmm/src/vstate/vm.rs

@@ -552,6 +552,19 @@ impl KvmVm {
         &self.common.guest_memory
     }
 
+    /// Whether a vhost-user backend can map this VM's guest memory.
+    ///
+    /// Guest memory is a shared memfd mapping only when a vhost-user device is
+    /// configured before boot. Otherwise it is anonymous, or a private mapping
+    /// of a snapshot file, and a backend given its descriptor would see other
+    /// pages than the guest.
+    pub fn vhost_user_memory_shareable(&self) -> bool {
+        // Never empty after boot, so `all` cannot pass vacuously.
+        self.guest_memory()
+            .iter()
+            .all(|r| r.shared_file_offset().is_some())
+    }
+
     /// Gets a mutable reference to this [`KvmVm`]'s [`ResourceAllocator`] object
     pub fn resource_allocator(&self) -> MutexGuard<'_, ResourceAllocator> {
         self.common
@@ -831,6 +844,39 @@ pub(crate) mod tests {
         vm
     }
 
+    #[test]
+    fn test_vhost_user_memory_shareable() {
+        // Anonymous: what a VM booted without a vhost-user device gets.
+        let vm = setup_vm_with_memory(mib_to_bytes(128));
+        assert!(!vm.vhost_user_memory_shareable());
+
+        // memfd-backed: what a VM configured with a vhost-user block before boot
+        // gets.
+        let mut vm = setup_vm();
+        let regions = arch::arch_memory_regions(mib_to_bytes(128));
+        let gm =
+            crate::vstate::memory::memfd_backed(&regions, false, HugePageConfig::None).unwrap();
+        vm.register_dram_memory_regions(gm).unwrap();
+        assert!(vm.vhost_user_memory_shareable());
+
+        // A snapshot file is mapped private: it has a descriptor and must still be
+        // rejected.
+        let mut vm = setup_vm();
+        let file = vmm_sys_util::tempfile::TempFile::new().unwrap().into_file();
+        let regions = arch::arch_memory_regions(mib_to_bytes(128));
+        let total: u64 = regions.iter().map(|&(_, size)| size as u64).sum();
+        file.set_len(total).unwrap();
+        let gm = crate::vstate::memory::snapshot_file(
+            file,
+            regions.into_iter(),
+            false,
+            HugePageConfig::None,
+        )
+        .unwrap();
+        vm.register_dram_memory_regions(gm).unwrap();
+        assert!(!vm.vhost_user_memory_shareable());
+    }
+
     #[test]
     fn test_new() {
         // Testing with a valid /dev/kvm descriptor.

tests/integration_tests/functional/test_drive_vhost_user.py

@@ -9,6 +9,7 @@
 import pytest
 
 import host_tools.drive as drive_tools
+from framework.artifacts import ACPI_GUEST_KERNELS, pin_guest_kernel, pin_pci
 from framework.utils_drive import partuuid_and_disk_path
 from host_tools.fcmetrics import FcDeviceMetrics
 
@@ -300,6 +301,86 @@ def test_partuuid_update(uvm_vhost_user_plain_any, rootfs):
     vhost_user_block_metrics.validate(vm)
 
 
+@pin_pci(True)
+@pin_guest_kernel(ACPI_GUEST_KERNELS)
+def test_hotplug_vhost_user(uvm_vhost_user_booted_ro):
+    """A vhost-user block can be hot-added to a VM booted with one.
+
+    Booting with a vhost-user drive makes guest memory memfd-backed, which is
+    what the backend of a hot-added drive maps. The new drive must appear in
+    the guest and carry data through the backend.
+    """
+    vm = uvm_vhost_user_booted_ro
+    _, before, _ = vm.ssh.check_output("ls /sys/block")
+
+    fs = drive_tools.FilesystemFile(size=16)
+    vm.add_vhost_user_drive("scratch", fs.path)
+
+    # No hotplug notification yet, so the guest rescans the bus itself.
+    vm.ssh.check_output("echo 1 > /sys/bus/pci/rescan")
+    _, after, _ = vm.ssh.check_output("ls /sys/block")
+    new = set(after.split()) - set(before.split())
+    assert len(new) == 1, new
+    dev = f"/dev/{new.pop()}"
+
+    # The rootfs is read-only; /tmp is writable.
+    vm.ssh.check_output(f"mkfs.ext4 {dev}")
+    vm.ssh.check_output(f"mkdir -p /tmp/scratch && mount {dev} /tmp/scratch")
+    vm.ssh.check_output("echo vhost_user_hotplug > /tmp/scratch/probe")
+    assert (
+        vm.ssh.check_output("cat /tmp/scratch/probe").stdout.strip()
+        == "vhost_user_hotplug"
+    )
+    vm.ssh.check_output("umount /tmp/scratch")
+
+
+@pin_pci(True)
+def test_hotplug_without_memfd_rejected(uvm):
+    """A vhost-user block cannot be hot-added to a VM booted without one.
+
+    Guest memory is memfd-backed only when a vhost-user device is configured
+    before boot. The request must be refused up front rather than accepted and
+    failed at DRIVER_OK. It is refused before any backend connection, so no
+    backend is started here.
+    """
+    vm = uvm
+    vm.spawn(log_level="Info")
+    vm.basic_config()
+    vm.add_net_iface()
+    vm.start()
+
+    with pytest.raises(RuntimeError, match="vhost-user hot-add requires guest memory"):
+        vm.api.drive.put(
+            drive_id="vub0",
+            socket="/tmp/nonexistent-vhost-user.sock",
+            is_root_device=False,
+        )
+
+
+@pin_pci(True)
+def test_hotplug_after_file_restore_rejected(uvm, microvm_factory):
+    """A vhost-user block cannot be hot-added to a VM restored from a snapshot file.
+
+    The restored memory is a private mapping of the snapshot file: it has a
+    descriptor, yet a backend given it would map its own copy of the file. The
+    request must be refused rather than accepted and failed at DRIVER_OK.
+    """
+    vm = uvm
+    vm.spawn(log_level="Info")
+    vm.basic_config()
+    vm.add_net_iface()
+    vm.start()
+    snapshot = vm.snapshot_full()
+
+    restored = microvm_factory.build_from_snapshot(snapshot)
+    with pytest.raises(RuntimeError, match="vhost-user hot-add requires guest memory"):
+        restored.api.drive.put(
+            drive_id="vub0",
+            socket="/tmp/nonexistent-vhost-user.sock",
+            is_root_device=False,
+        )
+
+
 def test_config_change(uvm):
     """
     Verify handling of block device resize.