Re: [PATCH v10 4/5] vhost_user: Function defs for add/rem mem regions

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 7 Apr 2026 22:42:04 +0000
<pravin.bathija@dell.com> wrote:

> From: Pravin M Bathija <pravin.bathija@dell.com>
> 
> These changes cover the function definition for add/remove memory
> region calls which are invoked on receiving vhost user message from
> vhost user front-end (e.g. Qemu). In our case, in addition to testing
> with qemu front-end, the testing has also been performed with libblkio
> front-end and spdk/dpdk back-end. We did I/O using libblkio based device
> driver, to spdk based drives. There are also changes for set_mem_table
> and new definition for get memory slots. Our changes optimize the set
> memory table call to use common support functions. Message get memory
> slots is how the vhost-user front-end queries the vhost-user back-end
> about the number of memory slots available to be registered by the
> back-end. In addition support function to invalidate vring is also
> defined which is used in add/remove memory region functions.
> 
> Signed-off-by: Pravin M Bathija <pravin.bathija@dell.com>
> ---

The rest of the series review cleanly but AI flagged one issue.
The initial description was a mess. Asked it to be clearer.

## Review: PATCH v10 4/5 — vhost_user: Function defs for add/rem mem regions

**Warning: incomplete cleanup in `vhost_user_add_mem_reg()` mmap-failure path.**

What is wrong: when `vhost_user_mmap_region()` returns -1, the handler
only closes `reg->fd`. But `vhost_user_mmap_region()` can fail *after*
setting `reg->mmap_addr`/`mmap_size`/`host_user_addr` and after
`add_guest_pages()` has appended entries (the `add_one_guest_page()`
realloc path).

Impact: leaked mmap and stale `dev->guest_pages` entries. Since
`nregions` is not incremented, the next ADD_MEM_REG reuses the same
slot and overwrites `mmap_addr`, so `free_all_mem_regions()` can no
longer reach the original mapping.

Fix: when `reg->mmap_addr` is set, take the `free_new_region` path
(`remove_guest_pages()` + `free_mem_region(reg)`) instead of just
`close(reg->fd)`. Keep the explicit `close()` for the case where
`mmap()` itself failed before `mmap_addr` was assigned.