vsock: Resolve deadlock in sibling VM communication

The deadlock occurs when two sibling VMs simultaneously try to send each other packets. The `VhostUserVsockThread`s corresponding to both the VMs hold their own locks while executing `thread_backend.send_pkt` and then try to lock each other to access their counterpart's `raw_pkts_queue`. This ultimately results in a deadlock. Resolved by separating the mutex over `raw_pkts_queue` from the mutex over `VhostUserVsockThread`. Signed-off-by: Priyansh Rathi <[email protected]>
rust-vmm · Jul 7, 2023 · ac9dfe3 · ac9dfe3
1 parent 055f27f
commit ac9dfe3
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 30 deletions.
diff --git a/crates/vsock/src/main.rs b/crates/vsock/src/main.rs
@@ -181,10 +181,6 @@ pub(crate) fn start_backend_server(config: VsockConfig, cid_map: Arc<RwLock<CidM
     loop {
         let backend =
             Arc::new(VhostUserVsockBackend::new(config.clone(), cid_map.clone()).unwrap());
-        cid_map
-            .write()
-            .unwrap()
-            .insert(config.get_guest_cid(), backend.clone());
 
         let listener = Listener::new(config.get_socket_path(), true).unwrap();
 
@@ -220,7 +216,6 @@ pub(crate) fn start_backend_server(config: VsockConfig, cid_map: Arc<RwLock<CidM
 
         // No matter the result, we need to shut down the worker thread.
         backend.exit_event.write(1).unwrap();
-        cid_map.write().unwrap().remove(&config.get_guest_cid());
     }
 }
 
@@ -382,8 +377,7 @@ mod tests {
 
         let cid_map: Arc<RwLock<CidMap>> = Arc::new(RwLock::new(HashMap::new()));
 
-        let backend = Arc::new(VhostUserVsockBackend::new(config, cid_map.clone()).unwrap());
-        cid_map.write().unwrap().insert(CID, backend.clone());
+        let backend = Arc::new(VhostUserVsockBackend::new(config, cid_map).unwrap());
 
         let daemon = VhostUserDaemon::new(
             String::from("vhost-user-vsock"),

diff --git a/crates/vsock/src/thread_backend.rs b/crates/vsock/src/thread_backend.rs
@@ -23,6 +23,8 @@ use crate::{
     vsock_conn::*,
 };
 
+pub(crate) type RawPktsQ = VecDeque<RawVsockPacket>;
+
 pub(crate) struct RawVsockPacket {
     pub header: [u8; PKT_HEADER_SIZE],
     pub data: Vec<u8>,
@@ -65,9 +67,9 @@ pub(crate) struct VsockThreadBackend {
     pub local_port_set: HashSet<u32>,
     tx_buffer_size: u32,
     /// Maps the guest CID to the corresponding backend. Used for sibling VM communication.
-    cid_map: Arc<RwLock<CidMap>>,
+    pub cid_map: Arc<RwLock<CidMap>>,
     /// Queue of raw vsock packets recieved from sibling VMs to be sent to the guest.
-    raw_pkts_queue: VecDeque<RawVsockPacket>,
+    pub raw_pkts_queue: Arc<RwLock<RawPktsQ>>,
 }
 
 impl VsockThreadBackend {
@@ -92,7 +94,7 @@ impl VsockThreadBackend {
             local_port_set: HashSet::new(),
             tx_buffer_size,
             cid_map,
-            raw_pkts_queue: VecDeque::new(),
+            raw_pkts_queue: Arc::new(RwLock::new(VecDeque::new())),
         }
     }
 
@@ -103,7 +105,7 @@ impl VsockThreadBackend {
 
     /// Checks if there are pending raw vsock packets to be sent to the guest.
     pub fn pending_raw_pkts(&self) -> bool {
-        !self.raw_pkts_queue.is_empty()
+        !self.raw_pkts_queue.read().unwrap().is_empty()
     }
 
     /// Deliver a vsock packet to the guest vsock driver.
@@ -178,14 +180,13 @@ impl VsockThreadBackend {
         if dst_cid != VSOCK_HOST_CID {
             let cid_map = self.cid_map.read().unwrap();
             if cid_map.contains_key(&dst_cid) {
-                let sibling_backend = cid_map.get(&dst_cid).unwrap();
-                let mut sibling_backend_thread = sibling_backend.threads[0].lock().unwrap();
+                let (sibling_raw_pkts_queue, sibling_event_fd) = cid_map.get(&dst_cid).unwrap();
 
-                sibling_backend_thread
-                    .thread_backend
-                    .raw_pkts_queue
+                sibling_raw_pkts_queue
+                    .write()
+                    .unwrap()
                     .push_back(RawVsockPacket::from_vsock_packet(pkt)?);
-                let _ = sibling_backend_thread.sibling_event_fd.write(1);
+                let _ = sibling_event_fd.write(1);
             } else {
                 warn!("vsock: dropping packet for unknown cid: {:?}", dst_cid);
             }
@@ -254,6 +255,8 @@ impl VsockThreadBackend {
     pub fn recv_raw_pkt<B: BitmapSlice>(&mut self, pkt: &mut VsockPacket<B>) -> Result<()> {
         let raw_vsock_pkt = self
             .raw_pkts_queue
+            .write()
+            .unwrap()
             .pop_front()
             .ok_or(Error::EmptyRawPktsQueue)?;
 
@@ -423,10 +426,6 @@ mod tests {
 
         let sibling_backend =
             Arc::new(VhostUserVsockBackend::new(sibling_config, cid_map.clone()).unwrap());
-        cid_map
-            .write()
-            .unwrap()
-            .insert(SIBLING_CID, sibling_backend.clone());
 
         let epoll_fd = epoll::create(false).unwrap();
         let mut vtp = VsockThreadBackend::new(

diff --git a/crates/vsock/src/vhu_vsock.rs b/crates/vsock/src/vhu_vsock.rs
@@ -21,9 +21,10 @@ use vmm_sys_util::{
     eventfd::{EventFd, EFD_NONBLOCK},
 };
 
+use crate::thread_backend::RawPktsQ;
 use crate::vhu_vsock_thread::*;
 
-pub(crate) type CidMap = HashMap<u64, Arc<VhostUserVsockBackend>>;
+pub(crate) type CidMap = HashMap<u64, (Arc<RwLock<RawPktsQ>>, EventFd)>;
 
 const NUM_QUEUES: usize = 2;
 const QUEUE_SIZE: usize = 256;

diff --git a/crates/vsock/src/vhu_vsock_thread.rs b/crates/vsock/src/vhu_vsock_thread.rs
@@ -92,21 +92,31 @@ impl VhostUserVsockThread {
 
         let sibling_event_fd = EventFd::new(EFD_NONBLOCK).map_err(Error::EventFdCreate)?;
 
+        let thread_backend = VsockThreadBackend::new(
+            uds_path.clone(),
+            epoll_fd,
+            guest_cid,
+            tx_buffer_size,
+            cid_map.clone(),
+        );
+
+        cid_map.write().unwrap().insert(
+            guest_cid,
+            (
+                thread_backend.raw_pkts_queue.clone(),
+                sibling_event_fd.try_clone().unwrap(),
+            ),
+        );
+
         let thread = VhostUserVsockThread {
             mem: None,
             event_idx: false,
             host_sock: host_sock.as_raw_fd(),
-            host_sock_path: uds_path.clone(),
+            host_sock_path: uds_path,
             host_listener: host_sock,
             vring_worker: None,
             epoll_file,
-            thread_backend: VsockThreadBackend::new(
-                uds_path,
-                epoll_fd,
-                guest_cid,
-                tx_buffer_size,
-                cid_map,
-            ),
+            thread_backend,
             guest_cid,
             pool: ThreadPoolBuilder::new()
                 .pool_size(1)
@@ -660,6 +670,11 @@ impl VhostUserVsockThread {
 impl Drop for VhostUserVsockThread {
     fn drop(&mut self) {
         let _ = std::fs::remove_file(&self.host_sock_path);
+        self.thread_backend
+            .cid_map
+            .write()
+            .unwrap()
+            .remove(&self.guest_cid);
     }
 }
 #[cfg(test)]