Re: [PATCH QEMU HCI-8.1 v7 30/35] 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 1 potential issue(s) to consider:
- [Low] migration: unpreserve_ram() uses raw MADV_DONTEXEC and ignores madvise() result
--

commit 4dac8e6fe698ac777e5eb0a332bee47fd286faab
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 failure path instead
of calling execvp(). That path also clears MADV_DOEXEC from the saved
ramblocks again.

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

[ ... ]

> @@ -163,6 +163,19 @@ static bool preserve_ram(cpr_ramblock_t ram, char *name)
>      return true;
>  }
>  
> +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]
This isn't a bug, but unpreserve_ram() is asymmetric with preserve_ram().

preserve_ram() goes through the QEMU_MADV_DOEXEC wrapper and reports
failures. unpreserve_ram() instead guards on #ifdef MADV_DONTEXEC, passes
the raw constant to qemu_madvise(), and discards the return value.

include/qemu/madvise.h has no QEMU_MADV_DONTEXEC counterpart. Would it be
cleaner to add one?

Separately, suppose cpr_exec_cb() reaches the fail: label after a
preserve_ram() or execvp() failure, and the madvise() clear in
unpreserve_ram() fails. The old QEMU would then keep stale DOEXEC marks
with no diagnostic. Those marks could carry over into a later exec from
the same process, for example a retried cpr-exec.

Should a failure here at least be reported with error_report() or similar?

In the posix_madvise fallback build, QEMU_MADV_DOEXEC is
QEMU_MADV_INVALID, so preserve_ram() fails first. unpreserve_ram() then
passes MADV_DONTEXEC to posix_madvise(), which returns EINVAL. That is
harmless because nothing was marked.

> +static void cpr_exec_unpreserve_ram(void)
> +{
> +    cpr_walk_ramblock(unpreserve_ram);
> +}
> +

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103-vfio_v1-v7-0-4467fdccbbe6@virtuozzo.com?part=30