On Tue, Oct 17, 2023 at 12:57 PM Stefan Hajnoczi <stefa...@gmail.com> wrote:
> On Tue, 17 Oct 2023 at 04:26, Albert Esteve <aest...@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: https://patchew.org/QEMU/c61c13f9a0c67dec473bdbfc8789c29ef26c900b.1696624734.git.quic._5fmathb...@quicinc.com/ > - 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 > >
