From 14db3cd16531f4cf46ea33e0154e614cb7df58e7 Mon Sep 17 00:00:00 2001 From: Alyssa Ross Date: Thu, 27 Mar 2025 13:34:58 +0100 Subject: [PATCH] vhost_user: fix replies without GET_PROTOCOL_FEATURES I observed hangs in the following situation when using Cloud Hypervisor with virtiofsd: 1. Start virtiofsd 2. Start Cloud Hypervisor instance 1, connected to virtiofsd. 3. Start Cloud Hypervisor instance 2, waiting for migration. 4. Migrate VM from Cloud Hypervisor instance 1 to 2. 5. Start virtiofsd again. The hangs happened because Cloud Hypervisor remembered, as part of the migration data, which vhost-user protocol features the backend for its fs device supported. Instance 2 therefore never sent GET_PROTOCOL_FEATURES to the second invocation of virtiofsd. This should work, but it didn't, because update_reply_ack_flag() checked whether self.protocol_features contained GET_PROTOCOL_FEATURES, but self.protocol_features is only filled in when GET_PROTOCOL_FEATURES is called. As a result, Cloud Hypervisor expected a reply that virtiofsd would never send. Since REPLY_ACK is handled entirely by the vhost-user library, and not by the backend, there's no need to ask the backend whether it supports REPLY_ACK in the first place, so we can just drop the check for that from update_reply_ack_flag(). We know that we always support it, so we just need to check whether the backend has acked it. This fixes the hang described above. Since we will now always reply if the backend acks the feature, REPLY_ACK is now always included in the set of features returned by GET_PROTOCOL_FEATURES, just like with XEN_MMAP (when enabled). Signed-off-by: Alyssa Ross --- vhost-user-backend/tests/vhost-user-server.rs | 3 ++- vhost/CHANGELOG.md | 4 ++++ vhost/src/vhost_user/backend_req_handler.rs | 8 +++----- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/vhost-user-backend/tests/vhost-user-server.rs b/vhost-user-backend/tests/vhost-user-server.rs index 70b3cc5..e371299 100644 --- a/vhost-user-backend/tests/vhost-user-server.rs +++ b/vhost-user-backend/tests/vhost-user-server.rs @@ -59,7 +59,8 @@ impl VhostUserBackendMut for MockVhostBackend { } fn protocol_features(&self) -> VhostUserProtocolFeatures { - VhostUserProtocolFeatures::all() + // Exclude REPLY_ACK to test that it is automatically added. + VhostUserProtocolFeatures::all() - VhostUserProtocolFeatures::REPLY_ACK } fn reset_device(&mut self) { diff --git a/vhost/CHANGELOG.md b/vhost/CHANGELOG.md index dbbb0c1..ead2f47 100644 --- a/vhost/CHANGELOG.md +++ b/vhost/CHANGELOG.md @@ -5,6 +5,10 @@ - [[#268]](https://github.com/rust-vmm/vhost/pull/268) Add support for `VHOST_USER_GET_SHARED_OBJECT` ### Changed +- [[#290]](https://github.com/rust-vmm/vhost/pull/290) Backends now + always support `VHOST_USER_PROTOCOL_F_REPLY_ACK`, without the + `VhostUserBackendReqHandler` implementation having to include it in + the features returned from `get_protocol_features`. ### Deprecated diff --git a/vhost/src/vhost_user/backend_req_handler.rs b/vhost/src/vhost_user/backend_req_handler.rs index 409073c..d74b045 100644 --- a/vhost/src/vhost_user/backend_req_handler.rs +++ b/vhost/src/vhost_user/backend_req_handler.rs @@ -327,7 +327,6 @@ pub struct BackendReqHandler { virtio_features: u64, acked_virtio_features: u64, - protocol_features: VhostUserProtocolFeatures, acked_protocol_features: u64, // sending ack for messages without payload @@ -347,7 +346,6 @@ impl BackendReqHandler { backend, virtio_features: 0, acked_virtio_features: 0, - protocol_features: VhostUserProtocolFeatures::empty(), acked_protocol_features: 0, reply_ack_enabled: false, error: None, @@ -512,7 +510,9 @@ impl BackendReqHandler { } Ok(FrontendReq::GET_PROTOCOL_FEATURES) => { self.check_request_size(&hdr, size, 0)?; - let features = self.backend.get_protocol_features()?; + // REPLY_ACK is implemented entirely by this library, so it's always supported. + let features = + self.backend.get_protocol_features()? | VhostUserProtocolFeatures::REPLY_ACK; // Enable the `XEN_MMAP` protocol feature for backends if xen feature is enabled. #[cfg(feature = "xen")] @@ -520,7 +520,6 @@ impl BackendReqHandler { let msg = VhostUserU64::new(features.bits()); self.send_reply_message(&hdr, &msg)?; - self.protocol_features = features; self.update_reply_ack_flag(); } Ok(FrontendReq::SET_PROTOCOL_FEATURES) => { @@ -952,7 +951,6 @@ impl BackendReqHandler { let pflag = VhostUserProtocolFeatures::REPLY_ACK; self.reply_ack_enabled = (self.virtio_features & vflag) != 0 - && self.protocol_features.contains(pflag) && (self.acked_protocol_features & pflag.bits()) != 0; }