Re: [PATCH QEMU HCI-8.1 v3 25/28] VZ: migration/cpr-exec: take the vhost-net devices back if exec fails

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vhost-net: cpr-exec failure path restores only SET_OWNER, not the vring err/call fds
- [Low] migration: cpr-exec calls vhost_net_cpr_set_owners() with no stub in vhost_net-stub.c
--

commit 4559492d0874959294013f874fff0e018375087a
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

VZ: migration/cpr-exec: take the vhost-net devices back if exec fails

When execvp() fails in cpr_exec_cb(), the old QEMU keeps running. This
patch records which vhost-net devices were released with
VHOST_RESET_OWNER in vhost_net_cpr_reset_owners(). It then takes them
back with VHOST_SET_OWNER through the new vhost_net_cpr_set_owners() on
the fail path.

> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c

[ ... ]

> +/*
> + * Take the ownership back when the exec fails and the VM goes on in the
> + * old QEMU.  VHOST_RESET_OWNER has reset the kernel device, but the devices
> + * are stopped, and vhost_net_start() sets everything up again when the VM
> + * resumes.
> + */
> +void vhost_net_cpr_set_owners(void)
> +{
> +    NetClientState *nc;
> +
> +    QTAILQ_FOREACH(nc, &net_clients, next) {
> +        VHostNetState *net = get_vhost_net(nc);
> +
> +        if (net && net->cpr_owner_reset) {
> +            int r = vhost_dev_set_owner(&net->dev);

[Severity: Medium]
Does restoring only the owner here leave the vhost device without its
per-virtqueue error and call eventfds? vhost_dev_init() registers them
right after SET_OWNER, through vhost_virtqueue_init():

    r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
    ...
    r = dev->vhost_ops->vhost_set_vring_err(dev, &file);

As far as I can tell, vhost_set_vring_err is called only from
vhost_virtqueue_init(), and that runs once from vhost_dev_init(). Neither
vhost_dev_start() nor vhost_virtqueue_start() calls it again. Would the
error fd therefore stay unregistered after VHOST_RESET_OWNER followed by
VHOST_SET_OWNER?

The call fd looks similar. In vhost_virtqueue_start() it is re-set only
when guest notifier masking is not in use:

    if (!vdev->use_guest_notifier_mask) {
        /* TODO: check and handle errors. */
        vhost_virtqueue_mask(dev, vdev, idx, false);
    }

With use_guest_notifier_mask and MSI-X, a vector that the guest keeps
masked seems to get no call fd after the reset. The initial setup had
pointed it at masked_notifier. Could a pending interrupt on such a vector
then be dropped, and go unseen by virtio_pci_vector_poll()?

I have not checked the kernel side. This assumes VHOST_RESET_OWNER clears
error_ctx and call_ctx, as vhost_dev_cleanup() does.

The comment above and the commit message say "vhost_net_start() sets up
the rest again when the VM resumes". Is that accurate for the vring err fd
and for the call fd of masked vectors?

> +
> +            if (r < 0) {
> +                warn_report("cpr: failed to set vhost-net owner for %s: %s",
> +                            nc->name, strerror(-r));
> +            } else {
> +                net->cpr_owner_reset = false;
>              }
>          }
>      }

[ ... ]

> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> --- a/include/net/vhost_net.h
> +++ b/include/net/vhost_net.h

[ ... ]

>  void vhost_net_cpr_reset_owners(void);
> +void vhost_net_cpr_set_owners(void);

[Severity: Low]
Is a stub needed for this function? hw/net/vhost_net-stub.c has no
vhost_net_cpr_reset_owners() or vhost_net_cpr_set_owners(). hw/net/meson.build
builds that stub file in place of vhost_net.c when vhost_net is not
available.

> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> --- a/migration/cpr-exec.c
> +++ b/migration/cpr-exec.c

[ ... ]

> +    /* Still the same mm, take back the vhost-net devices we released */
> +    vhost_net_cpr_set_owners();

[Severity: Low]
cpr_exec_cb() in migration/cpr-exec.c is always built and calls
vhost_net_cpr_set_owners() with no guard. In configurations that use
vhost_net-stub.c, would this give an undefined reference at link time?
The existing vhost_net_cpr_reset_owners() call in cpr_exec_cb() appears
to have the same problem, and this patch adds a second undefined symbol of
that kind.

>  
>      /* Note, we can go from state COMPLETED to FAILED */
>      migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261016-vfio_v1-v3-0-47487cb5a8e5@virtuozzo.com?part=25