[PATCH 0/3] libvhost-user: NEED_REPLY behaviour fixes

[PATCH 0/3] libvhost-user: NEED_REPLY behaviour fixes

[Kevin Wolf]

Command handlers shouldn't send replies themselves, but just prepare the
message and let vu_dispatch() send it. Otherwise, vu_dispatch() will add
a second reply as an ACK when NEED_REPLY was requested.

QEMU's vhost-user devices are not affected by these bugs because they
don't set NEED_REPLY for the commands that get it wrong. libblkio does
run into it, though.

Kevin Wolf (3):
  docs/vhost-user: Fix mismerge
  libvhost-user: Fix VHOST_USER_GET_MAX_MEM_SLOTS reply
  libvhost-user: Fix VHOST_USER_ADD_MEM_REG reply

 docs/interop/vhost-user.rst               | 16 ----------------
 subprojects/libvhost-user/libvhost-user.c | 19 +++----------------
 2 files changed, 3 insertions(+), 32 deletions(-)

-- 
2.35.3

[PATCH 1/3] docs/vhost-user: Fix mismerge

[Kevin Wolf]

This reverts commit 76b1b64370007234279ea4cc8b09c98cbd2523de.

The commit only duplicated some text that had already been merged in
commit 31009d13cc5.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 docs/interop/vhost-user.rst | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index d7cf904f7f..3f18ab424e 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -1376,14 +1376,6 @@ Front-end message types
   For further details on postcopy, see ``VHOST_USER_SET_MEM_TABLE``.
   They apply to ``VHOST_USER_ADD_MEM_REG`` accordingly.
 
-  Exactly one file descriptor from which the memory is mapped is
-  passed in the ancillary data.
-
-  In postcopy mode (see ``VHOST_USER_POSTCOPY_LISTEN``), the back-end
-  replies with the bases of the memory mapped region to the front-end.
-  For further details on postcopy, see ``VHOST_USER_SET_MEM_TABLE``.
-  They apply to ``VHOST_USER_ADD_MEM_REG`` accordingly.
-
 ``VHOST_USER_REM_MEM_REG``
   :id: 38
   :equivalent ioctl: N/A
@@ -1408,14 +1400,6 @@ Front-end message types
   accept messages with one file descriptor. If a file descriptor is
   passed, the back-end MUST close it without using it otherwise.
 
-  The memory region to be removed is identified by its guest address,
-  user address and size. The mmap offset is ignored.
-
-  No file descriptors SHOULD be passed in the ancillary data. For
-  compatibility with existing incorrect implementations, the back-end MAY
-  accept messages with one file descriptor. If a file descriptor is
-  passed, the back-end MUST close it without using it otherwise.
-
 ``VHOST_USER_SET_STATUS``
   :id: 39
   :equivalent ioctl: VHOST_VDPA_SET_STATUS
-- 
2.35.3

Re: [PATCH 1/3] docs/vhost-user: Fix mismerge

[Peter Maydell]

On Mon, 27 Jun 2022 at 14:49, Kevin Wolf <kwolf@redhat.com> wrote:
>
> This reverts commit 76b1b64370007234279ea4cc8b09c98cbd2523de.
>
> The commit only duplicated some text that had already been merged in
> commit 31009d13cc5.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  docs/interop/vhost-user.rst | 16 ----------------
>  1 file changed, 16 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 2/3] libvhost-user: Fix VHOST_USER_GET_MAX_MEM_SLOTS reply

[Kevin Wolf]

With REPLY_NEEDED, libvhost-user sends both the acutal result and an
additional ACK reply for VHOST_USER_GET_MAX_MEM_SLOTS. This is
incorrect, the spec mandates that it behave the same with and without
REPLY_NEEDED because it always sends a reply.

Fixes: 6fb2e173d20c9bbb5466183d33a3ad7dcd0375fa
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 subprojects/libvhost-user/libvhost-user.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index b4cc3c2d68..cfa1bcc334 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -1827,18 +1827,11 @@ vu_handle_vring_kick(VuDev *dev, VhostUserMsg *vmsg)
 
 static bool vu_handle_get_max_memslots(VuDev *dev, VhostUserMsg *vmsg)
 {
-    vmsg->flags = VHOST_USER_REPLY_MASK | VHOST_USER_VERSION;
-    vmsg->size  = sizeof(vmsg->payload.u64);
-    vmsg->payload.u64 = VHOST_USER_MAX_RAM_SLOTS;
-    vmsg->fd_num = 0;
-
-    if (!vu_message_write(dev, dev->sock, vmsg)) {
-        vu_panic(dev, "Failed to send max ram slots: %s\n", strerror(errno));
-    }
+    vmsg_set_reply_u64(vmsg, VHOST_USER_MAX_RAM_SLOTS);
 
     DPRINT("u64: 0x%016"PRIx64"\n", (uint64_t) VHOST_USER_MAX_RAM_SLOTS);
 
-    return false;
+    return true;
 }
 
 static bool
-- 
2.35.3

[PATCH 3/3] libvhost-user: Fix VHOST_USER_ADD_MEM_REG reply

[Kevin Wolf]

With REPLY_NEEDED, libvhost-user sends both the acutal result and an
additional ACK reply for VHOST_USER_ADD_MEM_REG. This is incorrect, the
spec mandates that it behave the same with and without REPLY_NEEDED
because it always sends a reply.

Fixes: ec94c8e621de96c50c2d381c8c9ec94f5beec7c1
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 subprojects/libvhost-user/libvhost-user.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index cfa1bcc334..ffed4729a3 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -779,15 +779,9 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
 
         /* Send the message back to qemu with the addresses filled in. */
         vmsg->fd_num = 0;
-        if (!vu_send_reply(dev, dev->sock, vmsg)) {
-            vu_panic(dev, "failed to respond to add-mem-region for postcopy");
-            return false;
-        }
-
         DPRINT("Successfully added new region in postcopy\n");
         dev->nregions++;
-        return false;
-
+        return true;
     } else {
         for (i = 0; i < dev->max_queues; i++) {
             if (dev->vq[i].vring.desc) {
-- 
2.35.3