From 8c00b8829f03efd952070961d0174672b9a000c2 Mon Sep 17 00:00:00 2001 From: "Jorge E. Moreira" Date: Thu, 12 Feb 2026 18:54:41 -0800 Subject: [PATCH] vhost-user-backend: Avoid losing vring kicks By registering the file descriptors only when the queue is ready and enabled. This fixes a race in which a buffer could be added to the queue and the kick sent after registration but before it being enabled which causes the kick to be lost and the event never delivered to the backend. This was particularly prevalent when restoring a snapshot. Signed-off-by: Jorge E. Moreira --- coverage_config_x86_64.json | 2 +- vhost-user-backend/CHANGELOG.md | 2 + vhost-user-backend/src/backend.rs | 4 + vhost-user-backend/src/handler.rs | 119 ++++++++++++++++++++++++------ vhost-user-backend/src/vring.rs | 5 ++ 5 files changed, 108 insertions(+), 24 deletions(-) diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json index db15495..59038ef 100644 --- a/coverage_config_x86_64.json +++ b/coverage_config_x86_64.json @@ -1,5 +1,5 @@ { - "coverage_score": 86.22, + "coverage_score": 86.82, "exclude_path": "vhost/src/vhost_kern/", "crate_features": "vhost/vhost-user-frontend,vhost/vhost-user-backend,vhost-user-backend/postcopy" } diff --git a/vhost-user-backend/CHANGELOG.md b/vhost-user-backend/CHANGELOG.md index d1ede25..e77091c 100644 --- a/vhost-user-backend/CHANGELOG.md +++ b/vhost-user-backend/CHANGELOG.md @@ -7,6 +7,8 @@ ### Deprecated ### Fixed +- [[#343]](https://github.com/rust-vmm/vhost/pull/343) Avoid losing vring kicks + ## v0.21.0 ### Changed diff --git a/vhost-user-backend/src/backend.rs b/vhost-user-backend/src/backend.rs index 20e7daf..172f3d3 100644 --- a/vhost-user-backend/src/backend.rs +++ b/vhost-user-backend/src/backend.rs @@ -634,6 +634,10 @@ pub mod tests { backend } + + pub fn events(&self) -> u64 { + self.events + } } impl VhostUserBackendMut for MockVhostBackend { diff --git a/vhost-user-backend/src/handler.rs b/vhost-user-backend/src/handler.rs index 86b8c86..60abc3b 100644 --- a/vhost-user-backend/src/handler.rs +++ b/vhost-user-backend/src/handler.rs @@ -204,23 +204,43 @@ where } fn initialize_vring(&self, vring: &T::Vring, index: u8) -> VhostUserResult<()> { - assert!(vring.get_ref().get_kick().is_some()); + vring.set_queue_ready(true); + self.update_vring_registration(vring, index) + } - if let Some(fd) = vring.get_ref().get_kick() { + /// Adds or removes the vring's kick fd to the epoll instance based on the vring status. + /// Ensures that notifications are handled only while the vring is both started and enabled + /// and that no notifications are lost. + fn update_vring_registration(&self, vring: &T::Vring, index: u8) -> VhostUserResult<()> { + let vring_state = vring.get_ref(); + if let Some(fd) = vring_state.get_kick() { for (thread_index, queues_mask) in self.queues_per_thread.iter().enumerate() { let shifted_queues_mask = queues_mask >> index; if shifted_queues_mask & 1u64 == 1u64 { let evt_idx = queues_mask.count_ones() - shifted_queues_mask.count_ones(); - self.handlers[thread_index] - .register_event(fd.as_raw_fd(), EventSet::IN, u64::from(evt_idx)) - .map_err(VhostUserError::ReqHandlerError)?; + if vring_state.get_queue().ready() && vring_state.is_enabled() { + if let Err(e) = self.handlers[thread_index].register_event( + fd.as_raw_fd(), + EventSet::IN, + u64::from(evt_idx), + ) { + if e.kind() != io::ErrorKind::AlreadyExists { + // This could happen if we're asked by the frontend to enable an + // already enabled queue, don't fail in that case. + return Err(VhostUserError::ReqHandlerError(e)); + } + } + } else { + let _ = self.handlers[thread_index].unregister_event( + fd.as_raw_fd(), + EventSet::IN, + u64::from(evt_idx), + ); + } break; } } } - - vring.set_queue_ready(true); - Ok(()) } @@ -256,8 +276,9 @@ where fn reset_device(&mut self) -> VhostUserResult<()> { // Disable all vrings - for vring in self.vrings.iter_mut() { + for (index, vring) in self.vrings.iter().enumerate() { vring.set_enabled(false); + self.update_vring_registration(vring, index as u8)?; } // Reset device state, retain protocol state @@ -288,8 +309,9 @@ where // Note: If `VHOST_USER_F_PROTOCOL_FEATURES` has been negotiated we must leave // the vrings in their current state. if self.acked_features & VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits() == 0 { - for vring in self.vrings.iter_mut() { + for (index, vring) in self.vrings.iter().enumerate() { vring.set_enabled(true); + self.update_vring_registration(vring, index as u8)?; } } @@ -431,19 +453,7 @@ where // VHOST_USER_SET_VRING_KICK, and stop ring upon receiving // VHOST_USER_GET_VRING_BASE. vring.set_queue_ready(false); - - if let Some(fd) = vring.get_ref().get_kick() { - for (thread_index, queues_mask) in self.queues_per_thread.iter().enumerate() { - let shifted_queues_mask = queues_mask >> index; - if shifted_queues_mask & 1u64 == 1u64 { - let evt_idx = queues_mask.count_ones() - shifted_queues_mask.count_ones(); - self.handlers[thread_index] - .unregister_event(fd.as_raw_fd(), EventSet::IN, u64::from(evt_idx)) - .map_err(VhostUserError::ReqHandlerError)?; - break; - } - } - } + self.update_vring_registration(vring, index as u8)?; let next_avail = vring.queue_next_avail(); @@ -529,6 +539,7 @@ where // or after it has been disabled by VHOST_USER_SET_VRING_ENABLE // with parameter 0. vring.set_enabled(enable); + self.update_vring_registration(vring, index as u8)?; Ok(()) } @@ -788,3 +799,65 @@ impl Drop for VhostUserHandler { } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::backend::tests::MockVhostBackend; + use std::os::fd::IntoRawFd; + use std::os::unix::io::FromRawFd; + use std::sync::Mutex; + use std::thread; + use std::time::Duration; + use vhost::vhost_user::message::VhostUserVirtioFeatures; + use vm_memory::{GuestAddress, GuestMemoryAtomic, GuestMemoryMmap}; + use vmm_sys_util::event::{new_event_consumer_and_notifier, EventFlag}; + + #[test] + fn test_no_lost_kicks() { + let mem = GuestMemoryAtomic::new( + GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0x100000), 0x10000)]).unwrap(), + ); + let backend = Arc::new(Mutex::new(MockVhostBackend::new())); + let mut handler = VhostUserHandler::new(backend.clone(), mem.clone()).unwrap(); + handler + .set_features(VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits()) + .unwrap(); + + // Simulate VMM initializing the vring + let vring_index = 0; + + let (kick_consumer, notifier) = + new_event_consumer_and_notifier(EventFlag::empty()).unwrap(); + // Safety: we know kick_consumer is valid. + let kick_consumer_file = unsafe { File::from_raw_fd(kick_consumer.into_raw_fd()) }; + handler + .set_vring_kick(vring_index as u8, Some(kick_consumer_file)) + .unwrap(); + + // Ring is NOT enabled yet by default (if protocol features negotiated) + handler.set_vring_enable(vring_index as u32, false).unwrap(); + + // Kick it + notifier.notify().unwrap(); + + // The worker thread is already running (started in VhostUserHandler::new). + // Give it some time to NOT process the event. + thread::sleep(Duration::from_millis(200)); + + let events = backend.lock().unwrap().events(); + assert_eq!( + events, 0, + "Backend should NOT have been kicked while disabled" + ); + + // Now enable it. + handler.set_vring_enable(vring_index as u32, true).unwrap(); + + // Give it some time to process the NOW-registered event. + thread::sleep(Duration::from_millis(200)); + + let events = backend.lock().unwrap().events(); + assert_eq!(events, 1, "Backend SHOULD have been kicked after enabling"); + } +} diff --git a/vhost-user-backend/src/vring.rs b/vhost-user-backend/src/vring.rs index 3b4284c..69baec2 100644 --- a/vhost-user-backend/src/vring.rs +++ b/vhost-user-backend/src/vring.rs @@ -129,6 +129,11 @@ impl VringState { }) } + /// Whether the vring is enabled or not + pub fn is_enabled(&self) -> bool { + self.enabled + } + /// Get an immutable reference to the underlying raw `Queue` object. pub fn get_queue(&self) -> &Queue { &self.queue