[PATCH v1] vhost-user: remove shared object entries during cleanup

[Yuho Choi <dbgh9129@gmail.com>]

SHARED_OBJECT_ADD publishes a borrowed vhost_dev pointer in the global
UUID table. If an exporter goes away without sending SHARED_OBJECT_REMOVE,
vhost_user_backend_cleanup() frees dev->opaque but leaves these entries
behind. A later lookup from another backend can dereference the NULL
opaque pointer, or access a freed vhost_dev if its containing allocation
has also been released.

Remove all entries exported by the device after closing its backend
request channel and before freeing its state. Match both the resource
type and owner pointer so other exporters and dma-buf entries are kept.

Add unit coverage for multiple UUIDs per owner, unrelated resources,
repeated cleanup, UUID reuse, and an uninitialized or empty table.

Fixes: 160947666276 ("vhost-user: add shared_object msg")
Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
---
 hw/display/virtio-dmabuf.c        | 18 +++++++++++++
 hw/virtio/vhost-user.c            |  1 +
 include/hw/virtio/virtio-dmabuf.h |  8 ++++++
 tests/unit/test-virtio-dmabuf.c   | 43 +++++++++++++++++++++++++++++++
 4 files changed, 70 insertions(+)

diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
index 5e0395be77c..636372543a5 100644
--- a/hw/display/virtio-dmabuf.c
+++ b/hw/display/virtio-dmabuf.c
@@ -96,6 +96,24 @@ bool virtio_remove_resource(const QemuUUID *uuid)
     return result;
 }
 
+static gboolean virtio_vhost_device_match(gpointer key, gpointer value,
+                                         gpointer dev)
+{
+    VirtioSharedObject *vso = value;
+
+    return vso->type == TYPE_VHOST_DEV && vso->value == dev;
+}
+
+void virtio_remove_vhost_device(struct vhost_dev *dev)
+{
+    g_mutex_lock(&lock);
+    if (resource_uuids != NULL) {
+        g_hash_table_foreach_remove(resource_uuids, virtio_vhost_device_match,
+                                    dev);
+    }
+    g_mutex_unlock(&lock);
+}
+
 static VirtioSharedObject *get_shared_object(const QemuUUID *uuid)
 {
     gpointer lookup_res = NULL;
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 2881cec72d9..4b32a61a0ba 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2679,6 +2679,7 @@ static int vhost_user_backend_cleanup(struct vhost_dev *dev)
     if (u->backend_sioc) {
         close_backend_channel(u);
     }
+    virtio_remove_vhost_device(dev);
     g_free(u->region_rb);
     u->region_rb = NULL;
     g_free(u->region_rb_offset);
diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
index 627c3b6db79..a1ad362c379 100644
--- a/include/hw/virtio/virtio-dmabuf.h
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -65,6 +65,14 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev);
  */
 bool virtio_remove_resource(const QemuUUID *uuid);
 
+/**
+ * virtio_remove_vhost_device() - Remove a vhost device's exported resources
+ * @dev: the exporter whose entries are to be removed
+ *
+ * The caller must remove the entries before cleaning up the device.
+ */
+void virtio_remove_vhost_device(struct vhost_dev *dev);
+
 /**
  * virtio_lookup_dmabuf() - Looks for a dma-buf resource in the lookup table
  * @uuid: resource's UUID
diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
index a45ec52f421..395ea6f464d 100644
--- a/tests/unit/test-virtio-dmabuf.c
+++ b/tests/unit/test-virtio-dmabuf.c
@@ -22,6 +22,47 @@
 #include "hw/virtio/virtio-dmabuf.h"
 
 
+static void test_remove_vhost_device(void)
+{
+    struct vhost_dev dev = { 0 }, other = { 0 };
+    QemuUUID uuids[2], other_uuid, dmabuf_uuid;
+    int i;
+
+    /* Also allow cleanup before any resources have been registered. */
+    virtio_remove_vhost_device(&dev);
+
+    for (i = 0; i < ARRAY_SIZE(uuids); i++) {
+        qemu_uuid_generate(&uuids[i]);
+        g_assert_true(virtio_add_vhost_device(&uuids[i], &dev));
+    }
+    qemu_uuid_generate(&other_uuid);
+    g_assert_true(virtio_add_vhost_device(&other_uuid, &other));
+    qemu_uuid_generate(&dmabuf_uuid);
+    g_assert_true(virtio_add_dmabuf(&dmabuf_uuid, 3));
+
+    virtio_remove_vhost_device(&dev);
+    for (i = 0; i < ARRAY_SIZE(uuids); i++) {
+        g_assert_null(virtio_lookup_vhost_device(&uuids[i]));
+        g_assert_cmpint(virtio_object_type(&uuids[i]), ==, TYPE_INVALID);
+    }
+
+    /* Repeated cleanup must preserve unrelated resources. */
+    virtio_remove_vhost_device(&dev);
+    g_assert_true(virtio_lookup_vhost_device(&other_uuid) == &other);
+    g_assert_cmpint(virtio_lookup_dmabuf(&dmabuf_uuid), ==, 3);
+
+    /* Removed UUIDs can be exported by another device. */
+    g_assert_true(virtio_add_vhost_device(&uuids[0], &other));
+    virtio_remove_vhost_device(&other);
+    g_assert_null(virtio_lookup_vhost_device(&uuids[0]));
+    g_assert_null(virtio_lookup_vhost_device(&other_uuid));
+    g_assert_true(virtio_remove_resource(&dmabuf_uuid));
+
+    virtio_remove_vhost_device(&dev);
+    virtio_free_resources();
+    virtio_remove_vhost_device(&dev);
+}
+
 static void test_add_remove_resources(void)
 {
     QemuUUID uuid;
@@ -125,6 +166,8 @@ static void test_free_resources(void)
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
+    g_test_add_func("/virtio-dmabuf/remove_vhost_device",
+                    test_remove_vhost_device);
     g_test_add_func("/virtio-dmabuf/add_rm_res", test_add_remove_resources);
     g_test_add_func("/virtio-dmabuf/add_rm_dev", test_add_remove_dev);
     g_test_add_func("/virtio-dmabuf/rm_invalid_res",

base-commit: 1df256f5968e9f7c3c4533a1383b071c044a36d6
-- 
2.43.0