Re: [PATCH v4 1/1] vhost-user: fix shared object lookup handler logic

["Michael S. Tsirkin" <mst@redhat.com>]

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


I fixed this, should include the subject.

> Cc: qemu-stable@nongnu.org
> Signed-off-by: Albert Esteve <aesteve@redhat.com>


So I picked this for now, but I worry that we are opening ourselves
up to buggy backends which forget to set the flag and expect
frontend to behave properly.

I think a better fix would be instead of forcing, check reply_ack,
and fail if not set correctly.

> ---
>  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