Re: [PATCH v3 1/3] vhost: Fix SHMEM_MAP transaction ordering and resulting deadlock

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Jul 17, 2026 at 03:11:12PM +0200, Albert Esteve wrote:
> On Fri, Jul 17, 2026 at 2:58 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Jul 17, 2026 at 01:50:31PM +0200, Albert Esteve wrote:
> > > On Fri, Jul 17, 2026 at 12:37 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Fri, Jul 17, 2026 at 12:32:59PM +0200, Dorinda Bassey 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.
> > > > >
> > > > > The explicit transaction begin/commit wrapping is dropped since
> > > > > virtio_add_shmem_map() already commits via memory_region_add_subregion().
> > > > >
> > > > > Fixes: b52e1896e764 ("vhost-user: Add VirtIO Shared Memory map request")
> > > > > Suggested-by: Albert Esteve <aesteve@redhat.com>
> > > > > Signed-off-by: Dorinda Bassey <dbassey@redhat.com>
> > > > > ---
> > > > >  hw/virtio/vhost-user.c | 20 ++------------------
> > > > >  hw/virtio/vhost.c      | 10 ++++++++++
> > > > >  2 files changed, 12 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > > index 517cc4ca716..01468f7441a 100644
> > > > > --- a/hw/virtio/vhost-user.c
> > > > > +++ b/hw/virtio/vhost-user.c
> > > > > @@ -1977,8 +1977,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,
> > > > > @@ -1986,7 +1984,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 */
> > > > > @@ -1994,22 +1992,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) {
> > > > > 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;
> > > > > +    }
> > > > > +
> > > > >      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;
> > > >
> > > >
> > > > This all seems risky i have to say.  Is this always map never unmap?
> > >
> > > The filter covers both, and this is a good point. For symmetry the
> > > handle_shmem_unmap() should call virtio_del_shmem_map() before
> > > replying.
> > >
> > > BR,
> > > Albert.
> >
> > the reverse order I would say?
> >
> > if we add, first we commit then update the backend
> >
> > if we delete first update the backend then commit?
> 
> I think it could be argued both ways, so I'd be ok with what you
> described (which is what's done in current mainline for the unmap
> side).
> 
> The reason the commit (the whole del_shmem_map() op) was delayed
> before on unmap was to avoid the deadlock (the same as for map).
> 
> With the filter, that is no longer a problem. So I would say that if
> the backend sends a SHMEM_UNMAP request, by the time it receives the
> reply the unmap operation is already commited. It's more symmetrical:
> the frontend only replies when the requested memory operation (map or
> unmap) is finished and committed.

I don't know, this thing makes my head hurt.
Pls document ordering for both in the commit log.

> >
> >
> >
> > > >
> > > > > --
> > > > > 2.52.0
> > > >
> >