Re: [PATCH v12 10/10] virtio-gpu-dmabuf: Create dmabuf for blobs associated with VFIO devices

[Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>]

On 2026/03/25 14:29, Kasireddy, Vivek wrote:
> Hi Akihiko,
> 
>> Subject: Re: [PATCH v12 10/10] virtio-gpu-dmabuf: Create dmabuf for
>> blobs associated with VFIO devices
>>
>> On 2026/03/19 14:15, Vivek Kasireddy wrote:
>>> In addition to memfd, a blob resource can also have its backing
>>> storage in a VFIO device region. Since, there is no effective way
>>> to determine where the backing storage is located, we first try to
>>> create a dmabuf assuming it is in memfd. If that fails, we try to
>>> create a dmabuf assuming it is in VFIO device region.
>>>
>>> So, we first call virtio_gpu_create_udmabuf() to check if the blob's
>>> backing storage is located in a memfd or not. If it is not, we invoke
>>> the vfio_device_create_dmabuf_fd() API which identifies the right
>>> VFIO device and eventually creates a dmabuf fd.
>>>
>>> Note that, for mmapping the dmabuf, we directly call mmap() if the
>>> dmabuf fd was created via virtio_gpu_create_udmabuf() since we
>> know
>>> that the udmabuf driver supports mmap(). However, if the dmabuf
>> was
>>> created via vfio_device_create_dmabuf_fd(), we use the
>>> vfio_device_mmap_dmabuf() API to get a mapping for the dmabuf.
>>>
>>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Cc: Alex Bennée <alex.bennee@linaro.org>
>>> Cc: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>>> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> Cc: Alex Williamson <alex@shazbot.org>
>>> Cc: Cédric Le Goater <clg@redhat.com>
>>> Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>> ---
>>>    hw/display/virtio-gpu-dmabuf.c | 23 ++++++++++++++++++++---
>>>    1 file changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/display/virtio-gpu-dmabuf.c b/hw/display/virtio-gpu-
>> dmabuf.c
>>> index 89aa487654..f953db0fbe 100644
>>> --- a/hw/display/virtio-gpu-dmabuf.c
>>> +++ b/hw/display/virtio-gpu-dmabuf.c
>>> @@ -147,9 +147,26 @@ void virtio_gpu_init_dmabuf(struct
>> virtio_gpu_simple_resource *res)
>>>            if (res->dmabuf_fd ==
>> VFIO_DMABUF_CREATE_ERR_INVALID_IOV) {
>>>                error_free_or_abort(&local_err);
>>>
>>> -            qemu_log_mask(LOG_GUEST_ERROR,
>>> -                          "Cannot create dmabuf: incompatible memory\n");
>>> -            return;
>>> +            res->dmabuf_fd = vfio_device_create_dmabuf_fd(res->iov,
>>> +                                                          res->iov_cnt,
>>> +                                                          res->blob_size,
>>
>> The correspondence between (iov, iov_cnt) and blob_size is more of a
>> internal concern of virtio-gpu, not of VFIO. This parameter is better
>> removed from vfio_device_create_dmabuf_fd() and
>> vfio_device_mmap_dmabuf().
> I don't disagree. So, should we add the following check in
> virtio_gpu_init_dmabuf() or somewhere?
> if (iov_size(iov, iov_cnt) != blob_size)

I suggest to have a check in virtio_gpu_create_mapping_iov() since it's 
not even specific to DMA-BUF.

And instead let's ensure iov_size(iov, iov_cnt) >= blob_size and reject 
otherwise instead. I cited Codex's reasoning for this, which I totally 
agree. (I applied the Codex for Open Source program for access to Codex. 
And just in case: we are currently not allowed to use LLMs for writing 
patches and its use is restricted for the other purposes.)

It will be also more of a bug fix, so I think it is better to be sent as 
an independent patch instead of including it into this series.

Regards,
Akihiko Odaki

Below is the Codex's output based on commit 8e711856d763 ("Merge tag 
'hppa-fixes-for-v11-pull-request' of 
https://github.com/hdeller/qemu-hppa into staging"):

In current QEMU, omitting the check leaves inconsistent state possible, 
and the effect depends on the direction of the mismatch.

If iov_size < blob_size, this is the bad case. The dmabuf/export backing 
is built from the iov lengths in virtio-gpu-udmabuf.c (line 45) and 
virtio-gpu-udmabuf.c (line 63), but remap and scanout bounds use 
blob_size in virtio-gpu-udmabuf.c (line 73) and virtio-gpu.c (line 761). 
There is also a fast path that directly exposes iov_base as res->blob 
for small single-iov blobs in virtio-gpu-udmabuf.c (line 136), and 
scanout later uses that pointer as image data in virtio-gpu.c (line 662) 
and virtio-gpu.c (line 674). So a too-small backing is not a clean or 
obviously safe case.
If iov_size > blob_size, it is mostly a semantics issue. QEMU still 
bounds scanout using blob_size in virtio-gpu.c (line 761), so the extra 
backing is usually just unused. But the resource state is still 
inconsistent.
QEMU does not currently enforce the relationship elsewhere. blob_size 
and the iov are populated independently at blob creation in virtio-gpu.c 
(line 362) and virtio-gpu.c (line 366), and later backing attach also 
does not compare them in virtio-gpu.c (line 946).
So if the question is “can we omit the check entirely?”, the answer is: 
yes, but then you are knowingly accepting malformed guest state and 
relying on later backend behavior. If you want a defensive check, 
iov_size < blob_size is the one with a concrete justification. iov_size 
!= blob_size is harder to defend from the spec.

> 
> Thanks,
> Vivek
>>
>> Regards,
>> Akihiko Odaki
>>
>>> +                                                          &local_err);
>>> +            if (res->dmabuf_fd ==
>> VFIO_DMABUF_CREATE_ERR_INVALID_IOV) {
>>> +                error_free_or_abort(&local_err);
>>> +                qemu_log_mask(LOG_GUEST_ERROR,
>>> +                              "Cannot create dmabuf: incompatible memory\n");
>>> +                return;
>>> +            }
>>> +
>>> +            if (res->dmabuf_fd >= 0) {
>>> +                pdata = vfio_device_mmap_dmabuf(res->iov, res->iov_cnt,
>>> +                                                res->blob_size, &local_err);
>>> +                if (!pdata) {
>>> +                    virtio_gpu_destroy_dmabuf(res);
>>> +                }
>>> +            } else {
>>> +                res->dmabuf_fd = -1;
>>> +            }
>>>            } else if (res->dmabuf_fd >= 0) {
>>>                pdata = virtio_gpu_remap_dmabuf(res, &local_err);
>>>                if (!pdata) {
>