vmbus_server: allow gpadls for revoked channels (#3959)

This change provides a corrected fix for the issue described in #3649.

If a guest sends a GpadlHeader message for a channel that was revoked on
the host, this should be allowed to succeed. The guest likely sent the
message before it received the RescindOffer message, so it is not
expecting failures, and in particular Linux does not handle it well if a
failure does occur here.

This change allows the gpadl to be created, and also lets subsequent
teardown requests to succeed should the guest choose to send them before
releasing the channel.

This matches the existing behavior in Hyper-V.

---------

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Author: SvenGroot

vm/devices/vmbus/vmbus_server/src/channels.rs

@@ -1083,6 +1083,9 @@ impl ChannelList {
     }
 
     /// Gets a channel by guest channel ID.
+    ///
+    /// It is an error to call this function on a channel that has been released
+    /// by the guest, since the guest should not be using that ID anymore.
     fn get_by_channel_id_mut(
         &mut self,
         assigned_channels: &AssignedChannels,
@@ -2741,27 +2744,28 @@ impl<'a, N: 'a + Notifier> ServerWithNotifier<'a, N> {
         Ok(())
     }
 
-    /// Sends a GPADL to the device when `ranges` is Some. Returns false if the
-    /// GPADL should be removed because the channel is already revoked.
-    #[must_use]
-    fn gpadl_updated(
+    /// Sends a GPADL to the device after the full list of ranges was received.
+    fn gpadl_completed(
         mut sender: MessageSender<'_, N>,
         offer_id: OfferId,
         channel: &Channel,
         gpadl_id: GpadlId,
-        gpadl: &Gpadl,
-    ) -> bool {
+        gpadl: &mut Gpadl,
+    ) {
         if channel.state.is_revoked() {
             let channel_id = channel.info.as_ref().expect("assigned").channel_id;
-            sender.send_gpadl_created(channel_id, gpadl_id, protocol::STATUS_UNSUCCESSFUL);
-            false
+
+            // A gpadl for a channel that was revoked but still referenced is
+            // allowed. In this case there is no channel to notify so
+            // immediately send a success response.
+            gpadl.state = GpadlState::Accepted;
+            sender.send_gpadl_created(channel_id, gpadl_id, protocol::STATUS_SUCCESS);
         } else {
-            // Notify the channel if the GPADL is done.
+            // Notify the channel of the completed GPADL.
             sender.notifier.notify(
                 offer_id,
                 Action::Gpadl(gpadl_id, gpadl.count, gpadl.buf.clone()),
             );
-            true
         }
     }
 
@@ -2793,12 +2797,22 @@ impl<'a, N: 'a + Notifier> ServerWithNotifier<'a, N> {
             Entry::Occupied(_) => return Err(ChannelError::DuplicateGpadlId),
         };
 
-        // If we're not done, track the offer ID for GPADL body requests
-        // N.B. The above only checks if the combination of (gpadl_id, offer_id) is unique, which
-        //      allows for a guest to reuse a gpadl ID in use by a reserved channel (which it may
-        //      not know about). But for in-progress GPADLs we need to ensure the gpadl ID itself
-        //      is unique, since the body message doesn't include a channel ID.
-        if !done {
+        if done {
+            Self::gpadl_completed(
+                self.inner
+                    .pending_messages
+                    .sender(self.notifier, self.inner.state.is_paused()),
+                offer_id,
+                channel,
+                input.gpadl_id,
+                gpadl,
+            )
+        } else {
+            // If we're not done, track the offer ID for GPADL body requests
+            // N.B. The above only checks if the combination of (gpadl_id, offer_id) is unique,
+            //      which allows for a guest to reuse a gpadl ID in use by a reserved channel (which
+            //      it may not know about). But for in-progress GPADLs we need to ensure the gpadl
+            //      ID itself is unique, since the body message doesn't include a channel ID.
             match self.inner.incomplete_gpadls.entry(input.gpadl_id) {
                 Entry::Vacant(entry) => {
                     entry.insert(offer_id);
@@ -2815,20 +2829,6 @@ impl<'a, N: 'a + Notifier> ServerWithNotifier<'a, N> {
                 }
             }
         }
-
-        if done
-            && !Self::gpadl_updated(
-                self.inner
-                    .pending_messages
-                    .sender(self.notifier, self.inner.state.is_paused()),
-                offer_id,
-                channel,
-                input.gpadl_id,
-                gpadl,
-            )
-        {
-            self.inner.gpadls.remove(&(input.gpadl_id, offer_id));
-        }
         Ok(())
     }
 
@@ -2881,17 +2881,15 @@ impl<'a, N: 'a + Notifier> ServerWithNotifier<'a, N> {
             Ok(done) => {
                 if done {
                     self.inner.incomplete_gpadls.remove(&input.gpadl_id);
-                    if !Self::gpadl_updated(
+                    Self::gpadl_completed(
                         self.inner
                             .pending_messages
                             .sender(self.notifier, self.inner.state.is_paused()),
                         offer_id,
                         channel,
                         input.gpadl_id,
                         gpadl,
-                    ) {
-                        self.inner.gpadls.remove(&(input.gpadl_id, offer_id));
-                    }
+                    )
                 }
             }
             Err(err) => {

vm/devices/vmbus/vmbus_server/src/tests.rs

@@ -552,6 +552,57 @@ async fn test_pause_resume(spawner: DefaultDriver) {
     assert!(matches!(poll!(channel.request_recv.next()), Poll::Pending));
 }
 
+#[async_test]
+async fn test_gpadl_on_revoked_channel(spawner: DefaultDriver) {
+    // Verify that the guest can create and tear down a GPADL for a channel that has been revoked
+    // by the host but not yet released by the guest.
+    let mut env = TestEnv::new(spawner);
+    let channel = env.offer(1, false).await;
+    env.vmbus.start();
+    env.connect(1, protocol::FeatureFlags::new(), false).await;
+
+    // Revoke the channel from the host side. This sends a RESCIND_CHANNEL_OFFER to the guest, but
+    // leaves the channel in the server's state until the guest releases it.
+    channel
+        .server_request_send
+        .call(ChannelServerRequest::Revoke, ())
+        .await
+        .unwrap();
+    env.expect_response(protocol::MessageType::RESCIND_CHANNEL_OFFER)
+        .await;
+
+    // Create a GPADL for the revoked channel. The server should accept it.
+    env.synic.send_message_core(
+        OutgoingMessage::with_data(
+            &protocol::GpadlHeader {
+                channel_id: ChannelId(1),
+                gpadl_id: GpadlId(10),
+                count: 1,
+                len: 16,
+            },
+            [1u64, 0u64].as_bytes(),
+        ),
+        false,
+    );
+    let created = env.get_response::<protocol::GpadlCreated>().await;
+    assert_eq!(created.channel_id, ChannelId(1));
+    assert_eq!(created.gpadl_id, GpadlId(10));
+    assert_eq!(created.status, protocol::STATUS_SUCCESS);
+
+    // Tear down the GPADL. The server should complete the teardown without notifying the device.
+    env.synic.send_message(protocol::GpadlTeardown {
+        channel_id: ChannelId(1),
+        gpadl_id: GpadlId(10),
+    });
+    env.expect_response(protocol::MessageType::GPADL_TORNDOWN)
+        .await;
+
+    // Release the channel so the server can clean up.
+    env.synic.send_message(protocol::RelIdReleased {
+        channel_id: ChannelId(1),
+    });
+}
+
 struct TestDeviceState {
     id: u32,
     started: bool,