Re: [PATCH QEMU HCI-8.1 v2 21/24] 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:
- [Low] migration: preserve_ram() leaks original Error after migrate_set_error()
- [Low] migration: cpr-exec fail path leaves RAM read-only after execvp() failure
--

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

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

This patch makes cpr_exec_cb() treat a failed MADV_DOEXEC on any saved
ramblock as a failed cpr-exec. It jumps to a shared fail: label, which is
also reached when execvp() fails. That label restores close-on-exec on the
fds and clears MADV_DOEXEC on the ramblocks, so the VM keeps running in the
old QEMU.

> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> index 7b7609e19cd9..cb962ce72529 100644
> --- a/migration/cpr-exec.c
> +++ b/migration/cpr-exec.c

[ ... ]

> @@ -202,7 +215,10 @@ static void cpr_exec_cb(void *opaque)
>       */
>      cpr_exec_preserve_fds();
>  
> -    cpr_walk_ramblock(preserve_ram);
> +    if (!cpr_walk_ramblock(preserve_ram)) {
> +        /* preserve_ram() has already reported the error and failed migration */
> +        goto fail;
> +    }

[Severity: Low]
Does this new goto fail path leak the original Error object?

preserve_ram() builds the error like this:

    error_setg_errno(&err, errno, "MADV_DOEXEC failed on memory region %s:"
                     " addr=%p length=%zu", name, (void *) ram.addr,
                     ram.length);
    error_report_err(error_copy(err));
    migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
    migrate_set_error(s, err);
    return false;

error_report_err(error_copy(err)) frees only the copy, and
migrate_set_error() stores its own error_copy() without taking ownership.
No error_free(err) is reached before return false, so the original Error
(the struct and its msg string) is never freed.

The execvp() failure path in cpr_exec_cb() does call error_free(err) after
migrate_set_error(). The leak in preserve_ram() predates this patch, but
this patch now relies on that path to handle the failure. Would an
error_free(err) before return false in preserve_ram() be appropriate?

>      if (cpr_ram_rdonly) {
>          cpr_walk_ramblock(mark_ram_rdonly);
>      }

[ ... ]

> @@ -221,15 +237,17 @@ static void cpr_exec_cb(void *opaque)
>       * or the system is very short on resources.
>       */
>      error_setg_errno(&err, errno, "execvp %s failed", argv[0]);
> -    g_clear_pointer(&argv, g_strfreev);
> -    cpr_exec_unpreserve_fds();
> -
>      error_report_err(error_copy(err));
>      migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>      migrate_set_error(s, err);
>      error_free(err);
>      err = NULL;
>  
> +fail:
> +    g_clear_pointer(&argv, g_strfreev);
> +    cpr_exec_unpreserve_fds();
> +    cpr_exec_unpreserve_ram();
> +

[Severity: Low]
Can the guest be resumed on read-only RAM when execvp() fails?

If cpr_ram_rdonly is set, cpr_exec_cb() calls:

    if (cpr_ram_rdonly) {
        cpr_walk_ramblock(mark_ram_rdonly);
    }

before execvp(). If execvp() then fails, the code reaches fail:, which
clears MADV_DOEXEC and restores close-on-exec, but nothing restores the
RAM protection. cpr_ram_restore_rw() exists:

    void cpr_ram_restore_rw(void)
    {
        cpr_walk_ramblock(mark_ram_rw);
    }

but its only caller is system/vl.c, which runs in the new QEMU after a
successful exec.

The code after the label then runs:

    if (runstate_is_live(s->vm_old_state)) {
        vm_start();
    }

With RAM still PROT_READ, can the first guest write, or a device emulation
write into guest memory, raise SIGSEGV and kill the VM? That seems contrary
to the commit message's statement that "the VM keeps running in the old
QEMU".

The preserve_ram() failure path jumps to fail: before mark_ram_rdonly() and
vhost_net_cpr_reset_owners() run, so it is not affected. The execvp()
failure path is affected. cpr_ram_rdonly is documented as a debug-only
option, which limits the exposure. Should the fail: path call
cpr_ram_restore_rw() when cpr_ram_rdonly is set?

Later patches in the series (the one that takes the vhost-net devices back
if exec fails) add a vhost_net_cpr_set_owners() call before the fail:
label. None of the later patches in the series restore RAM to read-write
on this path.

>      /* 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/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=21