On Tue, 2021-11-02 at 02:47 -0400, Michael S. Tsirkin wrote:
> On Tue, Nov 02, 2021 at 06:00:58AM +0000, Xueming(Steven) Li wrote:
> > On Mon, 2021-11-01 at 17:00 -0400, Michael S. Tsirkin wrote:
> > > On Mon, Nov 01, 2021 at 04:38:13PM +0800, Xueming Li wrote:
> > > > When vhost-user device cleanup is executed and un-mmaps notifier
> > > > address, VM cpu thread writing the notifier fails by accessing invalid
> > > > address error.
> > > >
> > > > To avoid this concurrent issue, call RCU and wait for a memory flatview
> > > > update, then un-mmap notifiers in callback.
> > > >
> > > > Fixes: 44866521bd6e ("vhost-user: support registering external host
> > > > notifiers")
> > > > Cc: qemu-sta...@nongnu.org
> > > > Cc: Yuwei Zhang <zhangyuwei.9...@bytedance.com>
> > > > Signed-off-by: Xueming Li <xuemi...@nvidia.com>
> > > > ---
> > > > hw/virtio/vhost-user.c | 50 +++++++++++++++++++++-------------
> > > > include/hw/virtio/vhost-user.h | 2 ++
> > > > 2 files changed, 33 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > index c671719e9b..5adad4d029 100644
> > > > --- a/hw/virtio/vhost-user.c
> > > > +++ b/hw/virtio/vhost-user.c
> > > > @@ -25,6 +25,7 @@
> > > > #include "migration/migration.h"
> > > > #include "migration/postcopy-ram.h"
> > > > #include "trace.h"
> > > > +#include "exec/ramblock.h"
> > > >
> > > > #include <sys/ioctl.h>
> > > > #include <sys/socket.h>
> > > > @@ -1143,15 +1144,27 @@ static int vhost_user_set_vring_num(struct
> > > > vhost_dev *dev,
> > > > return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
> > > > }
> > > >
> > > > -static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > > > - int queue_idx)
> > > > +static void vhost_user_host_notifier_free(VhostUserHostNotifier *n)
> > > > {
> > > > - struct vhost_user *u = dev->opaque;
> > > > - VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > > - VirtIODevice *vdev = dev->vdev;
> > > > + assert(n && n->old_addr);
> > > > + munmap(n->old_addr, qemu_real_host_page_size);
> > > > + n->old_addr = NULL;
> > > > +}
> > > > +
> > > > +static void vhost_user_host_notifier_remove(VhostUserState *user,
> > > > + VirtIODevice *vdev, int
> > > > queue_idx)
> > > > +{
> > > > + VhostUserHostNotifier *n = &user->notifier[queue_idx];
> > > >
> > > > if (n->addr) {
> > > > - virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr,
> > > > false);
> > > > + if (vdev) {
> > > > + virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr,
> > > > false);
> > > > + }
> > > > + assert(n->addr);
> > > > + assert(!n->old_addr);
> > > > + n->old_addr = n->addr;
> > > > + n->addr = NULL;
> > > > + call_rcu(n, vhost_user_host_notifier_free, rcu);
> > > > }
> > > > }
> > > >
> > > > @@ -1190,8 +1203,9 @@ static int vhost_user_get_vring_base(struct
> > > > vhost_dev *dev,
> > > > .payload.state = *ring,
> > > > .hdr.size = sizeof(msg.payload.state),
> > > > };
> > > > + struct vhost_user *u = dev->opaque;
> > > >
> > > > - vhost_user_host_notifier_remove(dev, ring->index);
> > > > + vhost_user_host_notifier_remove(u->user, dev->vdev, ring->index);
> > > >
> > > > if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> > > > return -1;
> > > > @@ -1486,12 +1500,7 @@ static int
> > > > vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> > > >
> > > > n = &user->notifier[queue_idx];
> > > >
> > > > - if (n->addr) {
> > > > - virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr,
> > > > false);
> > > > - object_unparent(OBJECT(&n->mr));
> > > > - munmap(n->addr, page_size);
> > > > - n->addr = NULL;
> > > > - }
> > > > + vhost_user_host_notifier_remove(user, vdev, queue_idx);
> > > >
> > > > if (area->u64 & VHOST_USER_VRING_NOFD_MASK) {
> > > > return 0;
> > > > @@ -1510,9 +1519,12 @@ static int
> > > > vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> > > >
> > > > name = g_strdup_printf("vhost-user/host-notifier@%p mmaps[%d]",
> > > > user, queue_idx);
> > > > - if (!n->mr.ram) /* Don't init again after suspend. */
> > > > + if (!n->mr.ram) { /* Don't init again after suspend. */
> > > > memory_region_init_ram_device_ptr(&n->mr, OBJECT(vdev), name,
> > > > page_size, addr);
> > > > + } else {
> > > > + n->mr.ram_block->host = addr;
> > > > + }
> > > > g_free(name);
> > > >
> > > > if (virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr,
> > > > true)) {
> > > > @@ -2460,17 +2472,17 @@ bool vhost_user_init(VhostUserState *user,
> > > > CharBackend *chr, Error **errp)
> > > > void vhost_user_cleanup(VhostUserState *user)
> > > > {
> > > > int i;
> > > > + VhostUserHostNotifier *n;
> > > >
> > > > if (!user->chr) {
> > > > return;
> > > > }
> > > > memory_region_transaction_begin();
> > > > for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> > > > - if (user->notifier[i].addr) {
> > > > - object_unparent(OBJECT(&user->notifier[i].mr));
> > > > - munmap(user->notifier[i].addr, qemu_real_host_page_size);
> > > > - user->notifier[i].addr = NULL;
> > > > - }
> > > > + n = &user->notifier[i];
> > > > + assert(!n->addr);
> > >
> > > I'm pretty confused as to why this assert holds.
> > > Add a comment?
> >
> > Seems notifiers are removed by vhost_user_get_vring_base(), I used this
> > assert to catch exception. Will remove it.
>
> Um I'm not actually asking about that. asserts are good but
> how do we know this one holds?
Normally notifier is removed in get_vring_base stage, not sure why here
we try to cleanup notifiers again. That's why I added assert to caputre
the case, just for debug. We don't need it for sure as formal version.
>
> > >
> > > > + vhost_user_host_notifier_remove(user, NULL, i);
> > > > + object_unparent(OBJECT(&n->mr));
> > > > }
> > > > memory_region_transaction_commit();
> > > > user->chr = NULL;
> > >
> > > I'm also confused on why we can do unparent for notifiers which have
> > > never been set up. Won't n->mr be invalid then?
> >
> > There is a parent check in object_unparent().
>
> It does not seem to be idempotent though in that it does not
> set parent to NULL. What if this is called twice?
This patch introduced why we need it:
1f89d3b91e3e ("hw/virtio: Fix leak of host-notifier memory-region")
If called twice, n->mr->parent is NULL and object_unparent() will do
nothing.
>
> > >
> > >
> > > > diff --git a/include/hw/virtio/vhost-user.h
> > > > b/include/hw/virtio/vhost-user.h
> > > > index f6012b2078..03aa22d450 100644
> > > > --- a/include/hw/virtio/vhost-user.h
> > > > +++ b/include/hw/virtio/vhost-user.h
> > > > @@ -12,8 +12,10 @@
> > > > #include "hw/virtio/virtio.h"
> > > >
> > > > typedef struct VhostUserHostNotifier {
> > > > + struct rcu_head rcu;
> > > > MemoryRegion mr;
> > > > void *addr;
> > > > + void *old_addr;
> > >
> > > That's not a very clear name. Is this literally just
> > > "address for the rcu callback to unmap"?
> > > Maybe unmap_addr then?
> >
> > LGTM, thanks!
> >
> >
> > >
> > > > } VhostUserHostNotifier;
> > > >
> > > > typedef struct VhostUserState {
> > > > --
> > > > 2.33.0
> > >
> >
>
