[pve-devel] [PATCH v3 container] fix #4192: revamp check for systemd version

[pve-devel] [PATCH v3 container] fix #4192: revamp check for systemd version

[Leo Nunner]

Instead of iterating through several folders, it might just be easier to
check the objdump output of /sbin/init and getting the version from there.
Resolving the /sbin/init symlink happens inside the chroot, but the
objdump from the host system is used, as to not run any untrusted
executables.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
I think putting the subroutine to resolve the /sbin/init symlink into
Setup.pm makes the most sense, since this isn't realy a task for the
plugin.

 src/PVE/LXC/Setup.pm           | 18 +++++++++++++++++-
 src/PVE/LXC/Setup/Alpine.pm    |  2 +-
 src/PVE/LXC/Setup/Base.pm      | 34 ++++++++++++++++++----------------
 src/PVE/LXC/Setup/Devuan.pm    |  2 +-
 src/PVE/LXC/Setup/Plugin.pm    |  2 +-
 src/PVE/LXC/Setup/Unmanaged.pm |  2 +-
 6 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/src/PVE/LXC/Setup.pm b/src/PVE/LXC/Setup.pm
index b72a18e..fe6f0db 100644
--- a/src/PVE/LXC/Setup.pm
+++ b/src/PVE/LXC/Setup.pm
@@ -285,7 +285,7 @@ sub post_create_hook {
 sub unified_cgroupv2_support {
     my ($self) = @_;
 
-    return $self->protected_call(sub { $self->{plugin}->unified_cgroupv2_support() });
+    return $self->{plugin}->unified_cgroupv2_support($self->get_ct_init_path());
 }
 
 # os-release(5):
@@ -335,4 +335,20 @@ sub get_ct_os_release {
     return &$parse_os_release($data);
 }
 
+# Checks whether /sbin/init is a symlink, and if it is, 
+# resolves it to the actual binary
+sub get_ct_init_path { 
+    my ($self) = @_;
+
+    my $init = $self->protected_call(sub {
+	my $init_path = "/sbin/init";
+	if($self->{plugin}->ct_is_symlink($init_path)) {
+    	    $init_path = $self->{plugin}->ct_readlink($init_path);
+	}
+	return $init_path;
+    });
+
+    return $init;
+}
+
 1;
diff --git a/src/PVE/LXC/Setup/Alpine.pm b/src/PVE/LXC/Setup/Alpine.pm
index b56d895..87d72be 100644
--- a/src/PVE/LXC/Setup/Alpine.pm
+++ b/src/PVE/LXC/Setup/Alpine.pm
@@ -102,7 +102,7 @@ sub setup_network {
 
 # non systemd based containers work with pure cgroupv2
 sub unified_cgroupv2_support {
-    my ($self) = @_;
+    my ($self, $init) = @_;
 
     return 1;
 }
diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
index cc12914..09155cf 100644
--- a/src/PVE/LXC/Setup/Base.pm
+++ b/src/PVE/LXC/Setup/Base.pm
@@ -514,40 +514,42 @@ sub clear_machine_id {
     }
 }
 
-# tries to guess the systemd (major) version based on the existence of
-# (/usr)?/lib/systemd/libsystemd-shared<version>.so. It was introduced in v231.
+# tries to guess the systemd (major) version based on the
+# libsystemd-shared<version>.so linked with /sbin/init
 sub get_systemd_version {
-    my ($self) = @_;
+    my ($self, $init) = @_;
 
-    my $sd_lib_dir = $self->ct_is_directory("/lib/systemd") ?
-	"/lib/systemd" : "/usr/lib/systemd";
-    my $libsd = PVE::Tools::dir_glob_regex($sd_lib_dir, "libsystemd-shared-.+\.so");
-    if (defined($libsd) && $libsd =~ /libsystemd-shared-(\d+)(?:\..*)?\.so/) {
-	return $1;
-    }
+    my $version = undef;
+    PVE::Tools::run_command(
+	['objdump', '-p', $self->{rootdir}.$init],
+	outfunc => sub {
+	    my $line = shift;
+	    if ($line =~ /libsystemd-shared-(\d+)(?:\.[a-zA-Z0-9]*)?\.so:$/) {
+		$version = $1;
+	    }},
+	errmsg => "objdump on $init failed",
+    );
 
-    return undef;
+    return $version;
 }
 
 sub unified_cgroupv2_support {
-    my ($self) = @_;
+    my ($self, $init) = @_;
 
     # https://www.freedesktop.org/software/systemd/man/systemd.html
     # systemd is installed as symlink to /sbin/init
-    my $systemd = $self->ct_readlink('/sbin/init');
-
     # assume non-systemd init will run with unified cgroupv2
-    if (!defined($systemd) || $systemd !~ m@/systemd$@) {
+    if (!defined($init) || $init !~ m@/systemd$@) {
 	return 1;
     }
 
     # systemd version 232 (e.g. debian stretch) supports the unified hierarchy
-    my $sdver = $self->get_systemd_version();
+    my $sdver = $self->get_systemd_version($init);
     if (!defined($sdver) || $sdver < 232) {
 	return 0;
     }
 
-    return 1
+    return 1;
 }
 
 sub ssh_host_key_types_to_generate {
diff --git a/src/PVE/LXC/Setup/Devuan.pm b/src/PVE/LXC/Setup/Devuan.pm
index 3e15bb2..059f145 100644
--- a/src/PVE/LXC/Setup/Devuan.pm
+++ b/src/PVE/LXC/Setup/Devuan.pm
@@ -42,7 +42,7 @@ sub new {
 
 # non systemd based containers work with pure cgroupv2
 sub unified_cgroupv2_support {
-    my ($self) = @_;
+    my ($self, $init) = @_;
 
     return 1;
 }
diff --git a/src/PVE/LXC/Setup/Plugin.pm b/src/PVE/LXC/Setup/Plugin.pm
index 8458ad8..7024856 100644
--- a/src/PVE/LXC/Setup/Plugin.pm
+++ b/src/PVE/LXC/Setup/Plugin.pm
@@ -48,7 +48,7 @@ sub set_user_password {
 }
 
 sub unified_cgroupv2_support {
-    my ($self) = @_;
+    my ($self, $init) = @_;
     croak "implement me in sub-class\n";
 }
 
diff --git a/src/PVE/LXC/Setup/Unmanaged.pm b/src/PVE/LXC/Setup/Unmanaged.pm
index 3b9febf..280af04 100644
--- a/src/PVE/LXC/Setup/Unmanaged.pm
+++ b/src/PVE/LXC/Setup/Unmanaged.pm
@@ -45,7 +45,7 @@ sub set_user_password {
 }
 
 sub unified_cgroupv2_support {
-    my ($self) = @_;
+    my ($self, $init) = @_;
     return 1; # faking it won't normally hurt ;-)
 }
 
-- 
2.30.2

[pve-devel] applied: [PATCH v3 container] fix #4192: revamp check for systemd version

[Thomas Lamprecht]

Am 15/09/2022 um 13:52 schrieb Leo Nunner:
> Instead of iterating through several folders, it might just be easier to
> check the objdump output of /sbin/init and getting the version from there.
> Resolving the /sbin/init symlink happens inside the chroot, but the
> objdump from the host system is used, as to not run any untrusted
> executables.
> 
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
> I think putting the subroutine to resolve the /sbin/init symlink into
> Setup.pm makes the most sense, since this isn't realy a task for the
> plugin.
> 
>  src/PVE/LXC/Setup.pm           | 18 +++++++++++++++++-
>  src/PVE/LXC/Setup/Alpine.pm    |  2 +-
>  src/PVE/LXC/Setup/Base.pm      | 34 ++++++++++++++++++----------------
>  src/PVE/LXC/Setup/Devuan.pm    |  2 +-
>  src/PVE/LXC/Setup/Plugin.pm    |  2 +-
>  src/PVE/LXC/Setup/Unmanaged.pm |  2 +-
>  6 files changed, 39 insertions(+), 21 deletions(-)
> 
>

applied, with a small followup for a few small white-space/indentation errors,
otherwise nice work - thanks!

Re: [pve-devel] [PATCH v3 container] fix #4192: revamp check for systemd version

[Thomas Lamprecht]

Am 08/11/2022 um 18:23 schrieb Thomas Lamprecht:
> Am 15/09/2022 um 13:52 schrieb Leo Nunner:
>> Instead of iterating through several folders, it might just be easier to
>> check the objdump output of /sbin/init and getting the version from there.
>> Resolving the /sbin/init symlink happens inside the chroot, but the
>> objdump from the host system is used, as to not run any untrusted
>> executables.
>>
>> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
>> ---
>> I think putting the subroutine to resolve the /sbin/init symlink into
>> Setup.pm makes the most sense, since this isn't realy a task for the
>> plugin.
>>
>>  src/PVE/LXC/Setup.pm           | 18 +++++++++++++++++-
>>  src/PVE/LXC/Setup/Alpine.pm    |  2 +-
>>  src/PVE/LXC/Setup/Base.pm      | 34 ++++++++++++++++++----------------
>>  src/PVE/LXC/Setup/Devuan.pm    |  2 +-
>>  src/PVE/LXC/Setup/Plugin.pm    |  2 +-
>>  src/PVE/LXC/Setup/Unmanaged.pm |  2 +-
>>  6 files changed, 39 insertions(+), 21 deletions(-)
>>
>>
> 
> applied, with a small followup for a few small white-space/indentation errors,
> otherwise nice work - thanks!
> 

actually I did not yet pushed it out and rethought this, while I don't think that
the perm change is problematic w.r.t. backward compat in practice, it seems a bit
odd to argue for disk only, maybe we need to also check for CDROM, as cloudinit
*adds* a cdrom drive, so it may required that too.

But, to decide that I wanted to check our privilege docs, only to notice that 
we do not mention the cloudinit one there at all, not great...

https://pve.proxmox.com/pve-docs/chapter-pveum.html#_privileges

Please check the dev history to see if Cloudinit is deemed enough to add the CDROM
or if we should add that priv to the check too, then re-send this with updated
commit message, the whitespace fixes and a docs patch.

Re: [pve-devel] [PATCH v3 container] fix #4192: revamp check for systemd version

[Thomas Lamprecht]

Am 16/11/2022 um 13:13 schrieb Thomas Lamprecht:
> actually I did not yet pushed it out and rethought this, while I don't think that
> the perm change is problematic w.r.t. backward compat in practice, it seems a bit
> odd to argue for disk only, maybe we need to also check for CDROM, as cloudinit
> *adds* a cdrom drive, so it may required that too.
> 
> But, to decide that I wanted to check our privilege docs, only to notice that 
> we do not mention the cloudinit one there at all, not great...
> 
> https://pve.proxmox.com/pve-docs/chapter-pveum.html#_privileges
> 
> Please check the dev history to see if Cloudinit is deemed enough to add the CDROM
> or if we should add that priv to the check too, then re-send this with updated
> commit message, the whitespace fixes and a docs patch.
> 

argh, replied to the wrong patch, please ignore.