On Tue, 17 Oct 2023 at 04:26, Albert Esteve <aesteve@redhat.com> wrote:
>
> Hi!
>
> Thanks for the patch, and sorry for not noticing the flag had already been assigned. The patch looks good to me!
Hi Albert,
I looked at the shared object code for the first time:
- There are memory leaks in virtio_add_dmabuf() and
virtio_add_vhost_device() when the UUID was added previously.
There is a patch already for fixing the leak:
- The hash table is global and there is no spoofing protection, so
vhost-user devices can hijack known UUIDs. Is it possible to associate
a vhost_dev owner with each shared object and only allow the owner to
remove it?
True, it is unprotected from another backend to remove an entry without ownership.
It sounds like a nice addition, I will post a patch. Thanks!
- Is there cleanup code that removes shared objects from the hash
table when a vhost_dev is destroyed? Otherwise TYPE_VHOST_DEV shared
objects are leaked and their stale vhost_dev pointers could be
dereferenced.
There is not. In a first thought, I assumed that the backends will be in charge
of cleaning their entries from the shared hash table when they are destroyed
(prematurely or no). I will look into occurrences of vhost_dev getting destroyed
that may need explicit handling of the leftover entries.
- virtio-dmabuf.h API naming suggests this is a core VirtIODevice API:
virtio_free_resources(), virtio_add_vhost_device(), etc rather than an
API for VirtioSharedObject. Can the names be made more specific:
virtio_dmabuf_*() or virtio_shobj_*() so it's clear these APIs are
related to the dmabufs/shared objects?
Improving the names with what you suggested sounds good to me.
I guess I'll go with virtio_dmabuf_*, for consistency with the file
name. But `shobj` would be ok too.
Thanks,
Stefan