Re: [PATCH QEMU HCI-8.1 v4 05/31] migration: close the CPR state if the outgoing channel fails to open

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: cpr-exec dup'd memfd and env var leak when channel open fails
--

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

migration: close the CPR state if the outgoing channel fails to open

This patch adds a cpr_state_close() call to qmp_migrate_finish(). The call
runs when the outgoing channel (file or exec) fails to open. Its purpose is
to release the CPR state that qmp_migrate() saved earlier.

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

[ ... ]

> @@ -2412,6 +2412,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
>          if (!resume_requested) {
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          }
> +        cpr_state_close();
>          migration_connect_set_error(s, local_err);
>          error_propagate(errp, local_err);
>          return;

[Severity: Medium]
The commit message says that without this change "the CPR state stays open:
the memfd for cpr-exec, the state file for cpr-transfer". Does this call
close everything for cpr-exec?

cpr_state_close() only does qemu_fclose(cpr_state_file). That closes the
original memfd.

In cpr-exec mode, cpr_state_save() calls cpr_exec_persist_state(). That
function creates a second reference to the memfd and sets an environment
variable:

migration/cpr-exec.c:cpr_exec_persist_state() {
    int mfd = dup(fioc->fd);
    ...
    qemu_clear_cloexec(mfd);
    ...
    g_setenv(CPR_EXEC_STATE_NAME, val, 1)
}

Nothing on this error path releases the dup'd fd or unsets
QEMU_CPR_EXEC_STATE. Only cpr_exec_unpersist_state() does that:

migration/cpr-exec.c:cpr_exec_unpersist_state() {
    g_unsetenv(CPR_EXEC_STATE_NAME);
    ...
    close(mfd);
}

It is called only from cpr_exec_notifier() on MIG_EVENT_PRECOPY_FAILED. This
event is raised from migration_cleanup(), from the postcopy failure path,
and from cpr_exec_cb() after a failed exec. None of these runs when
qmp_migrate_finish() fails to open the channel, because migration_cleanup()
doesn't run. migration_connect_set_error() only sets the state and the error.

Can a cpr-exec migrate to an exec: or file: URI that fails to open leak the
dup'd memfd? Each retry would leak one more fd, with CLOEXEC already cleared.
It would also leave a stale QEMU_CPR_EXEC_STATE variable pointing at that fd.
cpr_exec_has_state() would then keep returning true.

Should this path also call cpr_exec_unpersist_state(), or raise
MIG_EVENT_PRECOPY_FAILED, so the cpr-exec cleanup runs?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=5