[PATCH qemu-server v2 0/1] fix #7590: apply timeout to QEMU start fork

[PATCH qemu-server v2 0/1] fix #7590: apply timeout to QEMU start fork

[Samuel Rufinatscha]

The qmstart task forks a child process that enters the VM scope, starts
the helper processes and launches QEMU. qmstart then waits for the
child to report success or failure. While regular starts already apply
$start_timeout to the QEMU command, the wait for the forked child has
no timeout. Statefile starts do not use the QEMU command timeout
either, leaving both waits unbounded.

Timeout handling currently:

  Start type                       QEMU command timeout    forked startup timeout
  ------------------------------------------------------------------------------
  regular                          $start_timeout          none
  statefile                        none                    none

Timeout handling after this series:

  Start type                       QEMU command timeout    fork startup timeout
  ------------------------------------------------------------------------------
  regular                          $start_timeout          2 * $start_timeout (account for helpers)
  statefile with --timeout N       none                    N
  statefile without --timeout      none                    24 hours (as fallback)

Regular starts retain the existing QEMU command timeout and is twice
for the outer timeout to account for VM scope and helper setup.
Statefile starts continue to have no separate QEMU
command timeout. If --timeout N is specified, N is used as the deadline
for the complete forked startup, otherwise a 24 hour fallback is used.
Values can be changed if needed.

Tested on a PVE 9.2 node:
- the statefile/FIFO timeout released the configuration lock and left
  no QEMU process or VM scope behind, using the reproducer from [1]
- the existing regular QEMU timeout still fired before the outer
  safeguard
- an immediate QEMU startup error performed the same scope cleanup
- successful startup and shutdown continued to work

Note: the separate D-state waitpid() FIXME in
run_fork_with_timeout() [2] concerns the timeout handling itself
potentially blocking indefinitely and is therefore out of scope for
this series.

[0] https://lore.proxmox.com/pve-devel/20260625114500.159384-1-s.rufinatscha@proxmox.com/
[1] https://bugzilla.proxmox.com/show_bug.cgi?id=7590
[2] https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/Tools.pm;h=8e7646dea43e092fc9ec76b493b418a7ca0eaee2;hb=5054082fe492429fc37574985c2ca812af9a3125#l604

Samuel Rufinatscha (1):
  fix #7590: qemu-server: apply timeout to QEMU start fork

 src/PVE/QemuServer.pm | 93 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 77 insertions(+), 16 deletions(-)

-- 
2.47.3

[PATCH qemu-server v2 1/1] fix #7590: qemu-server: apply timeout to QEMU start fork

[Samuel Rufinatscha]

The qmstart task could wait indefinitely for the forked startup child
while holding the VM configuration lock.

Apply an outer timeout to the forked startup path. Regular starts keep
the existing QEMU command timeout, while the outer timeout is twice
that value to allow for VM scope and helper setup.

Statefile starts continue without a separate QEMU command timeout. When
--timeout N is specified, N bounds the complete forked startup
otherwise, use a 24 hour fallback safeguard.

Handle startup errors and timeouts in the same cleanup path. Pass the
swtpm PID and process start time to the parent, stop swtpm, clean up
QSD, then kill the processes remaining in the VM scope.

Signed-off-by: Samuel Rufinatscha <s.rufinatscha@proxmox.com>
Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7590
---
Changes since v1:
- account for VM scope and helper setup without reducing QEMU's existing
  timeout
- use the existing --timeout value for statefile starts, or a 24-hour
  fallback safeguard
- pass the swtpm PID and process start time from the startup child to the
  parent
- use the same cleanup path for startup failures and timeouts
- clean up swtpm and QSD before killing the VM scope
- explicitly clean up the scope after regular startup failures too

 thanks @Fabian for the review!

 src/PVE/QemuServer.pm | 93 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 77 insertions(+), 16 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 9aec7f9c..675f767a 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -5481,6 +5481,10 @@ my $log_filter_catch_outdated_zen5_firmware = sub {
     }
 };
 
+# Statefile starts can take a long time, depending on the state size and I/O throughput,
+# so only use a very high safeguard.
+my $STATEFILE_START_FORK_TIMEOUT = 24 * 60 * 60;
+
 # see vm_start_nolock for parameters, additionally:
 # migrate_opts:
 #   storagemap = parsed storage map for allocating NBD disks
@@ -5781,43 +5785,100 @@ sub vm_start_nolock {
     }
     $systemd_properties{timeout} = 10 if $statefile; # setting up the scope should be quick
 
-    my $cleanup_qsd = sub {
+    my $cleanup_failed_start = sub {
+        my ($tpmpid, $tpmpstart) = @_;
+
+        if ($tpmpid && PVE::ProcFSTools::check_process_running($tpmpid, $tpmpstart)) {
+            warn "stopping swtpm instance (pid $tpmpid) due to QEMU startup error\n";
+            kill 'TERM', $tpmpid;
+        }
+
         if (PVE::QemuServer::Helpers::qsd_running_locally($vmid)) {
             eval { PVE::QemuServer::QSD::quit($vmid); };
             warn "stopping QEMU storage daemon failed - $@" if $@;
         }
+
+        eval {
+            run_command(
+                [
+                    '/bin/systemctl', 'kill',
+                    '--kill-whom=all',
+                    '--signal=KILL',
+                    "$vmid.scope",
+                ],
+                %silence_std_outs,
+                noerr => 1,
+                timeout => 10,
+            );
+            PVE::Systemd::wait_for_unit_removed("$vmid.scope", 20);
+        };
+        warn "failed to clean up VM start scope - $@" if $@;
     };
 
     my $run_qemu = sub {
-        PVE::Tools::run_fork sub {
+        pipe(my $tpm_reader, my $tpm_writer)
+            or die "failed to create swtpm process info pipe: $!\n";
+        my $flags = fcntl($tpm_writer, F_GETFD, 0)
+            // die "failed to get swtpm process info pipe flags: $!\n";
+        fcntl($tpm_writer, F_SETFD, $flags | FD_CLOEXEC)
+            // die "failed to set CLOEXEC on swtpm process info pipe: $!\n";
+
+        my $run_qemu_child = sub {
+            close($tpm_reader);
+
             PVE::Systemd::enter_systemd_scope($vmid, "Proxmox VE VM $vmid",
                 %systemd_properties);
 
             my $virtiofs_sockets = start_all_virtiofsd($conf, $vmid);
 
-            my $tpmpid;
             if ((my $tpm = $conf->{tpmstate0}) && !PVE::QemuConfig->is_template($conf)) {
-                # start the TPM emulator so QEMU can connect on start
-                eval { $tpmpid = start_swtpm($storecfg, $vmid, $tpm, $migratedfrom); };
-                if (my $err = $@) {
-                    $cleanup_qsd->();
-                    die $err;
-                }
+                my $tpmpid = start_swtpm($storecfg, $vmid, $tpm, $migratedfrom);
+                my $tpmpstart = PVE::ProcFSTools::read_proc_starttime($tpmpid)
+                    or die "failed to read swtpm process start time\n";
+                my $msg = "$tpmpid $tpmpstart\n";
+                syswrite($tpm_writer, $msg) == length($msg)
+                    or die "failed to send swtpm process info to parent: $!\n";
             }
+            close($tpm_writer);
 
             my $exitcode = run_command($cmd, %run_params);
             eval { PVE::QemuServer::Virtiofs::close_sockets(@$virtiofs_sockets); };
             log_warn("closing virtiofs sockets failed - $@") if $@;
-            if ($exitcode) {
-                if ($tpmpid) {
-                    warn "stopping swtpm instance (pid $tpmpid) due to QEMU startup error\n";
-                    kill 'TERM', $tpmpid;
-                }
-                $cleanup_qsd->();
+            die "QEMU exited with code $exitcode\n" if $exitcode;
+        };
 
-                die "QEMU exited with code $exitcode\n";
+        my ($tpmpid, $tpmpstart);
+        my $afterfork = sub {
+            close($tpm_writer);
+            if (defined(my $msg = <$tpm_reader>)) {
+                die "received invalid swtpm process info from startup child\n"
+                    if $msg !~ m/^(\d+) (\d+)\n$/;
+                ($tpmpid, $tpmpstart) = ($1, $2);
             }
+            close($tpm_reader);
         };
+
+        # Allow the same amount of time once more for entering the scope and starting helpers.
+        my $fork_timeout = $statefile
+            ? ($params->{timeout} // $STATEFILE_START_FORK_TIMEOUT)
+            : 2 * $start_timeout;
+        $fork_timeout = undef if !$fork_timeout;
+
+        my $timed_out;
+        eval {
+            (undef, $timed_out) = PVE::Tools::run_fork_with_timeout(
+                $fork_timeout,
+                $run_qemu_child,
+                { afterfork => $afterfork },
+            );
+        };
+        my $err = $@;
+        $err = "QEMU start timed out after $fork_timeout seconds\n" if $timed_out;
+
+        if ($err) {
+            $cleanup_failed_start->($tpmpid, $tpmpstart);
+            die $err;
+        }
     };
 
     if ($conf->{hugepages}) {
-- 
2.47.3