* [PATCH v4 0/1] vhost-user: fix shared object lookup handler logic @ 2025-10-17 7:20 Albert Esteve 2025-10-17 7:20 ` [PATCH v4 1/1] " Albert Esteve 0 siblings, 1 reply; 3+ messages in thread From: Albert Esteve @ 2025-10-17 7:20 UTC (permalink / raw) To: qemu-devel; +Cc: Stefano Garzarella, Michael S. Tsirkin, Albert Esteve v3->v4: - Base the branch on [PATCH v10 0/7] vhost-user: Add SHMEM_MAP/UNMAP requests - Add `reply_ack = false;` for SHMEM_MAP/UNMAP handlers. v4 is based on: [PATCH v10 0/7] vhost-user: Add SHMEM_MAP/UNMAP requests Based-on: <20251016143827.1850397-1-aesteve@redhat.com> Refactor backend_read() function and add a reply_ack variable to have the option for handlers to force tweak whether they should send a reply or not without depending on VHOST_USER_NEED_REPLY_MASK flag. This fixes an issue with vhost_user_backend_handle_shared_object_lookup() logic, as the error path was not closing the backend channel correctly. So, we can remove the reply call from within the handler, make sure it returns early on errors as other handlers do and set the reply_ack variable on backend_read() to true to ensure that it will send a response, thus keeping the original intent. Albert Esteve (1): vhost-user: fix shared object lookup handler logic hw/virtio/vhost-user.c | 42 +++++++++++++++--------------------------- 1 file changed, 15 insertions(+), 27 deletions(-) -- 2.49.0 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v4 1/1] vhost-user: fix shared object lookup handler logic 2025-10-17 7:20 [PATCH v4 0/1] vhost-user: fix shared object lookup handler logic Albert Esteve @ 2025-10-17 7:20 ` Albert Esteve 2025-10-17 7:52 ` Stefano Garzarella 0 siblings, 1 reply; 3+ messages in thread From: Albert Esteve @ 2025-10-17 7:20 UTC (permalink / raw) To: qemu-devel Cc: Stefano Garzarella, Michael S. Tsirkin, Albert Esteve, qemu-stable Refactor backend_read() function and add a reply_ack variable to have the option for handlers to force tweak whether they should send a reply or not without depending on VHOST_USER_NEED_REPLY_MASK flag. This fixes an issue with vhost_user_backend_handle_shared_object_lookup() logic, as the error path was not closing the backend channel correctly. So, we can remove the reply call from within the handler, make sure it returns early on errors as other handlers do and set the reply_ack variable on backend_read() to true to ensure that it will send a response, thus keeping the original intent. Fixes: 160947666276c5b7f6bca4d746bcac2966635d79 Cc: qemu-stable@nongnu.org Signed-off-by: Albert Esteve <aesteve@redhat.com> --- hw/virtio/vhost-user.c | 42 +++++++++++++++--------------------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 890be55937..762d7218d3 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -1696,14 +1696,6 @@ static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr, return !qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), errp); } -static bool -vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr, - VhostUserPayload *payload, Error **errp) -{ - hdr->size = sizeof(payload->u64); - return vhost_user_send_resp(ioc, hdr, payload, errp); -} - int vhost_user_get_shared_object(struct vhost_dev *dev, unsigned char *uuid, int *dmabuf_fd) { @@ -1744,19 +1736,15 @@ int vhost_user_get_shared_object(struct vhost_dev *dev, unsigned char *uuid, static int vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u, - QIOChannel *ioc, - VhostUserHeader *hdr, - VhostUserPayload *payload) + VhostUserShared *object) { QemuUUID uuid; CharBackend *chr = u->user->chr; - Error *local_err = NULL; int dmabuf_fd = -1; int fd_num = 0; - memcpy(uuid.data, payload->object.uuid, sizeof(payload->object.uuid)); + memcpy(uuid.data, object->uuid, sizeof(object->uuid)); - payload->u64 = 0; switch (virtio_object_type(&uuid)) { case TYPE_DMABUF: dmabuf_fd = virtio_lookup_dmabuf(&uuid); @@ -1765,18 +1753,16 @@ vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u, { struct vhost_dev *dev = virtio_lookup_vhost_device(&uuid); if (dev == NULL) { - payload->u64 = -EINVAL; - break; + return -EINVAL; } int ret = vhost_user_get_shared_object(dev, uuid.data, &dmabuf_fd); if (ret < 0) { - payload->u64 = ret; + return ret; } break; } case TYPE_INVALID: - payload->u64 = -EINVAL; - break; + return -EINVAL; } if (dmabuf_fd != -1) { @@ -1785,11 +1771,6 @@ vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u, if (qemu_chr_fe_set_msgfds(chr, &dmabuf_fd, fd_num) < 0) { error_report("Failed to set msg fds."); - payload->u64 = -EINVAL; - } - - if (!vhost_user_backend_send_dmabuf_fd(ioc, hdr, payload, &local_err)) { - error_report_err(local_err); return -EINVAL; } @@ -2008,6 +1989,7 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition, struct iovec iov; g_autofree int *fd = NULL; size_t fdsize = 0; + bool reply_ack; int i; /* Read header */ @@ -2026,6 +2008,8 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition, goto err; } + reply_ack = hdr.flags & VHOST_USER_NEED_REPLY_MASK; + /* Read payload */ if (qio_channel_read_all(ioc, (char *) &payload, hdr.size, &local_err)) { error_report_err(local_err); @@ -2051,11 +2035,14 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition, &payload.object); break; case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP: - ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc, - &hdr, &payload); + /* The backend always expects a response */ + reply_ack = true; + ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, + &payload.object); break; case VHOST_USER_BACKEND_SHMEM_MAP: /* Handler manages its own response, check error and close connection */ + reply_ack = false; if (vhost_user_backend_handle_shmem_map(dev, ioc, &hdr, &payload, fd ? fd[0] : -1) < 0) { goto err; @@ -2063,6 +2050,7 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition, break; case VHOST_USER_BACKEND_SHMEM_UNMAP: /* Handler manages its own response, check error and close connection */ + reply_ack = false; if (vhost_user_backend_handle_shmem_unmap(dev, ioc, &hdr, &payload) < 0) { goto err; } @@ -2076,7 +2064,7 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition, * REPLY_ACK feature handling. Other reply types has to be managed * directly in their request handlers. */ - if (hdr.flags & VHOST_USER_NEED_REPLY_MASK) { + if (reply_ack) { payload.u64 = !!ret; hdr.size = sizeof(payload.u64); -- 2.49.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v4 1/1] vhost-user: fix shared object lookup handler logic 2025-10-17 7:20 ` [PATCH v4 1/1] " Albert Esteve @ 2025-10-17 7:52 ` Stefano Garzarella 0 siblings, 0 replies; 3+ messages in thread From: Stefano Garzarella @ 2025-10-17 7:52 UTC (permalink / raw) To: Albert Esteve, mst; +Cc: qemu-devel, Michael S. Tsirkin, qemu-stable On Fri, Oct 17, 2025 at 09:20:11AM +0200, Albert Esteve wrote: >Refactor backend_read() function and add a reply_ack variable >to have the option for handlers to force tweak whether they should >send a reply or not without depending on VHOST_USER_NEED_REPLY_MASK >flag. > >This fixes an issue with >vhost_user_backend_handle_shared_object_lookup() logic, as the >error path was not closing the backend channel correctly. So, >we can remove the reply call from within the handler, make >sure it returns early on errors as other handlers do and >set the reply_ack variable on backend_read() to true to ensure >that it will send a response, thus keeping the original intent. > >Fixes: 160947666276c5b7f6bca4d746bcac2966635d79 >Cc: qemu-stable@nongnu.org >Signed-off-by: Albert Esteve <aesteve@redhat.com> >--- > hw/virtio/vhost-user.c | 42 +++++++++++++++--------------------------- > 1 file changed, 15 insertions(+), 27 deletions(-) IIUC to backport this to stable now, we need to remove a couple of blocks, but I think that's fine. @Michael, or would it be better to rebase "[PATCH v10 0/7] vhost-user: Add SHMEM_MAP/UNMAP requests" on this and make sure this applies cleanly to stable? Either way, the changes are fine with me: Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Thanks, Stefano > >diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c >index 890be55937..762d7218d3 100644 >--- a/hw/virtio/vhost-user.c >+++ b/hw/virtio/vhost-user.c >@@ -1696,14 +1696,6 @@ static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr, > return !qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), errp); > } > >-static bool >-vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr, >- VhostUserPayload *payload, Error **errp) >-{ >- hdr->size = sizeof(payload->u64); >- return vhost_user_send_resp(ioc, hdr, payload, errp); >-} >- > int vhost_user_get_shared_object(struct vhost_dev *dev, unsigned char *uuid, > int *dmabuf_fd) > { >@@ -1744,19 +1736,15 @@ int vhost_user_get_shared_object(struct vhost_dev *dev, unsigned char *uuid, > > static int > vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u, >- QIOChannel *ioc, >- VhostUserHeader *hdr, >- VhostUserPayload *payload) >+ VhostUserShared *object) > { > QemuUUID uuid; > CharBackend *chr = u->user->chr; >- Error *local_err = NULL; > int dmabuf_fd = -1; > int fd_num = 0; > >- memcpy(uuid.data, payload->object.uuid, sizeof(payload->object.uuid)); >+ memcpy(uuid.data, object->uuid, sizeof(object->uuid)); > >- payload->u64 = 0; > switch (virtio_object_type(&uuid)) { > case TYPE_DMABUF: > dmabuf_fd = virtio_lookup_dmabuf(&uuid); >@@ -1765,18 +1753,16 @@ vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u, > { > struct vhost_dev *dev = virtio_lookup_vhost_device(&uuid); > if (dev == NULL) { >- payload->u64 = -EINVAL; >- break; >+ return -EINVAL; > } > int ret = vhost_user_get_shared_object(dev, uuid.data, &dmabuf_fd); > if (ret < 0) { >- payload->u64 = ret; >+ return ret; > } > break; > } > case TYPE_INVALID: >- payload->u64 = -EINVAL; >- break; >+ return -EINVAL; > } > > if (dmabuf_fd != -1) { >@@ -1785,11 +1771,6 @@ vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u, > > if (qemu_chr_fe_set_msgfds(chr, &dmabuf_fd, fd_num) < 0) { > error_report("Failed to set msg fds."); >- payload->u64 = -EINVAL; >- } >- >- if (!vhost_user_backend_send_dmabuf_fd(ioc, hdr, payload, &local_err)) { >- error_report_err(local_err); > return -EINVAL; > } > >@@ -2008,6 +1989,7 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition, > struct iovec iov; > g_autofree int *fd = NULL; > size_t fdsize = 0; >+ bool reply_ack; > int i; > > /* Read header */ >@@ -2026,6 +2008,8 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition, > goto err; > } > >+ reply_ack = hdr.flags & VHOST_USER_NEED_REPLY_MASK; >+ > /* Read payload */ > if (qio_channel_read_all(ioc, (char *) &payload, hdr.size, &local_err)) { > error_report_err(local_err); >@@ -2051,11 +2035,14 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition, > &payload.object); > break; > case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP: >- ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc, >- &hdr, &payload); >+ /* The backend always expects a response */ >+ reply_ack = true; >+ ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, >+ &payload.object); > break; > case VHOST_USER_BACKEND_SHMEM_MAP: > /* Handler manages its own response, check error and close connection */ >+ reply_ack = false; > if (vhost_user_backend_handle_shmem_map(dev, ioc, &hdr, &payload, > fd ? fd[0] : -1) < 0) { > goto err; >@@ -2063,6 +2050,7 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition, > break; > case VHOST_USER_BACKEND_SHMEM_UNMAP: > /* Handler manages its own response, check error and close connection */ >+ reply_ack = false; > if (vhost_user_backend_handle_shmem_unmap(dev, ioc, &hdr, &payload) < 0) { > goto err; > } >@@ -2076,7 +2064,7 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition, > * REPLY_ACK feature handling. Other reply types has to be managed > * directly in their request handlers. > */ >- if (hdr.flags & VHOST_USER_NEED_REPLY_MASK) { >+ if (reply_ack) { > payload.u64 = !!ret; > hdr.size = sizeof(payload.u64); > >-- >2.49.0 > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-10-17 7:54 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-17 7:20 [PATCH v4 0/1] vhost-user: fix shared object lookup handler logic Albert Esteve 2025-10-17 7:20 ` [PATCH v4 1/1] " Albert Esteve 2025-10-17 7:52 ` Stefano Garzarella
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).