Re: [PATCH 2/3] chardev: preserve the logfile fd across cpr-exec #VSTOR-148304

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] chardev: logfd saved in all CPR modes but claimed only on cpr-exec incoming
--

commit 05df918f015920ae0431d59bdf9bb6ff8741e4fc
Author: Denis V. Lunev <den@openvz.org>

chardev: preserve the logfile fd across cpr-exec #VSTOR-148304

This patch saves the chardev log fd in the cpr fd registry under the chardev label. On cpr-exec incoming, it takes the fd from the registry instead of reopening the logfile path. If the source did not save the fd, the chardev runs without a log.

> diff --git a/chardev/char.c b/chardev/char.c
> --- a/chardev/char.c
> +++ b/chardev/char.c

[ ... ]

> @@ -261,9 +262,21 @@ static void qemu_char_open(Chardev *chr, ChardevBackend *backend,
>          } else {
>              flags |= O_TRUNC;
>          }
> -        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
> -        if (chr->logfd < 0) {
> -            return;
> +        if (cpr_exec_is_incoming()) {
> +            chr->logfd = cpr_find_fd(chr->label, CPR_CHARDEV_LOG_ID);
> +            if (chr->logfd < 0) {
> +                warn_report("chardev '%s': log file was not preserved, "
> +                            "continuing without it", chr->label);
> +            }
> +        } else {
> +            chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
> +            if (chr->logfd < 0) {
> +                return;
> +            }
> +            if (!cpr_is_incoming()) {
> +                cpr_delete_fd(chr->label, CPR_CHARDEV_LOG_ID);
> +                cpr_save_fd(chr->label, CPR_CHARDEV_LOG_ID, chr->logfd);
> +            }
>          }
>      }

[Severity: Low]
Is the log fd handled consistently across cpr modes here? The fd is saved
to the registry for every start where cpr_is_incoming() is false, but it
is only claimed when cpr_exec_is_incoming() is true.

In cpr-transfer, the target takes the else branch in qemu_char_open():

    chr->logfd = qemu_create(common->logfile, flags, 0666, errp);

so it reopens the logfile path itself. The save is then skipped because
cpr_is_incoming() is true. Can this leave the transferred fd unclaimed in
the registry, and also hit the same permission and virtlogd-bypass problem
the commit message describes for cpr-exec?

This follows the exec-only pattern in char-file.c and char-pty.c. I did not
verify whether cpr-transfer sends every registry fd. If it does not, the
leak does not occur, but the path reopen remains.

[ ... ]

> @@ -315,11 +332,14 @@ static void char_finalize(Object *obj)
>      if (chr->be) {
>          chr->be->chr = NULL;
>      }
> -    g_free(chr->filename);
> -    g_free(chr->label);
>      if (chr->logfd != -1) {
> +        if (cpr_find_fd(chr->label, CPR_CHARDEV_LOG_ID) == chr->logfd) {
> +            cpr_delete_fd(chr->label, CPR_CHARDEV_LOG_ID);
> +        }
>          close(chr->logfd);
>      }
> +    g_free(chr->filename);
> +    g_free(chr->label);
>      qemu_mutex_destroy(&chr->chr_write_lock);
>  }

[Severity: Low]
In the cpr-transfer target, can the registry entry outlive the chardev
here? cpr_find_fd() in char_finalize() returns the transferred fd, which
differs from chr->logfd, so cpr_delete_fd() is not called.

Would the stale registry entry and its open fd then stay until process
exit?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261002164853.2130337-1-den@openvz.org?part=2