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

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

Posted by Zuo boqun via 1 year, 2 months ago

From: zuoboqun <zuoboqun@baidu.com>

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.

To be noted that the for loop to release ioeventfd should start from i+1,
not i, because the i-th ioeventfd has already been released in
vhost_dev_disable_notifiers_nvqs().

Fixes: 6166799f6 ("vhost_net: configure all host notifiers in a single MR transaction")
Signed-off-by: Zuo Boqun <zuoboqun@baidu.com>
Reported-by: Gao Shiyuan <gaoshiyuan@baidu.com>

---

v1->v2:
    *To explain why the for loop to release ioeventfd starts from i+1:
      1) add a comment in the code
      2) describe it in the commit message
---
 hw/net/vhost_net.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 997aab0557..891f235a0a 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,14 @@ static int vhost_net_enable_notifiers(VirtIODevice *dev,
     return 0;
 fail_nvhosts:
     vhost_net_disable_notifiers_nvhosts(dev, ncs, data_queue_pairs, i);
+    /*
+     * This for loop starts from i+1, not i, because the i-th ioeventfd
+     * has already been released in vhost_dev_disable_notifiers_nvqs().
+     */
+    for (k = i + 1; k < nvhosts; k++) {
+        virtio_device_release_ioeventfd(dev);
+    }
+
     return r;
 }
 
-- 
2.42.0.windows.2

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

Posted by Jason Wang 1 year, 2 months ago

On Fri, Nov 15, 2024 at 4:03 PM Zuo boqun <zuoboqun@baidu.com> wrote:
>
> From: zuoboqun <zuoboqun@baidu.com>
>
> 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.
>
> To be noted that the for loop to release ioeventfd should start from i+1,
> not i, because the i-th ioeventfd has already been released in
> vhost_dev_disable_notifiers_nvqs().
>
> Fixes: 6166799f6 ("vhost_net: configure all host notifiers in a single MR transaction")
> Signed-off-by: Zuo Boqun <zuoboqun@baidu.com>
> Reported-by: Gao Shiyuan <gaoshiyuan@baidu.com>

I think we need cc stable for this.

>
> ---
>
> v1->v2:
>     *To explain why the for loop to release ioeventfd starts from i+1:
>       1) add a comment in the code
>       2) describe it in the commit message
> ---
>  hw/net/vhost_net.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 997aab0557..891f235a0a 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;
> +        }

Could we tweak the code to reuse the fail_nvhosts?

> +    }
> +
>      /*
>       * 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,14 @@ static int vhost_net_enable_notifiers(VirtIODevice *dev,
>      return 0;
>  fail_nvhosts:
>      vhost_net_disable_notifiers_nvhosts(dev, ncs, data_queue_pairs, i);
> +    /*
> +     * This for loop starts from i+1, not i, because the i-th ioeventfd
> +     * has already been released in vhost_dev_disable_notifiers_nvqs().
> +     */
> +    for (k = i + 1; k < nvhosts; k++) {
> +        virtio_device_release_ioeventfd(dev);
> +    }
> +
>      return r;
>  }
>
> --
> 2.42.0.windows.2
>

Thanks

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

Posted by Stefano Garzarella 1 year, 2 months ago

On Fri, Nov 15, 2024 at 04:03:12PM +0800, Zuo boqun wrote:
>From: zuoboqun <zuoboqun@baidu.com>
>
>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.
>
>To be noted that the for loop to release ioeventfd should start from i+1,
>not i, because the i-th ioeventfd has already been released in
>vhost_dev_disable_notifiers_nvqs().
>
>Fixes: 6166799f6 ("vhost_net: configure all host notifiers in a single MR transaction")
>Signed-off-by: Zuo Boqun <zuoboqun@baidu.com>
>Reported-by: Gao Shiyuan <gaoshiyuan@baidu.com>
>
>---
>
>v1->v2:
>    *To explain why the for loop to release ioeventfd starts from i+1:
>      1) add a comment in the code
>      2) describe it in the commit message

Thanks for that!

LGTM, but I don't know vhost-net enough so I'd wait for a comment from 
Michael and Jason.

Acked-by: Stefano Garzarella <sgarzare@redhat.com>


>---
> hw/net/vhost_net.c | 35 ++++++++++++++++++++++++-----------
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
>diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>index 997aab0557..891f235a0a 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,14 @@ static int vhost_net_enable_notifiers(VirtIODevice *dev,
>     return 0;
> fail_nvhosts:
>     vhost_net_disable_notifiers_nvhosts(dev, ncs, data_queue_pairs, i);
>+    /*
>+     * This for loop starts from i+1, not i, because the i-th ioeventfd
>+     * has already been released in vhost_dev_disable_notifiers_nvqs().
>+     */
>+    for (k = i + 1; k < nvhosts; k++) {
>+        virtio_device_release_ioeventfd(dev);
>+    }
>+
>     return r;
> }
>
>-- 
>2.42.0.windows.2
>