On Thu, Sep 10, 2026 at 7:30 AM Yuho Choi <[email protected]> wrote: > > 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 <[email protected]> > --- > 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);
Nit: There is already `virtio_add_vhost_device()` which registers *one* UUID and has a matching virtio_remove_resource(). The function above sweeps every table entry owned by that vhost_device, so it could use a more descriptive name. Maybe something like `virtio_remove_vhost_device_resources()`? Either way, the patch is good shape as is, therefore: Reviewed-by: Albert Esteve <[email protected]> Thanks! Albert > + > /** > * 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 >
