[PATCH v3 1/1] vhost: tolerate file descriptor in REM_MEM_REG msg

From: Pravin M Bathija <[email protected]>

The vhost-user specification (vhost-user.rst) states that no file
descriptors SHOULD be passed with VHOST_USER_REM_MEM_REG.  However,
it also says: "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."

Some front-ends, notably libblkio, reuse the same message-building
helper for both ADD_MEM_REG and REM_MEM_REG and unconditionally attach
the mapping fd.  The previous implementation rejected any
REM_MEM_REG carrying a file descriptor with the error:

	expect 0 FDs for request VHOST_USER_REM_MEM_REG, received 1

This broke teardown and memory region hot-swap with these front-ends.

To reproduce, run any libblkio (v1.5.0) application using the
virtio-blk-vhost-user driver against a DPDK vhost back-end.  The
connection is dropped during cleanup or whenever a memory region is
unmapped and remapped.

QEMU's libvhost-user reference back-end (vu_rem_mem_reg) already
tolerates zero or one fd in this message.  Align DPDK's behavior
with both the specification's compatibility clause and the reference
implementation by accepting the message and closing any unexpected fd.

Tested with:
 - QEMU VM bring-up with runtime add/remove memory regions via
   QEMU monitor
 - QEMU post-copy live migration between source and destination
 - SPDK vhost-blk with libblkio (fio libblkio engine, write + md5
   verify)
 - libblkio alloc-mem-region and map-mem-region tests exercising
   ADD_MEM_REG / REM_MEM_REG / ADD_MEM_REG cycles against SPDK
   vhost-blk

Fixes: 1d730eea6a42 ("vhost: add memory region handlers")
Cc: [email protected]

Signed-off-by: Pravin M Bathija <[email protected]>
---
 lib/vhost/vhost_user.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index 020c993b29..82c62f2f64 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -73,7 +73,7 @@ VHOST_MESSAGE_HANDLER(VHOST_USER_RESET_OWNER, vhost_user_reset_owner, false, fal
 VHOST_MESSAGE_HANDLER(VHOST_USER_SET_MEM_TABLE, vhost_user_set_mem_table, true, true) \
 VHOST_MESSAGE_HANDLER(VHOST_USER_GET_MAX_MEM_SLOTS, vhost_user_get_max_mem_slots, false, false) \
 VHOST_MESSAGE_HANDLER(VHOST_USER_ADD_MEM_REG, vhost_user_add_mem_reg, true, true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_REM_MEM_REG, vhost_user_rem_mem_reg, false, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_REM_MEM_REG, vhost_user_rem_mem_reg, true, true) \
 VHOST_MESSAGE_HANDLER(VHOST_USER_SET_LOG_BASE, vhost_user_set_log_base, true, true) \
 VHOST_MESSAGE_HANDLER(VHOST_USER_SET_LOG_FD, vhost_user_set_log_fd, true, true) \
 VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_NUM, vhost_user_set_vring_num, false, true) \
@@ -1811,6 +1811,13 @@ vhost_user_rem_mem_reg(struct virtio_net **pdev,
 	struct virtio_net *dev = *pdev;
 	uint32_t i;
 
+	/*
+	 * The specification says no file descriptor should be passed with
+	 * this message, but some front-ends send one anyway. Tolerate it and
+	 * close it without using it, as the specification requires.
+	 */
+	close_msg_fds(ctx);
+
 	if (dev->mem == NULL || dev->mem->nregions == 0) {
 		VHOST_CONFIG_LOG(dev->ifname, ERR, "no memory regions to remove");
 		return RTE_VHOST_MSG_RESULT_ERR;
-- 
2.43.0