[PATCH v3 0/3] Virtio dmabuf improvements

[PATCH v3 0/3] Virtio dmabuf improvements

[Albert Esteve]

v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg1005257.html
v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg1014615.html
v2 -> v3
  - Documented the new owner check for shared object removal
  - Updated test function names error in the last patch

Various improvements for the virtio-dmabuf module.
This patch includes:

- Check for ownership before allowing a vhost device
  to remove an object from the table.
- Properly cleanup shared resources if a vhost device
  object gets cleaned up.
- Rename virtio dmabuf functions to `virtio_dmabuf_*`

Albert Esteve (3):
  hw/virtio: check owner for removing objects
  hw/virtio: cleanup shared resources
  hw/virtio: rename virtio dmabuf API

 docs/interop/vhost-user.rst       |  4 +-
 hw/display/virtio-dmabuf.c        | 36 ++++++++++++---
 hw/virtio/vhost-user.c            | 31 ++++++++++---
 hw/virtio/vhost.c                 |  3 ++
 include/hw/virtio/virtio-dmabuf.h | 43 ++++++++++-------
 tests/unit/test-virtio-dmabuf.c   | 77 ++++++++++++++++++++++---------
 6 files changed, 141 insertions(+), 53 deletions(-)

-- 
2.43.0

[PATCH v3 1/3] hw/virtio: check owner for removing objects

[Albert Esteve]

Shared objects lack spoofing protection.
For VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE messages
received by the vhost-user interface, any backend was
allowed to remove entries from the shared table just
by knowing the UUID. Only the owner of the entry
shall be allowed to removed their resources
from the table.

To fix that, add a check for all
*SHARED_OBJECT_REMOVE messages received.
A vhost device can only remove TYPE_VHOST_DEV
entries that are owned by them, otherwise skip
the removal, and inform the device that the entry
has not been removed in the answer.

Signed-off-by: Albert Esteve <aesteve@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 docs/interop/vhost-user.rst |  4 +++-
 hw/virtio/vhost-user.c      | 21 +++++++++++++++++++--
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 9f1103f85a..60ec2c9d48 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -1839,7 +1839,9 @@ is sent by the front-end.
   When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
   feature has been successfully negotiated, this message can be submitted
   by the backend to remove themselves from to the virtio-dmabuf shared
-  table API. The shared table will remove the back-end device associated with
+  table API. Only the back-end owning the entry (i.e., the one that first added
+  it) will have permission to remove it. Otherwise, the message is ignored.
+  The shared table will remove the back-end device associated with
   the UUID. If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is negotiated, and the
   back-end sets the ``VHOST_USER_NEED_REPLY`` flag, the front-end must respond
   with zero when operation is successfully completed, or non-zero otherwise.
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index f214df804b..1c3f2357be 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1611,11 +1611,27 @@ vhost_user_backend_handle_shared_object_add(struct vhost_dev *dev,
 }
 
 static int
-vhost_user_backend_handle_shared_object_remove(VhostUserShared *object)
+vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
+                                               VhostUserShared *object)
 {
     QemuUUID uuid;
 
     memcpy(uuid.data, object->uuid, sizeof(object->uuid));
+    switch (virtio_object_type(&uuid)) {
+    case TYPE_VHOST_DEV:
+    {
+        struct vhost_dev *owner = virtio_lookup_vhost_device(&uuid);
+        if (owner == NULL || dev != owner) {
+            /* Not allowed to remove non-owned entries */
+            return 0;
+        }
+        break;
+    }
+    default:
+        /* Not allowed to remove non-owned entries */
+        return 0;
+    }
+
     return virtio_remove_resource(&uuid);
 }
 
@@ -1794,7 +1810,8 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
         ret = vhost_user_backend_handle_shared_object_add(dev, &payload.object);
         break;
     case VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE:
-        ret = vhost_user_backend_handle_shared_object_remove(&payload.object);
+        ret = vhost_user_backend_handle_shared_object_remove(dev,
+                                                             &payload.object);
         break;
     case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
         ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
-- 
2.43.0

[PATCH v3 2/3] hw/virtio: cleanup shared resources

[Albert Esteve]

Ensure that we cleanup all virtio shared
resources when the vhost devices is cleaned
up (after a hot unplug, or a crash).

To do so, we add a new function to the virtio_dmabuf
API called `virtio_dmabuf_vhost_cleanup`, which
loop through the table and removes all
resources owned by the vhost device parameter.

Also, add a test to verify that the new
function in the API behaves as expected.

Signed-off-by: Albert Esteve <aesteve@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/display/virtio-dmabuf.c        | 22 +++++++++++++++++++++
 hw/virtio/vhost.c                 |  3 +++
 include/hw/virtio/virtio-dmabuf.h | 10 ++++++++++
 tests/unit/test-virtio-dmabuf.c   | 33 +++++++++++++++++++++++++++++++
 4 files changed, 68 insertions(+)

diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
index 3dba4577ca..6688809777 100644
--- a/hw/display/virtio-dmabuf.c
+++ b/hw/display/virtio-dmabuf.c
@@ -136,6 +136,28 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid)
     return vso->type;
 }
 
+static bool virtio_dmabuf_resource_is_owned(gpointer key,
+                                            gpointer value,
+                                            gpointer dev)
+{
+    VirtioSharedObject *vso;
+
+    vso = (VirtioSharedObject *) value;
+    return vso->type == TYPE_VHOST_DEV && vso->value == dev;
+}
+
+int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev)
+{
+    int num_removed;
+
+    g_mutex_lock(&lock);
+    num_removed = g_hash_table_foreach_remove(
+        resource_uuids, (GHRFunc) virtio_dmabuf_resource_is_owned, dev);
+    g_mutex_unlock(&lock);
+
+    return num_removed;
+}
+
 void virtio_free_resources(void)
 {
     g_mutex_lock(&lock);
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2c9ac79468..c5622eac14 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -16,6 +16,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/virtio/vhost.h"
+#include "hw/virtio/virtio-dmabuf.h"
 #include "qemu/atomic.h"
 #include "qemu/range.h"
 #include "qemu/error-report.h"
@@ -1599,6 +1600,8 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
     migrate_del_blocker(&hdev->migration_blocker);
     g_free(hdev->mem);
     g_free(hdev->mem_sections);
+    /* free virtio shared objects */
+    virtio_dmabuf_vhost_cleanup(hdev);
     if (hdev->vhost_ops) {
         hdev->vhost_ops->vhost_backend_cleanup(hdev);
     }
diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
index 627c3b6db7..73f70fb482 100644
--- a/include/hw/virtio/virtio-dmabuf.h
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -91,6 +91,16 @@ struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid);
  */
 SharedObjectType virtio_object_type(const QemuUUID *uuid);
 
+/**
+ * virtio_dmabuf_vhost_cleanup() - Destroys all entries of the shared
+ * resources lookup table that are owned by the vhost backend
+ * @dev: the pointer to the vhost device that owns the entries. Data is owned
+ *       by the called of the function.
+ * 
+ * Return: the number of resource entries removed.
+ */
+int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev);
+
 /**
  * virtio_free_resources() - Destroys all keys and values of the shared
  * resources lookup table, and frees them
diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
index a45ec52f42..1c8123c2d2 100644
--- a/tests/unit/test-virtio-dmabuf.c
+++ b/tests/unit/test-virtio-dmabuf.c
@@ -103,6 +103,38 @@ static void test_add_invalid_resource(void)
     }
 }
 
+static void test_cleanup_res(void)
+{
+    QemuUUID uuids[20], uuid_alt;
+    struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
+    struct vhost_dev *dev_alt = g_new0(struct vhost_dev, 1);
+    int i, num_removed;
+
+    for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
+        qemu_uuid_generate(&uuids[i]);
+        virtio_add_vhost_device(&uuids[i], dev);
+        /* vhost device is found */
+        g_assert(virtio_lookup_vhost_device(&uuids[i]) != NULL);
+    }
+    qemu_uuid_generate(&uuid_alt);
+    virtio_add_vhost_device(&uuid_alt, dev_alt);
+    /* vhost device is found */
+    g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
+    /* cleanup all dev resources */
+    num_removed = virtio_dmabuf_vhost_cleanup(dev);
+    g_assert_cmpint(num_removed, ==, ARRAY_SIZE(uuids));
+    for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
+        /* None of the dev resources is found after free'd */
+        g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, -1);
+    }
+    /* uuid_alt is still in the hash table */
+    g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
+
+    virtio_free_resources();
+    g_free(dev);
+    g_free(dev_alt);
+}
+
 static void test_free_resources(void)
 {
     QemuUUID uuids[20];
@@ -131,6 +163,7 @@ int main(int argc, char **argv)
                     test_remove_invalid_resource);
     g_test_add_func("/virtio-dmabuf/add_invalid_res",
                     test_add_invalid_resource);
+    g_test_add_func("/virtio-dmabuf/cleanup_dev", test_cleanup_res);
     g_test_add_func("/virtio-dmabuf/free_res", test_free_resources);
 
     return g_test_run();
-- 
2.43.0

[PATCH v3 3/3] hw/virtio: rename virtio dmabuf API

[Albert Esteve]

Functions in the virtio-dmabuf module
start with 'virtio_*', which is too
generic and may not correctly identify
them as part of the virtio dmabuf API.

Rename all functions to 'virtio_dmabuf_*'
instead to avoid confusion.

Signed-off-by: Albert Esteve <aesteve@redhat.com>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/display/virtio-dmabuf.c        | 14 ++++----
 hw/virtio/vhost-user.c            | 14 ++++----
 include/hw/virtio/virtio-dmabuf.h | 33 +++++++++---------
 tests/unit/test-virtio-dmabuf.c   | 58 +++++++++++++++----------------
 4 files changed, 60 insertions(+), 59 deletions(-)

diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
index 6688809777..42495f87ec 100644
--- a/hw/display/virtio-dmabuf.c
+++ b/hw/display/virtio-dmabuf.c
@@ -48,7 +48,7 @@ static bool virtio_add_resource(QemuUUID *uuid, VirtioSharedObject *value)
     return result;
 }
 
-bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
+bool virtio_dmabuf_add(QemuUUID *uuid, int udmabuf_fd)
 {
     bool result;
     VirtioSharedObject *vso;
@@ -66,7 +66,7 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
     return result;
 }
 
-bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
+bool virtio_dmabuf_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
 {
     bool result;
     VirtioSharedObject *vso;
@@ -84,7 +84,7 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
     return result;
 }
 
-bool virtio_remove_resource(const QemuUUID *uuid)
+bool virtio_dmabuf_remove_resource(const QemuUUID *uuid)
 {
     bool result;
     g_mutex_lock(&lock);
@@ -107,7 +107,7 @@ static VirtioSharedObject *get_shared_object(const QemuUUID *uuid)
     return (VirtioSharedObject *) lookup_res;
 }
 
-int virtio_lookup_dmabuf(const QemuUUID *uuid)
+int virtio_dmabuf_lookup(const QemuUUID *uuid)
 {
     VirtioSharedObject *vso = get_shared_object(uuid);
     if (vso == NULL) {
@@ -117,7 +117,7 @@ int virtio_lookup_dmabuf(const QemuUUID *uuid)
     return GPOINTER_TO_INT(vso->value);
 }
 
-struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
+struct vhost_dev *virtio_dmabuf_lookup_vhost_device(const QemuUUID *uuid)
 {
     VirtioSharedObject *vso = get_shared_object(uuid);
     if (vso == NULL) {
@@ -127,7 +127,7 @@ struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
     return (struct vhost_dev *) vso->value;
 }
 
-SharedObjectType virtio_object_type(const QemuUUID *uuid)
+SharedObjectType virtio_dmabuf_object_type(const QemuUUID *uuid)
 {
     VirtioSharedObject *vso = get_shared_object(uuid);
     if (vso == NULL) {
@@ -158,7 +158,7 @@ int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev)
     return num_removed;
 }
 
-void virtio_free_resources(void)
+void virtio_dmabuf_free_resources(void)
 {
     g_mutex_lock(&lock);
     g_hash_table_destroy(resource_uuids);
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 1c3f2357be..2ab9e13f9e 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1607,7 +1607,7 @@ vhost_user_backend_handle_shared_object_add(struct vhost_dev *dev,
     QemuUUID uuid;
 
     memcpy(uuid.data, object->uuid, sizeof(object->uuid));
-    return virtio_add_vhost_device(&uuid, dev);
+    return virtio_dmabuf_add_vhost_device(&uuid, dev);
 }
 
 static int
@@ -1617,10 +1617,10 @@ vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
     QemuUUID uuid;
 
     memcpy(uuid.data, object->uuid, sizeof(object->uuid));
-    switch (virtio_object_type(&uuid)) {
+    switch (virtio_dmabuf_object_type(&uuid)) {
     case TYPE_VHOST_DEV:
     {
-        struct vhost_dev *owner = virtio_lookup_vhost_device(&uuid);
+        struct vhost_dev *owner = virtio_dmabuf_lookup_vhost_device(&uuid);
         if (owner == NULL || dev != owner) {
             /* Not allowed to remove non-owned entries */
             return 0;
@@ -1632,7 +1632,7 @@ vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
         return 0;
     }
 
-    return virtio_remove_resource(&uuid);
+    return virtio_dmabuf_remove_resource(&uuid);
 }
 
 static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
@@ -1710,13 +1710,13 @@ vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
     memcpy(uuid.data, payload->object.uuid, sizeof(payload->object.uuid));
 
     payload->u64 = 0;
-    switch (virtio_object_type(&uuid)) {
+    switch (virtio_dmabuf_object_type(&uuid)) {
     case TYPE_DMABUF:
-        dmabuf_fd = virtio_lookup_dmabuf(&uuid);
+        dmabuf_fd = virtio_dmabuf_lookup(&uuid);
         break;
     case TYPE_VHOST_DEV:
     {
-        struct vhost_dev *dev = virtio_lookup_vhost_device(&uuid);
+        struct vhost_dev *dev = virtio_dmabuf_lookup_vhost_device(&uuid);
         if (dev == NULL) {
             payload->u64 = -EINVAL;
             break;
diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
index 73f70fb482..186a18a33b 100644
--- a/include/hw/virtio/virtio-dmabuf.h
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -28,7 +28,7 @@ typedef struct VirtioSharedObject {
 } VirtioSharedObject;
 
 /**
- * virtio_add_dmabuf() - Add a new dma-buf resource to the lookup table
+ * virtio_dmabuf_add() - Add a new dma-buf resource to the lookup table
  * @uuid: new resource's UUID
  * @dmabuf_fd: the dma-buf descriptor that will be stored and shared with
  *             other virtio devices. The caller retains ownership over the
@@ -41,11 +41,11 @@ typedef struct VirtioSharedObject {
  * Note that if it finds a repeated UUID, the resource is not inserted in
  * the lookup table.
  */
-bool virtio_add_dmabuf(QemuUUID *uuid, int dmabuf_fd);
+bool virtio_dmabuf_add(QemuUUID *uuid, int dmabuf_fd);
 
 /**
- * virtio_add_vhost_device() - Add a new exporter vhost device that holds the
- * resource with the associated UUID
+ * virtio_dmabuf_add_vhost_device() - Add a new exporter vhost device that
+ * holds the resource with the associated UUID
  * @uuid: new resource's UUID
  * @dev: the pointer to the vhost device that holds the resource. The caller
  *       retains ownership over the device struct and its lifecycle.
@@ -55,41 +55,42 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int dmabuf_fd);
  * Note that if it finds a repeated UUID, the resource is not inserted in
  * the lookup table.
  */
-bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev);
+bool virtio_dmabuf_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev);
 
 /**
- * virtio_remove_resource() - Removes a resource from the lookup table
+ * virtio_dmabuf_remove_resource() - Removes a resource from the lookup table
  * @uuid: resource's UUID
  *
  * Return: true if the UUID has been found and removed from the lookup table.
  */
-bool virtio_remove_resource(const QemuUUID *uuid);
+bool virtio_dmabuf_remove_resource(const QemuUUID *uuid);
 
 /**
- * virtio_lookup_dmabuf() - Looks for a dma-buf resource in the lookup table
+ * virtio_dmabuf_lookup() - Looks for a dma-buf resource in the lookup table
  * @uuid: resource's UUID
  *
  * Return: the dma-buf file descriptor integer, or -1 if the key is not found.
  */
-int virtio_lookup_dmabuf(const QemuUUID *uuid);
+int virtio_dmabuf_lookup(const QemuUUID *uuid);
 
 /**
- * virtio_lookup_vhost_device() - Looks for an exporter vhost device in the
- * lookup table
+ * virtio_dmabuf_lookup_vhost_device() - Looks for an exporter vhost device
+ * in the lookup table
  * @uuid: resource's UUID
  *
  * Return: pointer to the vhost_dev struct, or NULL if the key is not found.
  */
-struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid);
+struct vhost_dev *virtio_dmabuf_lookup_vhost_device(const QemuUUID *uuid);
 
 /**
- * virtio_object_type() - Looks for the type of resource in the lookup table
+ * virtio_dmabuf_object_type() - Looks for the type of resource in the
+ * lookup table
  * @uuid: resource's UUID
  *
  * Return: the type of resource associated with the UUID, or TYPE_INVALID if
  * the key is not found.
  */
-SharedObjectType virtio_object_type(const QemuUUID *uuid);
+SharedObjectType virtio_dmabuf_object_type(const QemuUUID *uuid);
 
 /**
  * virtio_dmabuf_vhost_cleanup() - Destroys all entries of the shared
@@ -102,9 +103,9 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid);
 int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev);
 
 /**
- * virtio_free_resources() - Destroys all keys and values of the shared
+ * virtio_dmabuf_free_resources() - Destroys all keys and values of the shared
  * resources lookup table, and frees them
  */
-void virtio_free_resources(void);
+void virtio_dmabuf_free_resources(void);
 
 #endif /* VIRTIO_DMABUF_H */
diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
index 1c8123c2d2..4e67c1e67b 100644
--- a/tests/unit/test-virtio-dmabuf.c
+++ b/tests/unit/test-virtio-dmabuf.c
@@ -31,12 +31,12 @@ static void test_add_remove_resources(void)
         qemu_uuid_generate(&uuid);
         dmabuf_fd = g_random_int_range(3, 500);
         /* Add a new resource */
-        g_assert(virtio_add_dmabuf(&uuid, dmabuf_fd));
-        g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, dmabuf_fd);
+        g_assert(virtio_dmabuf_add(&uuid, dmabuf_fd));
+        g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, dmabuf_fd);
         /* Remove the resource */
-        g_assert(virtio_remove_resource(&uuid));
+        g_assert(virtio_dmabuf_remove_resource(&uuid));
         /* Resource is not found anymore */
-        g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
+        g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, -1);
     }
 }
 
@@ -48,13 +48,13 @@ static void test_add_remove_dev(void)
 
     for (i = 0; i < 100; ++i) {
         qemu_uuid_generate(&uuid);
-        virtio_add_vhost_device(&uuid, dev);
+        virtio_dmabuf_add_vhost_device(&uuid, dev);
         /* vhost device is found */
-        g_assert(virtio_lookup_vhost_device(&uuid) != NULL);
+        g_assert(virtio_dmabuf_lookup_vhost_device(&uuid) != NULL);
         /* Remove the vhost device */
-        g_assert(virtio_remove_resource(&uuid));
+        g_assert(virtio_dmabuf_remove_resource(&uuid));
         /* vhost device is not found anymore */
-        g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
+        g_assert(virtio_dmabuf_lookup_vhost_device(&uuid) == NULL);
     }
     g_free(dev);
 }
@@ -66,9 +66,9 @@ static void test_remove_invalid_resource(void)
 
     for (i = 0; i < 20; ++i) {
         qemu_uuid_generate(&uuid);
-        g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
+        g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, -1);
         /* Removing a resource that does not exist returns false */
-        g_assert_false(virtio_remove_resource(&uuid));
+        g_assert_false(virtio_dmabuf_remove_resource(&uuid));
     }
 }
 
@@ -81,25 +81,25 @@ static void test_add_invalid_resource(void)
     for (i = 0; i < 20; ++i) {
         qemu_uuid_generate(&uuid);
         /* Add a new resource with invalid (negative) resource fd */
-        g_assert_false(virtio_add_dmabuf(&uuid, dmabuf_fd));
+        g_assert_false(virtio_dmabuf_add(&uuid, dmabuf_fd));
         /* Resource is not found */
-        g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
+        g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, -1);
         /* Add a new vhost device with invalid (NULL) pointer */
-        g_assert_false(virtio_add_vhost_device(&uuid, dev));
+        g_assert_false(virtio_dmabuf_add_vhost_device(&uuid, dev));
         /* vhost device is not found */
-        g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
+        g_assert(virtio_dmabuf_lookup_vhost_device(&uuid) == NULL);
     }
 
     for (i = 0; i < 20; ++i) {
         /* Add a valid resource */
         qemu_uuid_generate(&uuid);
         dmabuf_fd = g_random_int_range(3, 500);
-        g_assert(virtio_add_dmabuf(&uuid, dmabuf_fd));
-        g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, dmabuf_fd);
+        g_assert(virtio_dmabuf_add(&uuid, dmabuf_fd));
+        g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, dmabuf_fd);
         /* Add a new resource with repeated uuid returns false */
-        g_assert_false(virtio_add_dmabuf(&uuid, alt_dmabuf));
+        g_assert_false(virtio_dmabuf_add(&uuid, alt_dmabuf));
         /* The value for the uuid key is not replaced */
-        g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, dmabuf_fd);
+        g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, dmabuf_fd);
     }
 }
 
@@ -112,25 +112,25 @@ static void test_cleanup_res(void)
 
     for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
         qemu_uuid_generate(&uuids[i]);
-        virtio_add_vhost_device(&uuids[i], dev);
+        virtio_dmabuf_add_vhost_device(&uuids[i], dev);
         /* vhost device is found */
-        g_assert(virtio_lookup_vhost_device(&uuids[i]) != NULL);
+        g_assert(virtio_dmabuf_lookup_vhost_device(&uuids[i]) != NULL);
     }
     qemu_uuid_generate(&uuid_alt);
-    virtio_add_vhost_device(&uuid_alt, dev_alt);
+    virtio_dmabuf_add_vhost_device(&uuid_alt, dev_alt);
     /* vhost device is found */
-    g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
+    g_assert(virtio_dmabuf_lookup_vhost_device(&uuid_alt) != NULL);
     /* cleanup all dev resources */
     num_removed = virtio_dmabuf_vhost_cleanup(dev);
     g_assert_cmpint(num_removed, ==, ARRAY_SIZE(uuids));
     for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
         /* None of the dev resources is found after free'd */
-        g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, -1);
+        g_assert_cmpint(virtio_dmabuf_lookup(&uuids[i]), ==, -1);
     }
     /* uuid_alt is still in the hash table */
-    g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
+    g_assert(virtio_dmabuf_lookup_vhost_device(&uuid_alt) != NULL);
 
-    virtio_free_resources();
+    virtio_dmabuf_free_resources();
     g_free(dev);
     g_free(dev_alt);
 }
@@ -143,13 +143,13 @@ static void test_free_resources(void)
     for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
         qemu_uuid_generate(&uuids[i]);
         dmabuf_fd = g_random_int_range(3, 500);
-        g_assert(virtio_add_dmabuf(&uuids[i], dmabuf_fd));
-        g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, dmabuf_fd);
+        g_assert(virtio_dmabuf_add(&uuids[i], dmabuf_fd));
+        g_assert_cmpint(virtio_dmabuf_lookup(&uuids[i]), ==, dmabuf_fd);
     }
-    virtio_free_resources();
+    virtio_dmabuf_free_resources();
     for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
         /* None of the resources is found after free'd */
-        g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, -1);
+        g_assert_cmpint(virtio_dmabuf_lookup(&uuids[i]), ==, -1);
     }
 
 }
-- 
2.43.0

Re: [PATCH v3 3/3] hw/virtio: rename virtio dmabuf API

[Philippe Mathieu-Daudé]

On 9/1/24 13:56, Albert Esteve wrote:
> Functions in the virtio-dmabuf module
> start with 'virtio_*', which is too
> generic and may not correctly identify
> them as part of the virtio dmabuf API.
> 
> Rename all functions to 'virtio_dmabuf_*'
> instead to avoid confusion.
> 
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   hw/display/virtio-dmabuf.c        | 14 ++++----
>   hw/virtio/vhost-user.c            | 14 ++++----
>   include/hw/virtio/virtio-dmabuf.h | 33 +++++++++---------
>   tests/unit/test-virtio-dmabuf.c   | 58 +++++++++++++++----------------
>   4 files changed, 60 insertions(+), 59 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 0/3] Virtio dmabuf improvements

[Albert Esteve]

[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]

Friendly reminder & bump

Is this series waiting to be picked up, or is there anything left to do?

BR,
Albert




On Tue, Jan 9, 2024 at 1:56 PM Albert Esteve <aesteve@redhat.com> wrote:

> v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg1005257.html
> v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg1014615.html
> v2 -> v3
>   - Documented the new owner check for shared object removal
>   - Updated test function names error in the last patch
>
> Various improvements for the virtio-dmabuf module.
> This patch includes:
>
> - Check for ownership before allowing a vhost device
>   to remove an object from the table.
> - Properly cleanup shared resources if a vhost device
>   object gets cleaned up.
> - Rename virtio dmabuf functions to `virtio_dmabuf_*`
>
> Albert Esteve (3):
>   hw/virtio: check owner for removing objects
>   hw/virtio: cleanup shared resources
>   hw/virtio: rename virtio dmabuf API
>
>  docs/interop/vhost-user.rst       |  4 +-
>  hw/display/virtio-dmabuf.c        | 36 ++++++++++++---
>  hw/virtio/vhost-user.c            | 31 ++++++++++---
>  hw/virtio/vhost.c                 |  3 ++
>  include/hw/virtio/virtio-dmabuf.h | 43 ++++++++++-------
>  tests/unit/test-virtio-dmabuf.c   | 77 ++++++++++++++++++++++---------
>  6 files changed, 141 insertions(+), 53 deletions(-)
>
> --
> 2.43.0
>
>

[-- Attachment #2: Type: text/html, Size: 2313 bytes --]

Re: [PATCH v3 1/3] hw/virtio: check owner for removing objects

[Alex Bennée]

Albert Esteve <aesteve@redhat.com> writes:

> Shared objects lack spoofing protection.
> For VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE messages
> received by the vhost-user interface, any backend was
> allowed to remove entries from the shared table just
> by knowing the UUID. Only the owner of the entry
> shall be allowed to removed their resources
> from the table.

Was this buggy behaviour on the part of the vhost-user daemon?

> To fix that, add a check for all
> *SHARED_OBJECT_REMOVE messages received.
> A vhost device can only remove TYPE_VHOST_DEV
> entries that are owned by them, otherwise skip
> the removal, and inform the device that the entry
> has not been removed in the answer.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  docs/interop/vhost-user.rst |  4 +++-
>  hw/virtio/vhost-user.c      | 21 +++++++++++++++++++--
>  2 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> index 9f1103f85a..60ec2c9d48 100644
> --- a/docs/interop/vhost-user.rst
> +++ b/docs/interop/vhost-user.rst
> @@ -1839,7 +1839,9 @@ is sent by the front-end.
>    When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
>    feature has been successfully negotiated, this message can be submitted
>    by the backend to remove themselves from to the virtio-dmabuf shared
> -  table API. The shared table will remove the back-end device associated with
> +  table API. Only the back-end owning the entry (i.e., the one that first added
> +  it) will have permission to remove it. Otherwise, the message is ignored.
> +  The shared table will remove the back-end device associated with
>    the UUID. If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is negotiated, and the
>    back-end sets the ``VHOST_USER_NEED_REPLY`` flag, the front-end must respond
>    with zero when operation is successfully completed, or non-zero otherwise.
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index f214df804b..1c3f2357be 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1611,11 +1611,27 @@ vhost_user_backend_handle_shared_object_add(struct vhost_dev *dev,
>  }
>  
>  static int
> -vhost_user_backend_handle_shared_object_remove(VhostUserShared *object)
> +vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
> +                                               VhostUserShared *object)
>  {
>      QemuUUID uuid;
>  
>      memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> +    switch (virtio_object_type(&uuid)) {
> +    case TYPE_VHOST_DEV:

It would be nice if we could add a kdoc annotation to SharedObjectType
describing what the various types mean.

> +    {
> +        struct vhost_dev *owner = virtio_lookup_vhost_device(&uuid);
> +        if (owner == NULL || dev != owner) {

I dev is always set dev != owner should also cover the NULL case.
However will we see uuid's that aren't associated with anything?

> +            /* Not allowed to remove non-owned entries */
> +            return 0;
> +        }
> +        break;
> +    }
> +    default:
> +        /* Not allowed to remove non-owned entries */
> +        return 0;
> +    }
> +
>      return virtio_remove_resource(&uuid);
>  }
>  
> @@ -1794,7 +1810,8 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
>          ret = vhost_user_backend_handle_shared_object_add(dev, &payload.object);
>          break;
>      case VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE:
> -        ret = vhost_user_backend_handle_shared_object_remove(&payload.object);
> +        ret = vhost_user_backend_handle_shared_object_remove(dev,
> +                                                             &payload.object);
>          break;
>      case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
>          ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 2/3] hw/virtio: cleanup shared resources

[Alex Bennée]

Albert Esteve <aesteve@redhat.com> writes:

> Ensure that we cleanup all virtio shared
> resources when the vhost devices is cleaned
> up (after a hot unplug, or a crash).
>
> To do so, we add a new function to the virtio_dmabuf
> API called `virtio_dmabuf_vhost_cleanup`, which
> loop through the table and removes all
> resources owned by the vhost device parameter.
>
> Also, add a test to verify that the new
> function in the API behaves as expected.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/display/virtio-dmabuf.c        | 22 +++++++++++++++++++++
>  hw/virtio/vhost.c                 |  3 +++
>  include/hw/virtio/virtio-dmabuf.h | 10 ++++++++++
>  tests/unit/test-virtio-dmabuf.c   | 33 +++++++++++++++++++++++++++++++
>  4 files changed, 68 insertions(+)
>
> diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
> index 3dba4577ca..6688809777 100644
> --- a/hw/display/virtio-dmabuf.c
> +++ b/hw/display/virtio-dmabuf.c
> @@ -136,6 +136,28 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid)
>      return vso->type;
>  }
>  
> +static bool virtio_dmabuf_resource_is_owned(gpointer key,
> +                                            gpointer value,
> +                                            gpointer dev)
> +{
> +    VirtioSharedObject *vso;
> +
> +    vso = (VirtioSharedObject *) value;
> +    return vso->type == TYPE_VHOST_DEV && vso->value == dev;

It's a bit surprising to see vso->value being an anonymous gpointer
rather than the proper type and a bit confusing between value and
vso->value.

> +}
> +
> +int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev)
> +{
> +    int num_removed;
> +
> +    g_mutex_lock(&lock);
> +    num_removed = g_hash_table_foreach_remove(
> +        resource_uuids, (GHRFunc) virtio_dmabuf_resource_is_owned, dev);
> +    g_mutex_unlock(&lock);

I'll note if we used a QemuMutex for the lock we could:

  - use WITH_QEMU_LOCK_GUARD(&lock) { }
  - enable QSP porfiling for the lock

> +
> +    return num_removed;
> +}
> +
>  void virtio_free_resources(void)
>  {
>      g_mutex_lock(&lock);
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 2c9ac79468..c5622eac14 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -16,6 +16,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "hw/virtio/vhost.h"
> +#include "hw/virtio/virtio-dmabuf.h"
>  #include "qemu/atomic.h"
>  #include "qemu/range.h"
>  #include "qemu/error-report.h"
> @@ -1599,6 +1600,8 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
>      migrate_del_blocker(&hdev->migration_blocker);
>      g_free(hdev->mem);
>      g_free(hdev->mem_sections);
> +    /* free virtio shared objects */
> +    virtio_dmabuf_vhost_cleanup(hdev);
>      if (hdev->vhost_ops) {
>          hdev->vhost_ops->vhost_backend_cleanup(hdev);
>      }
> diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
> index 627c3b6db7..73f70fb482 100644
> --- a/include/hw/virtio/virtio-dmabuf.h
> +++ b/include/hw/virtio/virtio-dmabuf.h
> @@ -91,6 +91,16 @@ struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid);
>   */
>  SharedObjectType virtio_object_type(const QemuUUID *uuid);
>  
> +/**
> + * virtio_dmabuf_vhost_cleanup() - Destroys all entries of the shared
> + * resources lookup table that are owned by the vhost backend
> + * @dev: the pointer to the vhost device that owns the entries. Data is owned
> + *       by the called of the function.
> + * 
> + * Return: the number of resource entries removed.
> + */
> +int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev);
> +
>  /**
>   * virtio_free_resources() - Destroys all keys and values of the shared
>   * resources lookup table, and frees them
> diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
> index a45ec52f42..1c8123c2d2 100644
> --- a/tests/unit/test-virtio-dmabuf.c
> +++ b/tests/unit/test-virtio-dmabuf.c
> @@ -103,6 +103,38 @@ static void test_add_invalid_resource(void)
>      }
>  }
>  
> +static void test_cleanup_res(void)
> +{
> +    QemuUUID uuids[20], uuid_alt;
> +    struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
> +    struct vhost_dev *dev_alt = g_new0(struct vhost_dev, 1);
> +    int i, num_removed;
> +
> +    for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
> +        qemu_uuid_generate(&uuids[i]);
> +        virtio_add_vhost_device(&uuids[i], dev);
> +        /* vhost device is found */
> +        g_assert(virtio_lookup_vhost_device(&uuids[i]) != NULL);
> +    }
> +    qemu_uuid_generate(&uuid_alt);
> +    virtio_add_vhost_device(&uuid_alt, dev_alt);
> +    /* vhost device is found */
> +    g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
> +    /* cleanup all dev resources */
> +    num_removed = virtio_dmabuf_vhost_cleanup(dev);
> +    g_assert_cmpint(num_removed, ==, ARRAY_SIZE(uuids));
> +    for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
> +        /* None of the dev resources is found after free'd */
> +        g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, -1);
> +    }
> +    /* uuid_alt is still in the hash table */
> +    g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
> +
> +    virtio_free_resources();
> +    g_free(dev);
> +    g_free(dev_alt);
> +}
> +
>  static void test_free_resources(void)
>  {
>      QemuUUID uuids[20];
> @@ -131,6 +163,7 @@ int main(int argc, char **argv)
>                      test_remove_invalid_resource);
>      g_test_add_func("/virtio-dmabuf/add_invalid_res",
>                      test_add_invalid_resource);
> +    g_test_add_func("/virtio-dmabuf/cleanup_dev", test_cleanup_res);
>      g_test_add_func("/virtio-dmabuf/free_res", test_free_resources);
>  
>      return g_test_run();

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 1/3] hw/virtio: check owner for removing objects

[Albert Esteve]

[-- Attachment #1: Type: text/plain, Size: 4870 bytes --]

On Mon, Feb 5, 2024 at 1:57 PM Alex Bennée <alex.bennee@linaro.org> wrote:

> Albert Esteve <aesteve@redhat.com> writes:
>
> > Shared objects lack spoofing protection.
> > For VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE messages
> > received by the vhost-user interface, any backend was
> > allowed to remove entries from the shared table just
> > by knowing the UUID. Only the owner of the entry
> > shall be allowed to removed their resources
> > from the table.
>
> Was this buggy behaviour on the part of the vhost-user daemon?
>

Yes, although the feature is not really used yet, and it requires to know
the UUID to be able to exploit it. But yes, any vhost-user backend could
remove any entry.


>
> > To fix that, add a check for all
> > *SHARED_OBJECT_REMOVE messages received.
> > A vhost device can only remove TYPE_VHOST_DEV
> > entries that are owned by them, otherwise skip
> > the removal, and inform the device that the entry
> > has not been removed in the answer.
> >
> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  docs/interop/vhost-user.rst |  4 +++-
> >  hw/virtio/vhost-user.c      | 21 +++++++++++++++++++--
> >  2 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> > index 9f1103f85a..60ec2c9d48 100644
> > --- a/docs/interop/vhost-user.rst
> > +++ b/docs/interop/vhost-user.rst
> > @@ -1839,7 +1839,9 @@ is sent by the front-end.
> >    When the ``VHOST_USER_PROTOCOL_F_SHARED_OBJECT`` protocol
> >    feature has been successfully negotiated, this message can be
> submitted
> >    by the backend to remove themselves from to the virtio-dmabuf shared
> > -  table API. The shared table will remove the back-end device
> associated with
> > +  table API. Only the back-end owning the entry (i.e., the one that
> first added
> > +  it) will have permission to remove it. Otherwise, the message is
> ignored.
> > +  The shared table will remove the back-end device associated with
> >    the UUID. If ``VHOST_USER_PROTOCOL_F_REPLY_ACK`` is negotiated, and
> the
> >    back-end sets the ``VHOST_USER_NEED_REPLY`` flag, the front-end must
> respond
> >    with zero when operation is successfully completed, or non-zero
> otherwise.
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index f214df804b..1c3f2357be 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -1611,11 +1611,27 @@
> vhost_user_backend_handle_shared_object_add(struct vhost_dev *dev,
> >  }
> >
> >  static int
> > -vhost_user_backend_handle_shared_object_remove(VhostUserShared *object)
> > +vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
> > +                                               VhostUserShared *object)
> >  {
> >      QemuUUID uuid;
> >
> >      memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> > +    switch (virtio_object_type(&uuid)) {
> > +    case TYPE_VHOST_DEV:
>
> It would be nice if we could add a kdoc annotation to SharedObjectType
> describing what the various types mean.
>

I can add it.


>
> > +    {
> > +        struct vhost_dev *owner = virtio_lookup_vhost_device(&uuid);
> > +        if (owner == NULL || dev != owner) {
>
> I dev is always set dev != owner should also cover the NULL case.
>

True, I can remove the NULL case from the condition.


> However will we see uuid's that aren't associated with anything?
>

Theoretically, it shouldn't happen. Dmabufs in the host and the guest are
aligned,
and when one buffer is cleaned up it should not be requested anymore, as
the drivers
in the guest are aware. But a vhost-user backend could have buggy/malicious
requests, so worth the check.


>
> > +            /* Not allowed to remove non-owned entries */
> > +            return 0;
> > +        }
> > +        break;
> > +    }
> > +    default:
> > +        /* Not allowed to remove non-owned entries */
> > +        return 0;
> > +    }
> > +
> >      return virtio_remove_resource(&uuid);
> >  }
> >
> > @@ -1794,7 +1810,8 @@ static gboolean backend_read(QIOChannel *ioc,
> GIOCondition condition,
> >          ret = vhost_user_backend_handle_shared_object_add(dev,
> &payload.object);
> >          break;
> >      case VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE:
> > -        ret =
> vhost_user_backend_handle_shared_object_remove(&payload.object);
> > +        ret = vhost_user_backend_handle_shared_object_remove(dev,
> > +
>  &payload.object);
> >          break;
> >      case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
> >          ret =
> vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>
>

[-- Attachment #2: Type: text/html, Size: 6822 bytes --]

Re: [PATCH v3 2/3] hw/virtio: cleanup shared resources

[Albert Esteve]

[-- Attachment #1: Type: text/plain, Size: 6926 bytes --]

On Tue, Feb 6, 2024 at 12:11 AM Alex Bennée <alex.bennee@linaro.org> wrote:

> Albert Esteve <aesteve@redhat.com> writes:
>
> > Ensure that we cleanup all virtio shared
> > resources when the vhost devices is cleaned
> > up (after a hot unplug, or a crash).
> >
> > To do so, we add a new function to the virtio_dmabuf
> > API called `virtio_dmabuf_vhost_cleanup`, which
> > loop through the table and removes all
> > resources owned by the vhost device parameter.
> >
> > Also, add a test to verify that the new
> > function in the API behaves as expected.
> >
> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  hw/display/virtio-dmabuf.c        | 22 +++++++++++++++++++++
> >  hw/virtio/vhost.c                 |  3 +++
> >  include/hw/virtio/virtio-dmabuf.h | 10 ++++++++++
> >  tests/unit/test-virtio-dmabuf.c   | 33 +++++++++++++++++++++++++++++++
> >  4 files changed, 68 insertions(+)
> >
> > diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
> > index 3dba4577ca..6688809777 100644
> > --- a/hw/display/virtio-dmabuf.c
> > +++ b/hw/display/virtio-dmabuf.c
> > @@ -136,6 +136,28 @@ SharedObjectType virtio_object_type(const QemuUUID
> *uuid)
> >      return vso->type;
> >  }
> >
> > +static bool virtio_dmabuf_resource_is_owned(gpointer key,
> > +                                            gpointer value,
> > +                                            gpointer dev)
> > +{
> > +    VirtioSharedObject *vso;
> > +
> > +    vso = (VirtioSharedObject *) value;
> > +    return vso->type == TYPE_VHOST_DEV && vso->value == dev;
>
> It's a bit surprising to see vso->value being an anonymous gpointer
> rather than the proper type and a bit confusing between value and
> vso->value.
>
>
It is the signature required for this to be used with
`g_hash_table_foreach_remove`.
For the naming, the HashMap stores gpointers, that point to
`VirtioSharedObject`, and
these point to the underlying type (stored at `vso->value`). It may sound a
bit confusing,
but is a byproduct of the VirtioSharedObject indirection. Not sure which
names could be
more fit for this, but I'm open to change them.


> > +}
> > +
> > +int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev)
> > +{
> > +    int num_removed;
> > +
> > +    g_mutex_lock(&lock);
> > +    num_removed = g_hash_table_foreach_remove(
> > +        resource_uuids, (GHRFunc) virtio_dmabuf_resource_is_owned, dev);
> > +    g_mutex_unlock(&lock);
>
> I'll note if we used a QemuMutex for the lock we could:
>
>   - use WITH_QEMU_LOCK_GUARD(&lock) { }
>   - enable QSP porfiling for the lock
>
>
Was not aware of these QemuMutex's. I wouldn't mind changing the mutex in
this
file in a different commit.


> > +
> > +    return num_removed;
> > +}
> > +
> >  void virtio_free_resources(void)
> >  {
> >      g_mutex_lock(&lock);
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 2c9ac79468..c5622eac14 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -16,6 +16,7 @@
> >  #include "qemu/osdep.h"
> >  #include "qapi/error.h"
> >  #include "hw/virtio/vhost.h"
> > +#include "hw/virtio/virtio-dmabuf.h"
> >  #include "qemu/atomic.h"
> >  #include "qemu/range.h"
> >  #include "qemu/error-report.h"
> > @@ -1599,6 +1600,8 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
> >      migrate_del_blocker(&hdev->migration_blocker);
> >      g_free(hdev->mem);
> >      g_free(hdev->mem_sections);
> > +    /* free virtio shared objects */
> > +    virtio_dmabuf_vhost_cleanup(hdev);
> >      if (hdev->vhost_ops) {
> >          hdev->vhost_ops->vhost_backend_cleanup(hdev);
> >      }
> > diff --git a/include/hw/virtio/virtio-dmabuf.h
> b/include/hw/virtio/virtio-dmabuf.h
> > index 627c3b6db7..73f70fb482 100644
> > --- a/include/hw/virtio/virtio-dmabuf.h
> > +++ b/include/hw/virtio/virtio-dmabuf.h
> > @@ -91,6 +91,16 @@ struct vhost_dev *virtio_lookup_vhost_device(const
> QemuUUID *uuid);
> >   */
> >  SharedObjectType virtio_object_type(const QemuUUID *uuid);
> >
> > +/**
> > + * virtio_dmabuf_vhost_cleanup() - Destroys all entries of the shared
> > + * resources lookup table that are owned by the vhost backend
> > + * @dev: the pointer to the vhost device that owns the entries. Data is
> owned
> > + *       by the called of the function.
> > + *
> > + * Return: the number of resource entries removed.
> > + */
> > +int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev);
> > +
> >  /**
> >   * virtio_free_resources() - Destroys all keys and values of the shared
> >   * resources lookup table, and frees them
> > diff --git a/tests/unit/test-virtio-dmabuf.c
> b/tests/unit/test-virtio-dmabuf.c
> > index a45ec52f42..1c8123c2d2 100644
> > --- a/tests/unit/test-virtio-dmabuf.c
> > +++ b/tests/unit/test-virtio-dmabuf.c
> > @@ -103,6 +103,38 @@ static void test_add_invalid_resource(void)
> >      }
> >  }
> >
> > +static void test_cleanup_res(void)
> > +{
> > +    QemuUUID uuids[20], uuid_alt;
> > +    struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
> > +    struct vhost_dev *dev_alt = g_new0(struct vhost_dev, 1);
> > +    int i, num_removed;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
> > +        qemu_uuid_generate(&uuids[i]);
> > +        virtio_add_vhost_device(&uuids[i], dev);
> > +        /* vhost device is found */
> > +        g_assert(virtio_lookup_vhost_device(&uuids[i]) != NULL);
> > +    }
> > +    qemu_uuid_generate(&uuid_alt);
> > +    virtio_add_vhost_device(&uuid_alt, dev_alt);
> > +    /* vhost device is found */
> > +    g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
> > +    /* cleanup all dev resources */
> > +    num_removed = virtio_dmabuf_vhost_cleanup(dev);
> > +    g_assert_cmpint(num_removed, ==, ARRAY_SIZE(uuids));
> > +    for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
> > +        /* None of the dev resources is found after free'd */
> > +        g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, -1);
> > +    }
> > +    /* uuid_alt is still in the hash table */
> > +    g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
> > +
> > +    virtio_free_resources();
> > +    g_free(dev);
> > +    g_free(dev_alt);
> > +}
> > +
> >  static void test_free_resources(void)
> >  {
> >      QemuUUID uuids[20];
> > @@ -131,6 +163,7 @@ int main(int argc, char **argv)
> >                      test_remove_invalid_resource);
> >      g_test_add_func("/virtio-dmabuf/add_invalid_res",
> >                      test_add_invalid_resource);
> > +    g_test_add_func("/virtio-dmabuf/cleanup_dev", test_cleanup_res);
> >      g_test_add_func("/virtio-dmabuf/free_res", test_free_resources);
> >
> >      return g_test_run();
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>
>

[-- Attachment #2: Type: text/html, Size: 9111 bytes --]

Re: [PATCH v3 2/3] hw/virtio: cleanup shared resources

[Alex Bennée]

Albert Esteve <aesteve@redhat.com> writes:

> On Tue, Feb 6, 2024 at 12:11 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>  Albert Esteve <aesteve@redhat.com> writes:
>
>  > Ensure that we cleanup all virtio shared
>  > resources when the vhost devices is cleaned
>  > up (after a hot unplug, or a crash).
>  >
>  > To do so, we add a new function to the virtio_dmabuf
>  > API called `virtio_dmabuf_vhost_cleanup`, which
>  > loop through the table and removes all
>  > resources owned by the vhost device parameter.
>  >
>  > Also, add a test to verify that the new
>  > function in the API behaves as expected.
>  >
>  > Signed-off-by: Albert Esteve <aesteve@redhat.com>
>  > Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>  > ---
>  >  hw/display/virtio-dmabuf.c        | 22 +++++++++++++++++++++
>  >  hw/virtio/vhost.c                 |  3 +++
>  >  include/hw/virtio/virtio-dmabuf.h | 10 ++++++++++
>  >  tests/unit/test-virtio-dmabuf.c   | 33 +++++++++++++++++++++++++++++++
>  >  4 files changed, 68 insertions(+)
>  >
>  > diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
>  > index 3dba4577ca..6688809777 100644
>  > --- a/hw/display/virtio-dmabuf.c
>  > +++ b/hw/display/virtio-dmabuf.c
>  > @@ -136,6 +136,28 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid)
>  >      return vso->type;
>  >  }
>  >  
>  > +static bool virtio_dmabuf_resource_is_owned(gpointer key,
>  > +                                            gpointer value,
>  > +                                            gpointer dev)
>  > +{
>  > +    VirtioSharedObject *vso;
>  > +
>  > +    vso = (VirtioSharedObject *) value;
>  > +    return vso->type == TYPE_VHOST_DEV && vso->value == dev;
>
>  It's a bit surprising to see vso->value being an anonymous gpointer
>  rather than the proper type and a bit confusing between value and
>  vso->value.
>
> It is the signature required for this to be used with `g_hash_table_foreach_remove`.
> For the naming, the HashMap stores gpointers, that point to `VirtioSharedObject`, and
> these point to the underlying type (stored at `vso->value`). It may sound a bit confusing,
> but is a byproduct of the VirtioSharedObject indirection. Not sure which names could be
> more fit for this, but I'm open to change them.

This is the problem without overloading value and vso->value. I
appreciate that virtio_dmabuf_resource_is_owned() has to follow the
signature for g_hash_table_foreach_remove but usually the compare
function then casts gpointer to the underlying type for any comparison.

So something like:

  typedef struct VirtioSharedObject {
      SharedObjectType type;
      union {
            vhost_dev *dev; /* TYPE_VHOST_DEV */
            int udma_buf;   /* TYPE_DMABUF */
      } value;
  } VirtioSharedObject;

and then you would have:

  VirtioSharedObject *vso = value;
  if (vso->type == TYPE_VHOST_DEV) {
     vhost_dev *dev = dev;
     return vso->value.dev == dev;
  }

In fact I think you can skip the cast so have:

  VirtioSharedObject *vso = value;
  return vso->type == TYPE_VHOST_DEV && vso->value.dev == dev;

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 2/3] hw/virtio: cleanup shared resources

[Albert Esteve]

[-- Attachment #1: Type: text/plain, Size: 7437 bytes --]

On Thu, Feb 15, 2024 at 10:45 AM Albert Esteve <aesteve@redhat.com> wrote:

>
>
> On Tue, Feb 6, 2024 at 12:11 AM Alex Bennée <alex.bennee@linaro.org>
> wrote:
>
>> Albert Esteve <aesteve@redhat.com> writes:
>>
>> > Ensure that we cleanup all virtio shared
>> > resources when the vhost devices is cleaned
>> > up (after a hot unplug, or a crash).
>> >
>> > To do so, we add a new function to the virtio_dmabuf
>> > API called `virtio_dmabuf_vhost_cleanup`, which
>> > loop through the table and removes all
>> > resources owned by the vhost device parameter.
>> >
>> > Also, add a test to verify that the new
>> > function in the API behaves as expected.
>> >
>> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
>> > Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > ---
>> >  hw/display/virtio-dmabuf.c        | 22 +++++++++++++++++++++
>> >  hw/virtio/vhost.c                 |  3 +++
>> >  include/hw/virtio/virtio-dmabuf.h | 10 ++++++++++
>> >  tests/unit/test-virtio-dmabuf.c   | 33 +++++++++++++++++++++++++++++++
>> >  4 files changed, 68 insertions(+)
>> >
>> > diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
>> > index 3dba4577ca..6688809777 100644
>> > --- a/hw/display/virtio-dmabuf.c
>> > +++ b/hw/display/virtio-dmabuf.c
>> > @@ -136,6 +136,28 @@ SharedObjectType virtio_object_type(const QemuUUID
>> *uuid)
>> >      return vso->type;
>> >  }
>> >
>> > +static bool virtio_dmabuf_resource_is_owned(gpointer key,
>> > +                                            gpointer value,
>> > +                                            gpointer dev)
>> > +{
>> > +    VirtioSharedObject *vso;
>> > +
>> > +    vso = (VirtioSharedObject *) value;
>> > +    return vso->type == TYPE_VHOST_DEV && vso->value == dev;
>>
>> It's a bit surprising to see vso->value being an anonymous gpointer
>> rather than the proper type and a bit confusing between value and
>> vso->value.
>>
>>
> It is the signature required for this to be used with
> `g_hash_table_foreach_remove`.
> For the naming, the HashMap stores gpointers, that point to
> `VirtioSharedObject`, and
> these point to the underlying type (stored at `vso->value`). It may sound
> a bit confusing,
> but is a byproduct of the VirtioSharedObject indirection. Not sure which
> names could be
> more fit for this, but I'm open to change them.
>
>
>> > +}
>> > +
>> > +int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev)
>> > +{
>> > +    int num_removed;
>> > +
>> > +    g_mutex_lock(&lock);
>> > +    num_removed = g_hash_table_foreach_remove(
>> > +        resource_uuids, (GHRFunc) virtio_dmabuf_resource_is_owned,
>> dev);
>> > +    g_mutex_unlock(&lock);
>>
>> I'll note if we used a QemuMutex for the lock we could:
>>
>>   - use WITH_QEMU_LOCK_GUARD(&lock) { }
>>   - enable QSP porfiling for the lock
>>
>>
> Was not aware of these QemuMutex's. I wouldn't mind changing the mutex in
> this
> file in a different commit.
>

The problem is that current lock is a global static, and `QemuMutex` needs
to be
initialised by doing `qemu_mutex_init(&lock);`.

Maybe can be initialised at vhost-user.c by adding a public function?


>
>
>> > +
>> > +    return num_removed;
>> > +}
>> > +
>> >  void virtio_free_resources(void)
>> >  {
>> >      g_mutex_lock(&lock);
>> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> > index 2c9ac79468..c5622eac14 100644
>> > --- a/hw/virtio/vhost.c
>> > +++ b/hw/virtio/vhost.c
>> > @@ -16,6 +16,7 @@
>> >  #include "qemu/osdep.h"
>> >  #include "qapi/error.h"
>> >  #include "hw/virtio/vhost.h"
>> > +#include "hw/virtio/virtio-dmabuf.h"
>> >  #include "qemu/atomic.h"
>> >  #include "qemu/range.h"
>> >  #include "qemu/error-report.h"
>> > @@ -1599,6 +1600,8 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
>> >      migrate_del_blocker(&hdev->migration_blocker);
>> >      g_free(hdev->mem);
>> >      g_free(hdev->mem_sections);
>> > +    /* free virtio shared objects */
>> > +    virtio_dmabuf_vhost_cleanup(hdev);
>> >      if (hdev->vhost_ops) {
>> >          hdev->vhost_ops->vhost_backend_cleanup(hdev);
>> >      }
>> > diff --git a/include/hw/virtio/virtio-dmabuf.h
>> b/include/hw/virtio/virtio-dmabuf.h
>> > index 627c3b6db7..73f70fb482 100644
>> > --- a/include/hw/virtio/virtio-dmabuf.h
>> > +++ b/include/hw/virtio/virtio-dmabuf.h
>> > @@ -91,6 +91,16 @@ struct vhost_dev *virtio_lookup_vhost_device(const
>> QemuUUID *uuid);
>> >   */
>> >  SharedObjectType virtio_object_type(const QemuUUID *uuid);
>> >
>> > +/**
>> > + * virtio_dmabuf_vhost_cleanup() - Destroys all entries of the shared
>> > + * resources lookup table that are owned by the vhost backend
>> > + * @dev: the pointer to the vhost device that owns the entries. Data
>> is owned
>> > + *       by the called of the function.
>> > + *
>> > + * Return: the number of resource entries removed.
>> > + */
>> > +int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev);
>> > +
>> >  /**
>> >   * virtio_free_resources() - Destroys all keys and values of the shared
>> >   * resources lookup table, and frees them
>> > diff --git a/tests/unit/test-virtio-dmabuf.c
>> b/tests/unit/test-virtio-dmabuf.c
>> > index a45ec52f42..1c8123c2d2 100644
>> > --- a/tests/unit/test-virtio-dmabuf.c
>> > +++ b/tests/unit/test-virtio-dmabuf.c
>> > @@ -103,6 +103,38 @@ static void test_add_invalid_resource(void)
>> >      }
>> >  }
>> >
>> > +static void test_cleanup_res(void)
>> > +{
>> > +    QemuUUID uuids[20], uuid_alt;
>> > +    struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
>> > +    struct vhost_dev *dev_alt = g_new0(struct vhost_dev, 1);
>> > +    int i, num_removed;
>> > +
>> > +    for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
>> > +        qemu_uuid_generate(&uuids[i]);
>> > +        virtio_add_vhost_device(&uuids[i], dev);
>> > +        /* vhost device is found */
>> > +        g_assert(virtio_lookup_vhost_device(&uuids[i]) != NULL);
>> > +    }
>> > +    qemu_uuid_generate(&uuid_alt);
>> > +    virtio_add_vhost_device(&uuid_alt, dev_alt);
>> > +    /* vhost device is found */
>> > +    g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
>> > +    /* cleanup all dev resources */
>> > +    num_removed = virtio_dmabuf_vhost_cleanup(dev);
>> > +    g_assert_cmpint(num_removed, ==, ARRAY_SIZE(uuids));
>> > +    for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
>> > +        /* None of the dev resources is found after free'd */
>> > +        g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, -1);
>> > +    }
>> > +    /* uuid_alt is still in the hash table */
>> > +    g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
>> > +
>> > +    virtio_free_resources();
>> > +    g_free(dev);
>> > +    g_free(dev_alt);
>> > +}
>> > +
>> >  static void test_free_resources(void)
>> >  {
>> >      QemuUUID uuids[20];
>> > @@ -131,6 +163,7 @@ int main(int argc, char **argv)
>> >                      test_remove_invalid_resource);
>> >      g_test_add_func("/virtio-dmabuf/add_invalid_res",
>> >                      test_add_invalid_resource);
>> > +    g_test_add_func("/virtio-dmabuf/cleanup_dev", test_cleanup_res);
>> >      g_test_add_func("/virtio-dmabuf/free_res", test_free_resources);
>> >
>> >      return g_test_run();
>>
>> --
>> Alex Bennée
>> Virtualisation Tech Lead @ Linaro
>>
>>

[-- Attachment #2: Type: text/html, Size: 10053 bytes --]

Re: [PATCH v3 2/3] hw/virtio: cleanup shared resources

[Albert Esteve]

[-- Attachment #1: Type: text/plain, Size: 7816 bytes --]

On Mon, Feb 19, 2024 at 11:45 AM Albert Esteve <aesteve@redhat.com> wrote:

>
>
>
> On Thu, Feb 15, 2024 at 10:45 AM Albert Esteve <aesteve@redhat.com> wrote:
>
>>
>>
>> On Tue, Feb 6, 2024 at 12:11 AM Alex Bennée <alex.bennee@linaro.org>
>> wrote:
>>
>>> Albert Esteve <aesteve@redhat.com> writes:
>>>
>>> > Ensure that we cleanup all virtio shared
>>> > resources when the vhost devices is cleaned
>>> > up (after a hot unplug, or a crash).
>>> >
>>> > To do so, we add a new function to the virtio_dmabuf
>>> > API called `virtio_dmabuf_vhost_cleanup`, which
>>> > loop through the table and removes all
>>> > resources owned by the vhost device parameter.
>>> >
>>> > Also, add a test to verify that the new
>>> > function in the API behaves as expected.
>>> >
>>> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
>>> > Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> > ---
>>> >  hw/display/virtio-dmabuf.c        | 22 +++++++++++++++++++++
>>> >  hw/virtio/vhost.c                 |  3 +++
>>> >  include/hw/virtio/virtio-dmabuf.h | 10 ++++++++++
>>> >  tests/unit/test-virtio-dmabuf.c   | 33 +++++++++++++++++++++++++++++++
>>> >  4 files changed, 68 insertions(+)
>>> >
>>> > diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
>>> > index 3dba4577ca..6688809777 100644
>>> > --- a/hw/display/virtio-dmabuf.c
>>> > +++ b/hw/display/virtio-dmabuf.c
>>> > @@ -136,6 +136,28 @@ SharedObjectType virtio_object_type(const
>>> QemuUUID *uuid)
>>> >      return vso->type;
>>> >  }
>>> >
>>> > +static bool virtio_dmabuf_resource_is_owned(gpointer key,
>>> > +                                            gpointer value,
>>> > +                                            gpointer dev)
>>> > +{
>>> > +    VirtioSharedObject *vso;
>>> > +
>>> > +    vso = (VirtioSharedObject *) value;
>>> > +    return vso->type == TYPE_VHOST_DEV && vso->value == dev;
>>>
>>> It's a bit surprising to see vso->value being an anonymous gpointer
>>> rather than the proper type and a bit confusing between value and
>>> vso->value.
>>>
>>>
>> It is the signature required for this to be used with
>> `g_hash_table_foreach_remove`.
>> For the naming, the HashMap stores gpointers, that point to
>> `VirtioSharedObject`, and
>> these point to the underlying type (stored at `vso->value`). It may sound
>> a bit confusing,
>> but is a byproduct of the VirtioSharedObject indirection. Not sure which
>> names could be
>> more fit for this, but I'm open to change them.
>>
>>
>>> > +}
>>> > +
>>> > +int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev)
>>> > +{
>>> > +    int num_removed;
>>> > +
>>> > +    g_mutex_lock(&lock);
>>> > +    num_removed = g_hash_table_foreach_remove(
>>> > +        resource_uuids, (GHRFunc) virtio_dmabuf_resource_is_owned,
>>> dev);
>>> > +    g_mutex_unlock(&lock);
>>>
>>> I'll note if we used a QemuMutex for the lock we could:
>>>
>>>   - use WITH_QEMU_LOCK_GUARD(&lock) { }
>>>   - enable QSP porfiling for the lock
>>>
>>>
>> Was not aware of these QemuMutex's. I wouldn't mind changing the mutex in
>> this
>> file in a different commit.
>>
>
> The problem is that current lock is a global static, and `QemuMutex` needs
> to be
> initialised by doing `qemu_mutex_init(&lock);`.
>
> Maybe can be initialised at vhost-user.c by adding a public function?
>

I think `virtio_init` at `virtio.c` will be a better candidate.


>
>
>>
>>
>>> > +
>>> > +    return num_removed;
>>> > +}
>>> > +
>>> >  void virtio_free_resources(void)
>>> >  {
>>> >      g_mutex_lock(&lock);
>>> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>> > index 2c9ac79468..c5622eac14 100644
>>> > --- a/hw/virtio/vhost.c
>>> > +++ b/hw/virtio/vhost.c
>>> > @@ -16,6 +16,7 @@
>>> >  #include "qemu/osdep.h"
>>> >  #include "qapi/error.h"
>>> >  #include "hw/virtio/vhost.h"
>>> > +#include "hw/virtio/virtio-dmabuf.h"
>>> >  #include "qemu/atomic.h"
>>> >  #include "qemu/range.h"
>>> >  #include "qemu/error-report.h"
>>> > @@ -1599,6 +1600,8 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
>>> >      migrate_del_blocker(&hdev->migration_blocker);
>>> >      g_free(hdev->mem);
>>> >      g_free(hdev->mem_sections);
>>> > +    /* free virtio shared objects */
>>> > +    virtio_dmabuf_vhost_cleanup(hdev);
>>> >      if (hdev->vhost_ops) {
>>> >          hdev->vhost_ops->vhost_backend_cleanup(hdev);
>>> >      }
>>> > diff --git a/include/hw/virtio/virtio-dmabuf.h
>>> b/include/hw/virtio/virtio-dmabuf.h
>>> > index 627c3b6db7..73f70fb482 100644
>>> > --- a/include/hw/virtio/virtio-dmabuf.h
>>> > +++ b/include/hw/virtio/virtio-dmabuf.h
>>> > @@ -91,6 +91,16 @@ struct vhost_dev *virtio_lookup_vhost_device(const
>>> QemuUUID *uuid);
>>> >   */
>>> >  SharedObjectType virtio_object_type(const QemuUUID *uuid);
>>> >
>>> > +/**
>>> > + * virtio_dmabuf_vhost_cleanup() - Destroys all entries of the shared
>>> > + * resources lookup table that are owned by the vhost backend
>>> > + * @dev: the pointer to the vhost device that owns the entries. Data
>>> is owned
>>> > + *       by the called of the function.
>>> > + *
>>> > + * Return: the number of resource entries removed.
>>> > + */
>>> > +int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev);
>>> > +
>>> >  /**
>>> >   * virtio_free_resources() - Destroys all keys and values of the
>>> shared
>>> >   * resources lookup table, and frees them
>>> > diff --git a/tests/unit/test-virtio-dmabuf.c
>>> b/tests/unit/test-virtio-dmabuf.c
>>> > index a45ec52f42..1c8123c2d2 100644
>>> > --- a/tests/unit/test-virtio-dmabuf.c
>>> > +++ b/tests/unit/test-virtio-dmabuf.c
>>> > @@ -103,6 +103,38 @@ static void test_add_invalid_resource(void)
>>> >      }
>>> >  }
>>> >
>>> > +static void test_cleanup_res(void)
>>> > +{
>>> > +    QemuUUID uuids[20], uuid_alt;
>>> > +    struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
>>> > +    struct vhost_dev *dev_alt = g_new0(struct vhost_dev, 1);
>>> > +    int i, num_removed;
>>> > +
>>> > +    for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
>>> > +        qemu_uuid_generate(&uuids[i]);
>>> > +        virtio_add_vhost_device(&uuids[i], dev);
>>> > +        /* vhost device is found */
>>> > +        g_assert(virtio_lookup_vhost_device(&uuids[i]) != NULL);
>>> > +    }
>>> > +    qemu_uuid_generate(&uuid_alt);
>>> > +    virtio_add_vhost_device(&uuid_alt, dev_alt);
>>> > +    /* vhost device is found */
>>> > +    g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
>>> > +    /* cleanup all dev resources */
>>> > +    num_removed = virtio_dmabuf_vhost_cleanup(dev);
>>> > +    g_assert_cmpint(num_removed, ==, ARRAY_SIZE(uuids));
>>> > +    for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
>>> > +        /* None of the dev resources is found after free'd */
>>> > +        g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, -1);
>>> > +    }
>>> > +    /* uuid_alt is still in the hash table */
>>> > +    g_assert(virtio_lookup_vhost_device(&uuid_alt) != NULL);
>>> > +
>>> > +    virtio_free_resources();
>>> > +    g_free(dev);
>>> > +    g_free(dev_alt);
>>> > +}
>>> > +
>>> >  static void test_free_resources(void)
>>> >  {
>>> >      QemuUUID uuids[20];
>>> > @@ -131,6 +163,7 @@ int main(int argc, char **argv)
>>> >                      test_remove_invalid_resource);
>>> >      g_test_add_func("/virtio-dmabuf/add_invalid_res",
>>> >                      test_add_invalid_resource);
>>> > +    g_test_add_func("/virtio-dmabuf/cleanup_dev", test_cleanup_res);
>>> >      g_test_add_func("/virtio-dmabuf/free_res", test_free_resources);
>>> >
>>> >      return g_test_run();
>>>
>>> --
>>> Alex Bennée
>>> Virtualisation Tech Lead @ Linaro
>>>
>>>

[-- Attachment #2: Type: text/html, Size: 10833 bytes --]