Re: [PATCH] vhost_net: fix assertion triggered by batch of host notifiers processing

[Zuo,Boqun]

On Thursday, November 14, 2024 6:54 PM, Stefano Garzarella wrote:
> >
> >When the backend of vhost_net restarts during the vm is running,
> >vhost_net is stopped and started. The virtio_device_grab_ioeventfd()
> >fucntion in
> >vhost_net_enable_notifiers() will result in a call to
> >virtio_bus_set_host_notifier()(assign=false).
> >
> >And now virtio_device_grab_ioeventfd() is batched in a single
> >transaction with virtio_bus_set_host_notifier()(assign=true).
> >
> >This triggers the following assertion:
> >
> >kvm_mem_ioeventfd_del: error deleting ioeventfd: Bad file descriptor
> >
> >This patch moves virtio_device_grab_ioeventfd() out of the batch to fix
> >this problem.
> >
> >Fixes: 6166799f6 ("vhost_net: configure all host notifiers in a single
> >MR transaction")
> >Reported-by: Gao Shiyuan <gaoshiyuan@baidu.com>
> >Signed-off-by: Zuo Boqun <zuoboqun@baidu.com>
> >---
> > hw/net/vhost_net.c | 31 ++++++++++++++++++++-----------
> > 1 file changed, 20 insertions(+), 11 deletions(-)
> >
> >diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index
> >997aab0557..eb3e92ca0d 100644
> >--- a/hw/net/vhost_net.c
> >+++ b/hw/net/vhost_net.c
> >@@ -229,9 +229,24 @@ static int vhost_net_enable_notifiers(VirtIODevice
> *dev,
> >     int nvhosts = data_queue_pairs + cvq;
> >     struct vhost_net *net;
> >     struct vhost_dev *hdev;
> >-    int r, i, j;
> >+    int r, i, j, k;
> >     NetClientState *peer;
> >
> >+    /*
> >+     * We will pass the notifiers to the kernel, make sure that QEMU
> >+     * doesn't interfere.
> >+     */
> >+    for (i = 0; i < nvhosts; i++) {
> >+        r = virtio_device_grab_ioeventfd(dev);
> >+        if (r < 0) {
> >+            error_report("vhost %d binding does not support host 
> >notifiers", i);
> >+            for (k = 0; k < i; k++) {
> >+                virtio_device_release_ioeventfd(dev);
> >+            }
> >+            return r;
> >+        }
> >+    }
> >+
> >     /*
> >      * Batch all the host notifiers in a single transaction to avoid
> >      * quadratic time complexity in address_space_update_ioeventfds().
> >@@ -247,16 +262,6 @@ static int vhost_net_enable_notifiers(VirtIODevice
> >*dev,
> >
> >         net = get_vhost_net(peer);
> >         hdev = &net->dev;
> >-        /*
> >-         * We will pass the notifiers to the kernel, make sure that QEMU
> >-         * doesn't interfere.
> >-         */
> >-        r = virtio_device_grab_ioeventfd(dev);
> >-        if (r < 0) {
> >-            error_report("binding does not support host notifiers");
> >-            memory_region_transaction_commit();
> >-            goto fail_nvhosts;
> >-        }
> >
> >         for (j = 0; j < hdev->nvqs; j++) {
> >             r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus),
> >@@ -277,6 +282,10 @@ static int vhost_net_enable_notifiers(VirtIODevice
> *dev,
> >     return 0;
> > fail_nvhosts:
> >     vhost_net_disable_notifiers_nvhosts(dev, ncs, data_queue_pairs,
> >i);
> 
> IIUC this call is disabling notifiers from 0 to `i - 1` ...
> 
> >+    for (k = i + 1; k < nvhosts; k++) {
> 
> ... so, should we start from `i`, instead of `i + 1`?
> 
> Thanks,
> Stefano

The notifier `i` has been released when vhost_net_enable_notifiers() call 
vhost_dev_disable_notifiers_nvqs():

        for (j = 0; j < hdev->nvqs; j++) {
            r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus),
                                             hdev->vq_index + j,
                                             true);
            if (r < 0) {
                error_report("vhost %d VQ %d notifier binding failed: %d",
                              i, j, -r);
                memory_region_transaction_commit();
=>            vhost_dev_disable_notifiers_nvqs(hdev, dev, j);
                goto fail_nvhosts;
            }
        }

So we should start from `i+1`...

> 
> >+        virtio_device_release_ioeventfd(dev);
> >+    }
> >+
> >     return r;
> > }
> >
> >--
> >2.42.0.windows.2
> >