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

[Stefano Garzarella]

On Mon, Nov 11, 2024 at 04:57:25PM +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.
>
> 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

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