Re: [PATCH QEMU HCI-8.1 19/21] VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] migration: cpr-exec failure rollback does not restore vhost-net ownership
- [Low] migration: unpreserve_ram() ignores MADV_DONTEXEC failure in cpr-exec rollback
--

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

VZ: migration/cpr-exec: abort cpr-exec if memory can't be preserved

This commit makes cpr_exec_cb() jump to the failure path when
cpr_walk_ramblock(preserve_ram) fails, instead of calling execvp() anyway.
It adds unpreserve_ram() and cpr_exec_unpreserve_ram() to clear
MADV_DOEXEC on the saved ramblocks, and calls them from a shared fail:
label that execvp() failure also uses.

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

[ ... ]

> +static bool unpreserve_ram(cpr_ramblock_t ram, char *name)
> +{
> +#ifdef MADV_DONTEXEC
> +    qemu_madvise((void *) ram.addr, ram.length, MADV_DONTEXEC);
> +#endif
> +    return true;
> +}

[Severity: Low]
Can a failed MADV_DONTEXEC go unnoticed here?

The return value of qemu_madvise() in unpreserve_ram() is discarded.  If
the call fails for a block that preserve_ram() marked with MADV_DOEXEC, the
block keeps the flag in the surviving old QEMU, and nothing is logged.

By contrast, preserve_ram() reports a MADV_DOEXEC failure with
error_report_err().  The caller, cpr_exec_cb(), also drops the return value
of cpr_walk_ramblock(unpreserve_ram).

Would it be worth at least emitting an error_report() when the rollback
madvise fails?

[ ... ]

> @@ -221,15 +237,17 @@ static void cpr_exec_cb(void *opaque)
>      trace_cpr_exec();
>      execvp(argv[0], argv);

[ ... ]

> +fail:
> +    g_clear_pointer(&argv, g_strfreev);
> +    cpr_exec_unpreserve_fds();
> +    cpr_exec_unpreserve_ram();
> +
>      /* Note, we can go from state COMPLETED to FAILED */
>      migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);

[Severity: Medium]
Does this failure path restore vhost-net ownership after an execvp()
failure?

Earlier in cpr_exec_cb(), just before execvp(), this is called:

    vhost_net_cpr_reset_owners();
    trace_cpr_exec();
    execvp(argv[0], argv);

vhost_net_cpr_reset_owners() calls vhost_dev_reset_owner() for each kernel
vhost-net device, which issues VHOST_RESET_OWNER.

If execvp() fails, control reaches the fail: label.  The cleanup there runs
cpr_exec_unpreserve_fds() and cpr_exec_unpreserve_ram(), but nothing
re-issues VHOST_SET_OWNER.  The only caller of vhost_dev_set_owner() in the
net path is vhost_dev_init(), which runs once at device creation.

After vm_start(), virtio-net calls vhost_net_start() on devices that have
no owner.  My recollection is that the kernel rejects the vring and memory
ioctls in that state, because vhost_dev_check_owner() requires
dev->mm == current->mm.  I have not verified that against the kernel
source.

If that is right, virtio-net would log "unable to start vhost net ... falling
back on userspace virtio", or fail outright for vhost-force setups.

The new early goto fail after preserve_ram() failure runs before
vhost_net_cpr_reset_owners(), so it is not affected.  Only the execvp()
failure path is.

Since the commit message says the VM keeps running in the old QEMU on this
path, should the rollback also re-take ownership of the vhost devices?

A diff of migration/cpr-exec.c and hw/net/vhost_net.c from this commit to
the end of the series is empty, so no later patch appears to address this.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=19