Re: [PATCH QEMU HCI-8.1 v2 22/24] 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 1 potential issue(s) to consider:
- [Low] vhost-net: cpr_set_owners() retries SET_OWNER on devices whose reset failed
--

commit 5dfb07a07fff9bf62ccc97d74b7cefc2e5892991
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

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

This patch adds vhost_net_cpr_set_owners(), which issues VHOST_SET_OWNER on
every kernel-backend vhost-net device. cpr_exec_cb() calls it after a failed
execvp(), so the old QEMU takes the devices back that it released with
VHOST_RESET_OWNER before the exec.

> 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

[ ... ]

> +void vhost_net_cpr_set_owners(void)
> +{
> +    NetClientState *nc;
> +
> +    QTAILQ_FOREACH(nc, &net_clients, next) {
> +        VHostNetState *net = get_vhost_net(nc);
> +
> +        if (net && net->dev.vhost_ops &&
> +            net->dev.vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL) {
> +            int r = vhost_dev_set_owner(&net->dev);
> +
> +            if (r < 0 && r != -ENOSYS) {
> +                warn_report("cpr: failed to set vhost-net owner for %s: %s",
> +                            nc->name, strerror(-r));
> +            }
> +        }
> +    }
> +}

[Severity: Low]
Can this issue VHOST_SET_OWNER on a device that was never released?

vhost_net_cpr_reset_owners() only warns when vhost_dev_reset_owner() fails
and does not record which devices were actually reset:

    int r = vhost_dev_reset_owner(&net->dev);

    if (r < 0 && r != -ENOSYS) {
        warn_report("cpr: failed to reset vhost-net owner for %s: %s",
                    nc->name, strerror(-r));
    }

A device whose reset failed is still owned by this mm. After a failed
execvp(), vhost_net_cpr_set_owners() then calls vhost_dev_set_owner() on it
as well. From my recollection of the kernel code, which I have not
verified, VHOST_SET_OWNER returns -EBUSY for a device that already has an
owner. If so, the rollback would log a second, misleading "cpr: failed to
set vhost-net owner" warning for that device. The device would still be
owned by the old QEMU, so I don't expect functional loss.

The vhost-vsock sibling avoids this. In vhost_vsock_pre_save(), owner_reset
is set only after a successful vhost_dev_reset_owner(), and
vhost_vsock_cpr_notifier() restores ownership only when it is set:

    if (e->type == MIG_EVENT_PRECOPY_FAILED && vsock->owner_reset) {
        ret = vhost_dev_set_owner(&vvc->vhost_dev);

Would it make sense to track per device whether the reset succeeded in
vhost_net_cpr_reset_owners(), and have vhost_net_cpr_set_owners() restore
only those devices?

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

[ ... ]

> @@ -243,6 +243,9 @@ static void cpr_exec_cb(void *opaque)
>      error_free(err);
>      err = NULL;
>  
> +    /* Still the same mm, take the vhost-net devices back */
> +    vhost_net_cpr_set_owners();
> +

This is the only rollback call site. It runs after a failed execvp() and
has no per-device information about which devices were released.

>  fail:
>      g_clear_pointer(&argv, g_strfreev);
>      cpr_exec_unpreserve_fds();

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=22