Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Manciukic wants to merge 4 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f86d41c
fix(vhost-user): require a shared mapping in update_mem_table
Manciukic Sep 10, 2026
08407e5
fix(vmm): reject vhost-user hot-add when memory cannot be shared
Manciukic Sep 10, 2026
f2cb533
test(vhost-user): cover hot-add of a vhost-user block
Manciukic Sep 10, 2026
19af4ff
docs: note that vhost-user hot-add needs shareable guest memory
Manciukic Sep 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 49 additions & 9 deletions src/vmm/src/devices/virtio/vhost_user.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,

@ShadowCurse ShadowCurse Sep 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this is a valid error for vhost-user devices. We must not create them if we don't use sharable memfd for memory.

@Manciukic Manciukic Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the only way preventing that is that we can't snapshot, but in theory the code could restore it. A modified snapshot could in theory contain it and this makes the check more generic and robust for the future.

/// Invalid used address
UsedAddress(GuestMemoryError),
}
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
9 changes: 9 additions & 0 deletions src/vmm/src/vstate/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Comment on lines +653 to +657

@ShadowCurse ShadowCurse Sep 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about just exposing is_shared function instead? This will convert the usage to just:

assert!(region.is_shared());
let Some(_file_offset) = region.file_offset() else {
  panic!("...")
}
let (mmap_handle, mmap_offset) = (_file_offset.file().as_raw_fd(), _file_offset.start());

@Manciukic Manciukic Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which is longer and more complicated than the code above? no thanks. Maybe I can add a separate is_shared helper but I don't really see the point.


/// 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());
Expand Down