[PATCH 0/3] vhost-user: Fixes for VHOST_USER_ADD/REM_MEM_REG

[PATCH 0/3] vhost-user: Fixes for VHOST_USER_ADD/REM_MEM_REG

[Kevin Wolf]

While implementing a vhost-user-blk driver for libblkio, I found some
problems with VHOST_USER_ADD/REM_MEM_REG both in the spec and in the
implementations in QEMU and libvhost-user that this series addresses.

I also noticed that you can use REM_MEM_REG or SET_MEM_TABLE to unmap a
memory region that is still in use (e.g. a block I/O request using
addresses from the region has been started, but not completed yet),
which is not great. I'm not sure how to fix this best, though.

We would have to wait for these requests to complete (maybe introduce a
refcount and wait for it to drop to zero), but waiting seems impossible
in libvhost-user because it doesn't have any main loop integration. Just
failing the memory region removal would be safe, but potentially a
rather awkward interface because clients would have to implement some
retry logic.

Kevin Wolf (3):
  docs/vhost-user: Clarifications for VHOST_USER_ADD/REM_MEM_REG
  libvhost-user: Fix extra vu_add/rem_mem_reg reply
  vhost-user: Don't pass file descriptor for VHOST_USER_REM_MEM_REG

 docs/interop/vhost-user.rst               | 17 +++++++++++++++++
 hw/virtio/vhost-user.c                    |  2 +-
 subprojects/libvhost-user/libvhost-user.c | 17 +++++++----------
 3 files changed, 25 insertions(+), 11 deletions(-)

-- 
2.35.1

[PATCH 1/3] docs/vhost-user: Clarifications for VHOST_USER_ADD/REM_MEM_REG

[Kevin Wolf]

The specification for VHOST_USER_ADD/REM_MEM_REG messages is unclear
in several points, which has led to clients having incompatible
implementations. This changes the specification to be more explicit
about them:

* VHOST_USER_ADD_MEM_REG is not specified as receiving a file
  descriptor, though it obviously does need to do so. All
  implementations agree on this one, fix the specification.

* VHOST_USER_REM_MEM_REG is not specified as receiving a file
  descriptor either, and it also has no reason to do so. rust-vmm does
  not send file descriptors for removing a memory region (in agreement
  with the specification), libvhost-user and QEMU do (which is a bug),
  though libvhost-user doesn't actually make any use of it.

  Change the specification so that for compatibility QEMU's behaviour
  becomes legal, even if discouraged, but rust-vmm's behaviour becomes
  the explicitly recommended mode of operation.

* VHOST_USER_ADD_MEM_REG doesn't have a documented return value, which
  is the desired behaviour in the non-postcopy case. It also implemented
  like this in QEMU and rust-vmm, though libvhost-user is buggy and
  sometimes sends an unexpected reply. This will be fixed in a separate
  patch.

  However, in postcopy mode it does reply like VHOST_USER_SET_MEM_TABLE.
  This behaviour is shared between libvhost-user and QEMU; rust-vmm
  doesn't implement postcopy mode yet. Mention it explicitly in the
  spec.

* The specification doesn't mention how VHOST_USER_REM_MEM_REG
  identifies the memory region to be removed. Change it to describe the
  existing behaviour of libvhost-user (guest address, user address and
  size must match).

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

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 4dbc84fd00..f9e721ba5f 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -308,6 +308,7 @@ replies. Here is a list of the ones that do:
 There are several messages that the master sends with file descriptors passed
 in the ancillary data:
 
+* ``VHOST_USER_ADD_MEM_REG``
 * ``VHOST_USER_SET_MEM_TABLE``
 * ``VHOST_USER_SET_LOG_BASE`` (if ``VHOST_USER_PROTOCOL_F_LOG_SHMFD``)
 * ``VHOST_USER_SET_LOG_FD``
@@ -1334,6 +1335,14 @@ Master message types
   ``VHOST_USER_REM_MEM_REG`` message, this message is used to set and
   update the memory tables of the slave device.
 
+  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 slave
+  replies with the bases of the memory mapped region to the master.
+  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
@@ -1349,6 +1358,14 @@ Master message types
   ``VHOST_USER_ADD_MEM_REG`` message, this message is used to set and
   update the memory tables of the slave device.
 
+  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 slave MAY
+  accept messages with one file descriptor. If a file descriptor is
+  passed, the slave MUST close it without using it otherwise.
+
 ``VHOST_USER_SET_STATUS``
   :id: 39
   :equivalent ioctl: VHOST_VDPA_SET_STATUS
-- 
2.35.1

[PATCH 2/3] libvhost-user: Fix extra vu_add/rem_mem_reg reply

[Kevin Wolf]

Outside of postcopy mode, neither VHOST_USER_ADD_MEM_REG nor
VHOST_USER_REM_MEM_REG are supposed to send a reply unless explicitly
requested with the need_reply flag. Their current implementation always
sends a reply, even if it isn't requested. This confuses the master
because it will interpret the reply as a reply for the next message for
which it actually expects a reply.

need_reply is already handled correctly by vu_dispatch(), so just don't
send a reply in the non-postcopy part of the message handler for these
two commands.

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

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 47d2efc60f..eccaff5168 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -800,8 +800,7 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
 
         DPRINT("Successfully added new region\n");
         dev->nregions++;
-        vmsg_set_reply_u64(vmsg, 0);
-        return true;
+        return false;
     }
 }
 
@@ -874,15 +873,13 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
         }
     }
 
-    if (found) {
-        vmsg_set_reply_u64(vmsg, 0);
-    } else {
+    if (!found) {
         vu_panic(dev, "Specified region not found\n");
     }
 
     close(vmsg->fds[0]);
 
-    return true;
+    return false;
 }
 
 static bool
-- 
2.35.1

[PATCH 3/3] vhost-user: Don't pass file descriptor for VHOST_USER_REM_MEM_REG

[Kevin Wolf]

The spec clarifies now that QEMU should not send a file descriptor in a
request to remove a memory region. Change it accordingly.

For libvhost-user, this is a bug fix that makes it compatible with
rust-vmm's implementation that doesn't send a file descriptor. Keep
accepting, but ignoring a file descriptor for compatibility with older
QEMU versions.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/virtio/vhost-user.c                    | 2 +-
 subprojects/libvhost-user/libvhost-user.c | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 6abbc9da32..82caf607e5 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -751,7 +751,7 @@ static int send_remove_regions(struct vhost_dev *dev,
             vhost_user_fill_msg_region(&region_buffer, shadow_reg, 0);
             msg->payload.mem_reg.region = region_buffer;
 
-            ret = vhost_user_write(dev, msg, &fd, 1);
+            ret = vhost_user_write(dev, msg, NULL, 0);
             if (ret < 0) {
                 return ret;
             }
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index eccaff5168..d0041c864b 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -822,15 +822,15 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
     int i;
     bool found = false;
 
-    if (vmsg->fd_num != 1) {
+    if (vmsg->fd_num > 1) {
         vmsg_close_fds(vmsg);
-        vu_panic(dev, "VHOST_USER_REM_MEM_REG received %d fds - only 1 fd "
+        vu_panic(dev, "VHOST_USER_REM_MEM_REG received %d fds - at most 1 fd "
                       "should be sent for this message type", vmsg->fd_num);
         return false;
     }
 
     if (vmsg->size < VHOST_USER_MEM_REG_SIZE) {
-        close(vmsg->fds[0]);
+        vmsg_close_fds(vmsg);
         vu_panic(dev, "VHOST_USER_REM_MEM_REG requires a message size of at "
                       "least %d bytes and only %d bytes were received",
                       VHOST_USER_MEM_REG_SIZE, vmsg->size);
@@ -877,7 +877,7 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
         vu_panic(dev, "Specified region not found\n");
     }
 
-    close(vmsg->fds[0]);
+    vmsg_close_fds(vmsg);
 
     return false;
 }
-- 
2.35.1

Re: [PATCH 2/3] libvhost-user: Fix extra vu_add/rem_mem_reg reply

[Raphael Norwitz]

On Thu, Apr 07, 2022 at 03:36:56PM +0200, Kevin Wolf wrote:
> Outside of postcopy mode, neither VHOST_USER_ADD_MEM_REG nor
> VHOST_USER_REM_MEM_REG are supposed to send a reply unless explicitly
> requested with the need_reply flag. Their current implementation always
> sends a reply, even if it isn't requested. This confuses the master
> because it will interpret the reply as a reply for the next message for
> which it actually expects a reply.
> 
> need_reply is already handled correctly by vu_dispatch(), so just don't
> send a reply in the non-postcopy part of the message handler for these
> two commands.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> ---
>  subprojects/libvhost-user/libvhost-user.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index 47d2efc60f..eccaff5168 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -800,8 +800,7 @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
>  
>          DPRINT("Successfully added new region\n");
>          dev->nregions++;
> -        vmsg_set_reply_u64(vmsg, 0);
> -        return true;
> +        return false;
>      }
>  }
>  
> @@ -874,15 +873,13 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
>          }
>      }
>  
> -    if (found) {
> -        vmsg_set_reply_u64(vmsg, 0);
> -    } else {
> +    if (!found) {
>          vu_panic(dev, "Specified region not found\n");
>      }
>  
>      close(vmsg->fds[0]);
>  
> -    return true;
> +    return false;
>  }
>  
>  static bool
> -- 
> 2.35.1
> 

Re: [PATCH 3/3] vhost-user: Don't pass file descriptor for VHOST_USER_REM_MEM_REG

[Raphael Norwitz]

On Thu, Apr 07, 2022 at 03:36:57PM +0200, Kevin Wolf wrote:
> The spec clarifies now that QEMU should not send a file descriptor in a
> request to remove a memory region. Change it accordingly.
> 
> For libvhost-user, this is a bug fix that makes it compatible with
> rust-vmm's implementation that doesn't send a file descriptor. Keep
> accepting, but ignoring a file descriptor for compatibility with older
> QEMU versions.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> ---
>  hw/virtio/vhost-user.c                    | 2 +-
>  subprojects/libvhost-user/libvhost-user.c | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 6abbc9da32..82caf607e5 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -751,7 +751,7 @@ static int send_remove_regions(struct vhost_dev *dev,
>              vhost_user_fill_msg_region(&region_buffer, shadow_reg, 0);
>              msg->payload.mem_reg.region = region_buffer;
>  
> -            ret = vhost_user_write(dev, msg, &fd, 1);
> +            ret = vhost_user_write(dev, msg, NULL, 0);
>              if (ret < 0) {
>                  return ret;
>              }
> diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
> index eccaff5168..d0041c864b 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -822,15 +822,15 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
>      int i;
>      bool found = false;
>  
> -    if (vmsg->fd_num != 1) {
> +    if (vmsg->fd_num > 1) {
>          vmsg_close_fds(vmsg);
> -        vu_panic(dev, "VHOST_USER_REM_MEM_REG received %d fds - only 1 fd "
> +        vu_panic(dev, "VHOST_USER_REM_MEM_REG received %d fds - at most 1 fd "
>                        "should be sent for this message type", vmsg->fd_num);
>          return false;
>      }
>  
>      if (vmsg->size < VHOST_USER_MEM_REG_SIZE) {
> -        close(vmsg->fds[0]);
> +        vmsg_close_fds(vmsg);
>          vu_panic(dev, "VHOST_USER_REM_MEM_REG requires a message size of at "
>                        "least %d bytes and only %d bytes were received",
>                        VHOST_USER_MEM_REG_SIZE, vmsg->size);
> @@ -877,7 +877,7 @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {
>          vu_panic(dev, "Specified region not found\n");
>      }
>  
> -    close(vmsg->fds[0]);
> +    vmsg_close_fds(vmsg);
>  
>      return false;
>  }
> -- 
> 2.35.1
> 

Re: [PATCH 1/3] docs/vhost-user: Clarifications for VHOST_USER_ADD/REM_MEM_REG

[Raphael Norwitz]

On Thu, Apr 07, 2022 at 03:36:55PM +0200, Kevin Wolf wrote:
> The specification for VHOST_USER_ADD/REM_MEM_REG messages is unclear
> in several points, which has led to clients having incompatible
> implementations. This changes the specification to be more explicit
> about them:
> 
> * VHOST_USER_ADD_MEM_REG is not specified as receiving a file
>   descriptor, though it obviously does need to do so. All
>   implementations agree on this one, fix the specification.
> 
> * VHOST_USER_REM_MEM_REG is not specified as receiving a file
>   descriptor either, and it also has no reason to do so. rust-vmm does
>   not send file descriptors for removing a memory region (in agreement
>   with the specification), libvhost-user and QEMU do (which is a bug),
>   though libvhost-user doesn't actually make any use of it.
> 
>   Change the specification so that for compatibility QEMU's behaviour
>   becomes legal, even if discouraged, but rust-vmm's behaviour becomes
>   the explicitly recommended mode of operation.
> 
> * VHOST_USER_ADD_MEM_REG doesn't have a documented return value, which
>   is the desired behaviour in the non-postcopy case. It also implemented
>   like this in QEMU and rust-vmm, though libvhost-user is buggy and
>   sometimes sends an unexpected reply. This will be fixed in a separate
>   patch.
> 
>   However, in postcopy mode it does reply like VHOST_USER_SET_MEM_TABLE.
>   This behaviour is shared between libvhost-user and QEMU; rust-vmm
>   doesn't implement postcopy mode yet. Mention it explicitly in the
>   spec.
> 
> * The specification doesn't mention how VHOST_USER_REM_MEM_REG
>   identifies the memory region to be removed. Change it to describe the
>   existing behaviour of libvhost-user (guest address, user address and
>   size must match).
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> ---
>  docs/interop/vhost-user.rst | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> index 4dbc84fd00..f9e721ba5f 100644
> --- a/docs/interop/vhost-user.rst
> +++ b/docs/interop/vhost-user.rst
> @@ -308,6 +308,7 @@ replies. Here is a list of the ones that do:
>  There are several messages that the master sends with file descriptors passed
>  in the ancillary data:
>  
> +* ``VHOST_USER_ADD_MEM_REG``
>  * ``VHOST_USER_SET_MEM_TABLE``
>  * ``VHOST_USER_SET_LOG_BASE`` (if ``VHOST_USER_PROTOCOL_F_LOG_SHMFD``)
>  * ``VHOST_USER_SET_LOG_FD``
> @@ -1334,6 +1335,14 @@ Master message types
>    ``VHOST_USER_REM_MEM_REG`` message, this message is used to set and
>    update the memory tables of the slave device.
>  
> +  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 slave
> +  replies with the bases of the memory mapped region to the master.
> +  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
> @@ -1349,6 +1358,14 @@ Master message types
>    ``VHOST_USER_ADD_MEM_REG`` message, this message is used to set and
>    update the memory tables of the slave device.
>  
> +  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 slave MAY
> +  accept messages with one file descriptor. If a file descriptor is
> +  passed, the slave MUST close it without using it otherwise.
> +
>  ``VHOST_USER_SET_STATUS``
>    :id: 39
>    :equivalent ioctl: VHOST_VDPA_SET_STATUS
> -- 
> 2.35.1
> 

Re: [PATCH 0/3] vhost-user: Fixes for VHOST_USER_ADD/REM_MEM_REG

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]

On Thu, Apr 07, 2022 at 03:36:54PM +0200, Kevin Wolf wrote:
> While implementing a vhost-user-blk driver for libblkio, I found some
> problems with VHOST_USER_ADD/REM_MEM_REG both in the spec and in the
> implementations in QEMU and libvhost-user that this series addresses.
> 
> I also noticed that you can use REM_MEM_REG or SET_MEM_TABLE to unmap a
> memory region that is still in use (e.g. a block I/O request using
> addresses from the region has been started, but not completed yet),
> which is not great. I'm not sure how to fix this best, though.
> 
> We would have to wait for these requests to complete (maybe introduce a
> refcount and wait for it to drop to zero), but waiting seems impossible
> in libvhost-user because it doesn't have any main loop integration. Just
> failing the memory region removal would be safe, but potentially a
> rather awkward interface because clients would have to implement some
> retry logic.
> 
> Kevin Wolf (3):
>   docs/vhost-user: Clarifications for VHOST_USER_ADD/REM_MEM_REG
>   libvhost-user: Fix extra vu_add/rem_mem_reg reply
>   vhost-user: Don't pass file descriptor for VHOST_USER_REM_MEM_REG
> 
>  docs/interop/vhost-user.rst               | 17 +++++++++++++++++
>  hw/virtio/vhost-user.c                    |  2 +-
>  subprojects/libvhost-user/libvhost-user.c | 17 +++++++----------
>  3 files changed, 25 insertions(+), 11 deletions(-)
> 
> -- 
> 2.35.1
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]