Hi Dorinda, On Thu, Jul 16, 2026 at 3:25 PM Dorinda Bassey <[email protected]> wrote: > > Commit the memory region transaction before sending the SHMEM_MAP reply > so the KVM memory slot exists before the guest can access the mapped > region. > > This moves the commit to while the backend is still blocked waiting for > the reply, which can trigger ADD_MEM_REG back to the backend and > deadlock. Filter shmem mapping regions out of vhost_section() by > checking the MR owner type to prevent this.
This is a nice alternative that prevents the deadlock without delaying the commit. > > Signed-off-by: Dorinda Bassey <[email protected]> > --- > hw/virtio/vhost-user.c | 6 +++--- > hw/virtio/vhost.c | 10 ++++++++++ > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c > index 517cc4ca716..8d0f463aab8 100644 > --- a/hw/virtio/vhost-user.c > +++ b/hw/virtio/vhost-user.c > @@ -1998,17 +1998,17 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev > *dev, > } > > send_reply_commit: > - /* Send reply and commit after transaction started */ > + /* Commit before reply so the KVM memory slot exists before guest access > */ > + memory_region_transaction_commit(); Without the need to delay the commit, both the transaction_start() and transaction_commit() can be dropped entirely. virtio_add_shmem_map() already opens and commits a transaction via memory_region_add_subregion(). That also makes send_reply_commit redundant. Something like this (untested, but should work fine with your filter below)?: diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index d627351f45..55d8ce1f83 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -1955,8 +1955,6 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev *dev, } } - memory_region_transaction_begin(); - /* Create VirtioSharedMemoryMapping object */ VirtioSharedMemoryMapping *mapping = virtio_shared_memory_mapping_new( vu_mmap->shmid, fd, vu_mmap->fd_offset, vu_mmap->shm_offset, @@ -1964,7 +1962,7 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev *dev, if (!mapping) { ret = -EFAULT; - goto send_reply_commit; + goto send_reply; } /* Add the mapping to the shared memory region */ @@ -1972,22 +1970,8 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev *dev, error_report("Failed to add shared memory mapping"); object_unref(OBJECT(mapping)); ret = -EFAULT; - goto send_reply_commit; - } - -send_reply_commit: - /* Send reply and commit after transaction started */ - if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) { - payload->u64 = !!ret; - hdr->size = sizeof(payload->u64); - if (!vhost_user_send_resp(ioc, hdr, payload, &local_err)) { - error_report_err(local_err); - memory_region_transaction_commit(); - return -EFAULT; - } + goto send_reply; } - memory_region_transaction_commit(); - return 0; send_reply: if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) { ... } return 0; > + > if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) { > payload->u64 = !!ret; > hdr->size = sizeof(payload->u64); > if (!vhost_user_send_resp(ioc, hdr, payload, &local_err)) { > error_report_err(local_err); > - memory_region_transaction_commit(); > return -EFAULT; > } > } > - memory_region_transaction_commit(); > return 0; > > send_reply: > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index af41841b529..e5c219be095 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -636,6 +636,16 @@ static bool vhost_section(struct vhost_dev *dev, > MemoryRegionSection *section) > { > MemoryRegion *mr = section->mr; > > + /* > + * Skip shmem mapping regions, they are managed via SHMEM_MAP/UNMAP. > + * Including them triggers ADD_MEM_REG during the SHMEM_MAP transaction > + * commit, deadlocking the backend. > + */ > + if (object_dynamic_cast(memory_region_owner(mr), > + TYPE_VIRTIO_SHARED_MEMORY_MAPPING)) { > + return false; > + } > + This is really nice. The filter looks correct to me. > if (memory_region_is_ram(mr) && !memory_region_is_rom(mr)) { > uint8_t dirty_mask = memory_region_get_dirty_log_mask(mr); > uint8_t handled_dirty; > -- > 2.52.0 >
