On 2026/02/19 16:26, Kasireddy, Vivek wrote:
Hi Akihiko,
Subject: Re: [PATCH v6 06/11] virtio-gpu-dmabuf: Improve error
handling by introducing 'Error **'
Make the error handling more robust in
virtio_gpu_init_udmabuf()
by passing in 'Error **' parameter to capture errors from
virtio_gpu_create_udmabuf() and virtio_gpu_remap_dmabuf().
And, since they now take in 'Error **' parameter, have them
return a bool to adhere to best practices.
Cc: Marc-André Lureau <[email protected]>
Cc: Alex Bennée <[email protected]>
Cc: Akihiko Odaki <[email protected]>
Cc: Dmitry Osipenko <[email protected]>
Cc: Alex Williamson <[email protected]>
Cc: Cédric Le Goater <[email protected]>
Signed-off-by: Vivek Kasireddy <[email protected]>
---
hw/display/virtio-gpu-dmabuf.c | 45 ++++++++++++++++++++++-
-----
-
-----
1 file changed, 30 insertions(+), 15 deletions(-)
diff --git a/hw/display/virtio-gpu-dmabuf.c b/hw/display/virtio-
gpu-
dmabuf.c
index 8d67ef7c2a..d9b2ecaf31 100644
--- a/hw/display/virtio-gpu-dmabuf.c
+++ b/hw/display/virtio-gpu-dmabuf.c
@@ -27,7 +27,8 @@
#include "standard-headers/linux/udmabuf.h"
#include "standard-headers/drm/drm_fourcc.h"
-static void virtio_gpu_create_udmabuf(struct
virtio_gpu_simple_resource *res)
+static bool virtio_gpu_create_udmabuf(struct
virtio_gpu_simple_resource *res,
+ Error **errp)
{
g_autofree struct udmabuf_create_list *list = NULL;
RAMBlock *rb;
@@ -36,7 +37,8 @@ static void
virtio_gpu_create_udmabuf(struct
virtio_gpu_simple_resource *res)
udmabuf = udmabuf_fd();
if (udmabuf < 0) {
- return;
+ error_setg(errp, "udmabuf device not available");
+ return false;
}
list = g_malloc0(sizeof(struct udmabuf_create_list) +
@@ -45,7 +47,10 @@ static void
virtio_gpu_create_udmabuf(struct
virtio_gpu_simple_resource *res)
for (i = 0; i < res->iov_cnt; i++) {
rb = qemu_ram_block_from_host(res->iov[i].iov_base,
false,
&offset);
if (!rb || rb->fd < 0) {
- return;
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Could not find valid ramblock\n",
+ __func__);
+ return false;
include/qapi/error.h suggests the following error handling pattern:
> Call a function, receive an error from it, and pass it to the caller
> - when the function returns a value that indicates failure, say
> false:
> if (!foo(arg, errp)) {
> handle the error...
> }
Returning false without passing an error to the caller breaks it so
please don't do that.
That's why I added an additional check in the caller:
if (local_err) {
error_report_err(local_err);
}
Having said that, I am not sure what is the right thing to do here.
IMO, the proper way would be to add error_setg() but you
mentioned
doing that would be incorrect given that this is a Guest error.
So, how should I proceed?
Any further thoughts/comments here and below?
Sorry, I missed them.
Here, this function should not require its caller to have such an
additional check. Ideally, the idiom described in include/qapi/error.h
should work.
include/qapi/error.h says:
> - Whenever practical, also return a value that indicates success /
> failure. This can make the error checking more concise, and can
> avoid useless error object creation and destruction. Note that
> we still have many functions returning void. We recommend
> . bool-valued functions return true on success / false on failure,
> . pointer-valued functions return non-null / null pointer, and
> . integer-valued functions return non-negative / negative.
Since this function is deriving a file descriptor, it should return one.
The error condition should be represented by a negative value. It is a
common to return -errno and let the caller distinguish error conditions,
but the error conditions we care cannot properly represented with
-errno, so there should be an enum for the two error conditions: invalid
iov or anything else.
Ok, I'll look into adding an enum but I feel like this is getting unnecessarily
complex particularly with the introduction of 'Error *'. I think it might be
easier to just not use 'Error *' in virtio_gpu_create_udmabuf() and elsewhere.
Error * is just a convenient mechanism to propagate error messages. You
will want it to report the "anything else invalid iov" condition because
you will have no idea what it is without Error *.
list->list[i].memfd = rb->fd;
@@ -58,20 +63,26 @@ static void
virtio_gpu_create_udmabuf(struct
virtio_gpu_simple_resource *res)
res->dmabuf_fd = ioctl(udmabuf, UDMABUF_CREATE_LIST,
list);
if (res->dmabuf_fd < 0) {
- warn_report("%s: UDMABUF_CREATE_LIST: %s", __func__,
- strerror(errno));
+ error_setg_errno(errp, -res->dmabuf_fd,
+ "Could not create dmabuf fd via udmabuf
driver");
This is a guest error since it indicates that the guest specified
wrong
addresses.
I don't have a strong opinion here but my understanding is that this
should
not be considered a Guest error because the rb (which we obtained
using
Guest addresses) is valid at this point, so the
UDMABUF_CREATE_LIST
IOCTL
failure should be treated as a Host error.
Regardless, it is not clear how we can precisely determine who
(Guest or
Host)
is responsible for the failure here and in some other places.
rb->fd may still contain something other than memfd.
Assume anything the guest passes can be invalid. iov the guest passes
can be whatever you do not expect.
Right, but my point is that, for example, if UDMABUF_CREATE_LIST IOCTL
failure is due to -ENOMEM, then how can we distinguish this case and
categorize it as Host error?
And, similarly, rb->fd being something other than memfd does lead to
UDMABUF_CREATE_LIST IOCTL failure and this needs to be categorized
as Guest error. So, can we rely on errno being -EINVAL or -EBADFD to
identify Guest errors and categorize all other errors as Host errors?
Yes. As far as I can tell, the ioctl will receive an invalid argument or
bad file descriptor if and only if the guest makes an error, so -EINVAL
or -EBADFD should be used.
Perhaps we may get these errors if the kernel has a bug or the hardware
is faulty, but it's fine to ignore the possibility. Whatever can happen
if QEMU and the platform are unreliable and it's impossible to recover
errors in such a scenario. We are effectively drawing a security and
reliability boundary here; anything inside is trusted and reliable and
anything outside is not. The kernel is inside and the iov is from the
outside.
Regards,
Akihiko Odaki
Thanks,
Vivek
Regards,
Akihiko Odaki
Thanks,
Vivek
Regards,
Akihiko Odaki
+ return false;
}
+ return true;
}
-static void virtio_gpu_remap_dmabuf(struct
virtio_gpu_simple_resource
*res)
+static bool virtio_gpu_remap_dmabuf(struct
virtio_gpu_simple_resource
*res,
+ Error **errp)
{
- res->remapped = mmap(NULL, res->blob_size, PROT_READ,
- MAP_SHARED, res->dmabuf_fd, 0);
- if (res->remapped == MAP_FAILED) {
- warn_report("%s: dmabuf mmap failed: %s", __func__,
- strerror(errno));
+ void *map;
+
+ map = mmap(NULL, res->blob_size, PROT_READ,
MAP_SHARED,
res-
dmabuf_fd, 0);
+ if (map == MAP_FAILED) {
+ error_setg_errno(errp, errno, "dmabuf mmap failed");
res->remapped = NULL;
+ return false;
}
+ res->remapped = map;
+ return true;
}
static void virtio_gpu_destroy_dmabuf(struct
virtio_gpu_simple_resource *res)
@@ -125,6 +136,7 @@ bool virtio_gpu_have_udmabuf(void)
void virtio_gpu_init_dmabuf(struct virtio_gpu_simple_resource
*res)
{
+ Error *local_err = NULL;
void *pdata = NULL;
res->dmabuf_fd = -1;
@@ -132,12 +144,15 @@ void virtio_gpu_init_dmabuf(struct
virtio_gpu_simple_resource *res)
res->iov[0].iov_len < 4096) {
pdata = res->iov[0].iov_base;
} else {
- virtio_gpu_create_udmabuf(res);
- if (res->dmabuf_fd < 0) {
+ if (!virtio_gpu_create_udmabuf(res, &local_err)) {
+ if (local_err) {
+ error_report_err(local_err);
+ }
return;
}
- virtio_gpu_remap_dmabuf(res);
- if (!res->remapped) {
+
+ if (!virtio_gpu_remap_dmabuf(res, &local_err)) {
+ error_report_err(local_err);
return;
}
pdata = res->remapped;
