[pve-devel] [PATCH v4 pve-container 0/5] warn that nesting may be required

[pve-devel] [PATCH v4 pve-container 0/5] warn that nesting may be required

[Robert Obkircher]

This patch adds a task-log warning on CT start if systemd is detected.

Changes since v3:
- split and reordered the first 3 commits
- constrain and untaint the objdump path
- also warn in post_clone and post_create 

Changes since v2:
- read $@ before new eval to preserve error
- remove trailing whitespace

Changes since v1:
- increase minimum systemd version to something more reasonable
- introduce helper callback to log warinings
    - replace RESTEnvironmnet::log_warn in setup plugins
- syntactic changes:
    - renamed get_may_require_nesting_warning to check_systemd_nesting
    - use trailing if for return statements
    - call code from pre_start_hook as suggested


Robert Obkircher (5):
  Ensure that container startup warnings are displayed if startup fails.
  Propagate prestart-hook warnings to task-log.
  fix #6897: warn that nesting may be required for systemd
  fix #6897: constrain and untaint path for systemd version detection
  fix #6897: also warn in the post_clone and post_create hooks

 src/PVE/LXC.pm              |  6 ++++--
 src/PVE/LXC/Setup.pm        | 21 ++++++++++++++++++---
 src/PVE/LXC/Setup/Base.pm   | 30 ++++++++++++++++++++++++++++--
 src/PVE/LXC/Setup/Debian.pm |  5 ++---
 src/PVE/LXC/Setup/Plugin.pm |  2 +-
 src/PVE/LXC/Setup/Ubuntu.pm |  5 ++---
 src/lxc-pve-prestart-hook   | 24 +++++++++++-------------
 7 files changed, 66 insertions(+), 27 deletions(-)

-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v4 pve-container 1/5] Ensure that container startup warnings are displayed if startup fails.

[Robert Obkircher]

During container startup, warnings are written to a file, which is
later logged to the RESTEnvironment. This should also happen in the
presence of errors, so move it outside the eval block.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/PVE/LXC.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 89ccb54..44e20bc 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -2976,10 +2976,12 @@ sub vm_start {
 
         # if debug is requested, print the log it also when the start succeeded
         print_ct_stderr_log($vmid) if $is_debug;
-
+    };
+    my $err = $@;
+    eval {
         print_ct_warn_log($vmid); # always print warn log, if any
     };
-    if (my $err = $@) {
+    if ($err) {
         unlink $skiplock_flag_fn;
         die $err;
     }
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v4 pve-container 2/5] Propagate prestart-hook warnings to task-log.

[Robert Obkircher]

Add a callback to the LXC setup plugins to allow custom logging when
the RESTEnvironment is not available and use it to redirect warnings
to a file during the prestart-hook.

Only calls to RESTEnvironment::log_warn were migrated, calls to "warn"
are left unmodified for now.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/PVE/LXC/Setup.pm        |  9 ++++++---
 src/PVE/LXC/Setup/Base.pm   |  2 +-
 src/PVE/LXC/Setup/Debian.pm |  5 ++---
 src/PVE/LXC/Setup/Plugin.pm |  2 +-
 src/PVE/LXC/Setup/Ubuntu.pm |  5 ++---
 src/lxc-pve-prestart-hook   | 24 +++++++++++-------------
 6 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
index 87330c4..57b8df1 100644
--- a/src/PVE/LXC/Setup.pm
+++ b/src/PVE/LXC/Setup.pm
@@ -6,6 +6,7 @@ use warnings;
 use POSIX;
 use Cwd 'abs_path';
 
+use PVE::RESTEnvironment;
 use PVE::Tools;
 
 use PVE::LXC::Setup::Alpine;
@@ -97,11 +98,13 @@ my $autodetect_type = sub {
 };
 
 sub new {
-    my ($class, $conf, $rootdir, $type) = @_;
+    my ($class, $conf, $rootdir, $type, $log_warn) = @_;
 
     die "no root directory\n" if !$rootdir || $rootdir eq '/';
 
-    my $self = bless { conf => $conf, rootdir => $rootdir }, $class;
+    $log_warn ||= sub { PVE::RESTEnvironment::log_warn(@_); };
+
+    my $self = bless { conf => $conf, rootdir => $rootdir, log_warn => $log_warn }, $class;
 
     my $os_release = $self->get_ct_os_release();
 
@@ -121,7 +124,7 @@ sub new {
 
     my $plugin_class = $plugins->{$type} || die "no such OS type '$type'\n";
 
-    my $plugin = $plugin_class->new($conf, $rootdir, $os_release);
+    my $plugin = $plugin_class->new($conf, $rootdir, $os_release, $log_warn);
     $self->{plugin} = $plugin;
     $self->{in_chroot} = 0;
 
diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
index bb4a12b..2e1c84c 100644
--- a/src/PVE/LXC/Setup/Base.pm
+++ b/src/PVE/LXC/Setup/Base.pm
@@ -24,7 +24,7 @@ use PVE::LXC::Tools;
 use base qw(PVE::LXC::Setup::Plugin);
 
 sub new {
-    my ($class, $conf, $rootdir, $os_release) = @_;
+    my ($class, $conf, $rootdir, $os_release, $log_warn) = @_;
 
     return bless { conf => $conf, rootdir => $rootdir, os_release => $os_release }, $class;
 }
diff --git a/src/PVE/LXC/Setup/Debian.pm b/src/PVE/LXC/Setup/Debian.pm
index 030d934..dbb5050 100644
--- a/src/PVE/LXC/Setup/Debian.pm
+++ b/src/PVE/LXC/Setup/Debian.pm
@@ -6,7 +6,6 @@ use warnings;
 use PVE::Tools qw($IPV6RE);
 use PVE::LXC;
 use PVE::Network;
-use PVE::RESTEnvironment qw(log_warn);
 
 use File::Path;
 
@@ -20,7 +19,7 @@ use constant {
 };
 
 sub new {
-    my ($class, $conf, $rootdir) = @_;
+    my ($class, $conf, $rootdir, $os_release, $log_warn) = @_;
 
     my $version = PVE::Tools::file_read_firstline("$rootdir/etc/debian_version");
 
@@ -47,7 +46,7 @@ sub new {
     die "Container Debian version '$version' is too old\n" if $version < DEBIAN_MINIMUM_RELEASE;
 
     if ($version >= (DEBIAN_MAXIMUM_RELEASE + 1)) { # also allow all MAX.X point releases.
-        log_warn("The container's Debian version '$version' is newer than the tested version '"
+        $log_warn->("The container's Debian version '$version' is newer than the tested version '"
             . DEBIAN_MAXIMUM_RELEASE
             . "'. While everything may work fine, full compatibility cannot be guaranteed."
             . " Please check for PVE system updates.\n");
diff --git a/src/PVE/LXC/Setup/Plugin.pm b/src/PVE/LXC/Setup/Plugin.pm
index b9d9c2d..fbcfa8e 100644
--- a/src/PVE/LXC/Setup/Plugin.pm
+++ b/src/PVE/LXC/Setup/Plugin.pm
@@ -8,7 +8,7 @@ use warnings;
 use Carp;
 
 sub new {
-    my ($class, $conf, $rootdir, $os_release) = @_;
+    my ($class, $conf, $rootdir, $os_release, $log_warn) = @_;
     croak "implement me in sub-class\n";
 }
 
diff --git a/src/PVE/LXC/Setup/Ubuntu.pm b/src/PVE/LXC/Setup/Ubuntu.pm
index e364fa8..a213541 100644
--- a/src/PVE/LXC/Setup/Ubuntu.pm
+++ b/src/PVE/LXC/Setup/Ubuntu.pm
@@ -5,7 +5,6 @@ use warnings;
 
 use PVE::Tools;
 use PVE::LXC;
-use PVE::RESTEnvironment qw(log_warn);
 
 use File::Path;
 
@@ -43,7 +42,7 @@ my $known_versions = {
 };
 
 sub new {
-    my ($class, $conf, $rootdir) = @_;
+    my ($class, $conf, $rootdir, $os_release, $log_warn) = @_;
 
     my $lsb_fn = "$rootdir/etc/lsb-release";
     my $lsbinfo = PVE::Tools::file_get_contents($lsb_fn);
@@ -64,7 +63,7 @@ sub new {
         # cannot support 16.10 or older, their systemd is not cgroupv2 ready
         die "unsupported ancient Ubuntu version '$version'\n" if $major < 17;
 
-        log_warn("The container's Ubuntu version '$version' is not in the known version list."
+        $log_warn->("The container's Ubuntu version '$version' is not in the known version list."
             . " As it's newer than the minimum supported version it's likely to work OK, but full"
             . " compatibility cannot be guaranteed. Please check for PVE system updates.\n");
     } else {
diff --git a/src/lxc-pve-prestart-hook b/src/lxc-pve-prestart-hook
index 73125e1..f5dd728 100755
--- a/src/lxc-pve-prestart-hook
+++ b/src/lxc-pve-prestart-hook
@@ -28,17 +28,6 @@ eval {
     $have_sdn = 1;
 };
 
-my $WARNFD;
-
-sub log_warn {
-    my ($vmid, $message) = @_;
-
-    if (!defined($WARNFD)) {
-        open($WARNFD, '>', "/run/pve/ct-${vmid}.warnings");
-    }
-    print $WARNFD "$message\n";
-}
-
 PVE::LXC::Tools::lxc_hook(
     'pre-start',
     'lxc',
@@ -53,6 +42,15 @@ PVE::LXC::Tools::lxc_hook(
 
         PVE::RESTEnvironment->setup_default_cli_env();
 
+        my $warn_file = "/run/pve/ct-${vmid}.warnings";
+        # open eagerly so logging works inside the protected_call chroot
+        open(my $warnfd, '>', $warn_file) or die "Failed to open $warn_file: $!";
+        my $log_warn = sub {
+            my ($message) = @_;
+            print $warnfd "$message\n";
+            $warnfd->flush; # required because protected_call calls POSIX::_exit
+        };
+
         return undef if !-f PVE::LXC::Config->config_file($vmid);
 
         my $conf = PVE::LXC::Config->load_config($vmid);
@@ -155,12 +153,12 @@ PVE::LXC::Tools::lxc_hook(
 
         PVE::LXC::Config->foreach_passthrough_device($conf, $setup_passthrough_device);
 
-        my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir);
+        my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir, undef, $log_warn);
         $lxc_setup->pre_start_hook();
 
         if (PVE::CGroup::cgroup_mode() == 2) {
             if (!$lxc_setup->unified_cgroupv2_support()) {
-                log_warn(
+                $log_warn->(
                     $vmid,
                     "old systemd (< v232) detected, container won't run in a pure cgroupv2"
                         . " environment! Please see documentation -> container -> cgroup version.",
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v4 pve-container 3/5] fix #6897: warn that nesting may be required for systemd

[Robert Obkircher]

Recent versions of systemd require nesting to isolate services. If
nesting is disabled Debian 11 and 12 containers hang for 25 seconds
after login and Debian 13 just shows an empty console. To make this
less confusing for users, add a task-log warning on CT start if a
systemd version >241 (used by Debian 10) is detected.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/PVE/LXC/Setup.pm      | 10 ++++++++++
 src/PVE/LXC/Setup/Base.pm | 19 +++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
index 57b8df1..adb8d6c 100644
--- a/src/PVE/LXC/Setup.pm
+++ b/src/PVE/LXC/Setup.pm
@@ -296,10 +296,20 @@ sub rewrite_ssh_host_keys {
     });
 }
 
+sub check_systemd_nesting {
+    my ($self) = @_;
+
+    my $init = $self->get_ct_init_path();
+    # not a protected_call because it calls objdump
+    my $warning = $self->{plugin}->check_systemd_nesting($self->{conf}, $init);
+    $self->{log_warn}->($warning) if $warning;
+}
+
 sub pre_start_hook {
     my ($self) = @_;
 
     $self->protected_call(sub { $self->{plugin}->pre_start_hook($self->{conf}) });
+    $self->check_systemd_nesting();
 }
 
 sub post_clone_hook {
diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
index 2e1c84c..12e3097 100644
--- a/src/PVE/LXC/Setup/Base.pm
+++ b/src/PVE/LXC/Setup/Base.pm
@@ -648,6 +648,25 @@ sub get_ct_init_path {
     return $init_path;
 }
 
+sub check_systemd_nesting {
+    my ($self, $conf, $init) = @_;
+
+    my $features = PVE::LXC::Config->parse_features($conf->{features});
+    return if $features->{nesting};
+
+    return if (!defined($init) || $init !~ m@/systemd$@);
+
+    my $sdver = $self->get_systemd_version($init);
+
+    # 241 is the systemd version used by Debian 10. It was chosen based
+    # on a forum post that suggested enabling nesting for the upgrade
+    # from PMG 6.x to 7 and after a quick test where a Debian 11 container
+    # hung 25 seconds after login.
+    return if (!defined($sdver) || $sdver <= 241);
+
+    return "Systemd $sdver detected. You may need to enable nesting.";
+}
+
 sub ssh_host_key_types_to_generate {
     my ($self) = @_;
 
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v4 pve-container 4/5] fix #6897: constrain and untaint path for systemd version detection

[Robert Obkircher]

Ensure that the concatenated path stays within the container and
untaint it to make it callable from other hooks that run in taint mode
and would otherwise get an "Insecure dependency in exec" error.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/PVE/LXC/Setup/Base.pm | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
index 12e3097..bd595ab 100644
--- a/src/PVE/LXC/Setup/Base.pm
+++ b/src/PVE/LXC/Setup/Base.pm
@@ -604,9 +604,16 @@ sub clear_machine_id {
 sub get_systemd_version {
     my ($self, $init) = @_;
 
+    my $binary = abs_path($self->{rootdir} . $init);
+    if ($binary =~ /(^\Q$self->{rootdir}\E.*)/) {
+        $binary = $1; # untainted
+    } else {
+        die "Could not construct path to systemd binary: $self->{rootdir}, $init";
+    }
+
     my $version = undef;
     PVE::Tools::run_command(
-        ['objdump', '-p', $self->{rootdir} . $init],
+        ['objdump', '-p', $binary],
         outfunc => sub {
             my $line = shift;
             if ($line =~ /libsystemd-shared-(\d+)(?:[-_.][a-zA-Z0-9]+)*\.so:?$/) {
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v4 pve-container 4/5] fix #6897: constrain and untaint path when detecting systemd version

[Robert Obkircher]

Ensure that the concatenated path stays within the container and
untaint it to make it callable from other hooks that run in taint mode
and would otherwise get an "Insecure dependency in exec" error.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/PVE/LXC/Setup/Base.pm | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
index 12e3097..bd595ab 100644
--- a/src/PVE/LXC/Setup/Base.pm
+++ b/src/PVE/LXC/Setup/Base.pm
@@ -604,9 +604,16 @@ sub clear_machine_id {
 sub get_systemd_version {
     my ($self, $init) = @_;
 
+    my $binary = abs_path($self->{rootdir} . $init);
+    if ($binary =~ /(^\Q$self->{rootdir}\E.*)/) {
+        $binary = $1; # untainted
+    } else {
+        die "Could not construct path to systemd binary: $self->{rootdir}, $init";
+    }
+
     my $version = undef;
     PVE::Tools::run_command(
-        ['objdump', '-p', $self->{rootdir} . $init],
+        ['objdump', '-p', $binary],
         outfunc => sub {
             my $line = shift;
             if ($line =~ /libsystemd-shared-(\d+)(?:[-_.][a-zA-Z0-9]+)*\.so:?$/) {
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v4 pve-container 5/5] fix #6897: also warn in the post_clone and post_create hooks

[Robert Obkircher]

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/PVE/LXC/Setup.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
index adb8d6c..113093d 100644
--- a/src/PVE/LXC/Setup.pm
+++ b/src/PVE/LXC/Setup.pm
@@ -316,6 +316,7 @@ sub post_clone_hook {
     my ($self, $conf) = @_;
 
     $self->protected_call(sub { $self->{plugin}->post_clone_hook($conf) });
+    $self->check_systemd_nesting();
 }
 
 sub post_create_hook {
@@ -325,6 +326,7 @@ sub post_create_hook {
         $self->{plugin}->post_create_hook($self->{conf}, $root_password, $ssh_keys);
     });
     $self->rewrite_ssh_host_keys();
+    $self->check_systemd_nesting();
 }
 
 sub unified_cgroupv2_support {
-- 
2.47.3



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v4 pve-container 1/5] Ensure that container startup warnings are displayed if startup fails.

[Fiona Ebner]

Am 13.11.25 um 4:04 PM schrieb Robert Obkircher:
> During container startup, warnings are written to a file, which is
> later logged to the RESTEnvironment. This should also happen in the
> presence of errors, so move it outside the eval block.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>  src/PVE/LXC.pm | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 89ccb54..44e20bc 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -2976,10 +2976,12 @@ sub vm_start {
>  
>          # if debug is requested, print the log it also when the start succeeded
>          print_ct_stderr_log($vmid) if $is_debug;

Should the same be done for the error log here?

> -
> +    };
> +    my $err = $@;
> +    eval {
>          print_ct_warn_log($vmid); # always print warn log, if any
>      };

Nit: looking at the implementation of print_ct_warn_log(), it already
seems like it's written to avoid die-ing, so we could avoid the eval
here. Otherwise, I'd add a comment stating that the error from this eval
is ignored on purpose, but again, I don't think we need the eval here.

> -    if (my $err = $@) {
> +    if ($err) {
>          unlink $skiplock_flag_fn;
>          die $err;
>      }



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v4 pve-container 2/5] Propagate prestart-hook warnings to task-log.

[Fiona Ebner]

Am 13.11.25 um 4:04 PM schrieb Robert Obkircher:
> diff --git a/src/lxc-pve-prestart-hook b/src/lxc-pve-prestart-hook
> index 73125e1..f5dd728 100755
> --- a/src/lxc-pve-prestart-hook
> +++ b/src/lxc-pve-prestart-hook
> @@ -28,17 +28,6 @@ eval {
>      $have_sdn = 1;
>  };
>  
> -my $WARNFD;
> -
> -sub log_warn {
> -    my ($vmid, $message) = @_;
> -
> -    if (!defined($WARNFD)) {
> -        open($WARNFD, '>', "/run/pve/ct-${vmid}.warnings");
> -    }
> -    print $WARNFD "$message\n";
> -}
> -
>  PVE::LXC::Tools::lxc_hook(
>      'pre-start',
>      'lxc',
> @@ -53,6 +42,15 @@ PVE::LXC::Tools::lxc_hook(
>  
>          PVE::RESTEnvironment->setup_default_cli_env();
>  
> +        my $warn_file = "/run/pve/ct-${vmid}.warnings";
> +        # open eagerly so logging works inside the protected_call chroot

Nit: Maybe "early" instead of "eagerly"?

> +        open(my $warnfd, '>', $warn_file) or die "Failed to open $warn_file: $!";
> +        my $log_warn = sub {
> +            my ($message) = @_;

Nit: I'd add a chomp() so that callers don't need to worry about not
including a newline

> +            print $warnfd "$message\n";
> +            $warnfd->flush; # required because protected_call calls POSIX::_exit
> +        };
> +
>          return undef if !-f PVE::LXC::Config->config_file($vmid);
>  
>          my $conf = PVE::LXC::Config->load_config($vmid);
> @@ -155,12 +153,12 @@ PVE::LXC::Tools::lxc_hook(
>  
>          PVE::LXC::Config->foreach_passthrough_device($conf, $setup_passthrough_device);
>  
> -        my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir);
> +        my $lxc_setup = PVE::LXC::Setup->new($conf, $rootdir, undef, $log_warn);
>          $lxc_setup->pre_start_hook();
>  
>          if (PVE::CGroup::cgroup_mode() == 2) {
>              if (!$lxc_setup->unified_cgroupv2_support()) {
> -                log_warn(
> +                $log_warn->(
>                      $vmid,
>                      "old systemd (< v232) detected, container won't run in a pure cgroupv2"
>                          . " environment! Please see documentation -> container -> cgroup version.",

The call here still uses the VMID argument, but the new helper only
takes a single argument.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel