Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8

["Luca Boccassi" <luca.boccassi@microsoft.com>]

[-- Attachment #1: Type: text/plain, Size: 8428 bytes --]

On Fri, 2021-01-15 at 00:26 -0500, Paul Gortmaker wrote:
> Recent systemd started using ascii args to "hidepid=" mount options
> for proc fs - unconditionally -- even though kernels older than v5.8
> emit an error message on each attempt:
> 
> root@qemux86-64:~# cat /proc/version
> Linux version 5.4.87-yocto-standard (oe-user@oe-host) (gcc version 10.2.0 (GCC)) #1 SMP PREEMPT Fri Jan 8 01:47:13 UTC 2021
> root@qemux86-64:~# dmesg|grep proc:
> [   29.487995] proc: Bad value for 'hidepid'
> [   43.170571] proc: Bad value for 'hidepid'
> [   44.175615] proc: Bad value for 'hidepid'
> [   46.213300] proc: Bad value for 'hidepid'
> root@qemux86-64:~#
> 
> Simply ignoring them as the systemd maintainer unconditionally says
> is the resolution is clearly not acceptable, given the above.
> 
> Add a kernel version check to avoid calling mount with invalid args.
> Further details are within the enclosed systemd commit.
> 
> Cc: Luca Boccassi <luca.boccassi@microsoft.com>
> Cc: Richard Purdie <richard.purdie@linuxfoundation.org>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> 
> diff --git a/meta/recipes-core/systemd/systemd/0027-proc-dont-trigger-mount-error-with-invalid-options-o.patch b/meta/recipes-core/systemd/systemd/0027-proc-dont-trigger-mount-error-with-invalid-options-o.patch
> new file mode 100644
> index 000000000000..65e7eca32d05
> --- /dev/null
> +++ b/meta/recipes-core/systemd/systemd/0027-proc-dont-trigger-mount-error-with-invalid-options-o.patch
> @@ -0,0 +1,126 @@
> +From 297aba739cd689e4dc9f43bb1422ec88d481099a Mon Sep 17 00:00:00 2001
> +From: Paul Gortmaker <paul.gortmaker@windriver.com>
> +Date: Wed, 13 Jan 2021 21:09:33 +0000
> +Subject: [PATCH] proc: dont trigger mount error with invalid options on old
> + kernels
> +
> +As of commit 4e39995371738b04d98d27b0d34ea8fe09ec9fab ("core: introduce
> +ProtectProc= and ProcSubset= to expose hidepid= and subset= procfs
> +mount options") kernels older than v5.8 generate multple warnings at
> +boot, as seen in this Yocto build from today:
> +
> +     qemux86-64 login: root
> +     [   65.829009] proc: Bad value for 'hidepid'
> +     root@qemux86-64:~# dmesg|grep proc:
> +     [   16.990706] proc: Bad value for 'hidepid'
> +     [   28.060178] proc: Bad value for 'hidepid'
> +     [   28.874229] proc: Bad value for 'hidepid'
> +     [   32.685107] proc: Bad value for 'hidepid'
> +     [   65.829009] proc: Bad value for 'hidepid'
> +     root@qemux86-64:~#
> +
> +The systemd maintainer has dismissed this as something people should
> +simply ignore[1] and has no interest in trying to avoid it by
> +proactively checking the kernel version, so people can safely assume
> +that they will never see this version check commit upstream.
> +
> +However, as can be seen above, telling people to just ignore it is not
> +an option, as we'll end up answering the same question and dealing with
> +the same bug over and over again.
> +
> +The commit that triggers this is systemd v247-rc1~378^2~3 -- so any
> +systemd 247 and above plus kernel v5.7 or older will need this.
> +
> +[1] https://github.com/systemd/systemd/issues/16896
> +
> +Upstream-Status: Actively hostile
> +Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> +
> +diff --git a/src/core/namespace.c b/src/core/namespace.c
> +index cdf427a6ea93..f8fc33a89fc2 100644
> +--- a/src/core/namespace.c
> ++++ b/src/core/namespace.c
> +@@ -4,7 +4,9 @@
> + #include <linux/loop.h>
> + #include <sched.h>
> + #include <stdio.h>
> ++#include <stdlib.h>
> + #include <sys/mount.h>
> ++#include <sys/utsname.h>
> + #include <unistd.h>
> + #include <linux/fs.h>
> + 
> +@@ -859,14 +861,34 @@ static int mount_sysfs(const MountEntry *m) {
> + }
> + 
> + static int mount_procfs(const MountEntry *m, const NamespaceInfo *ns_info) {
> ++        _cleanup_free_ char *opts = NULL;
> +         const char *entry_path;
> +-        int r;
> ++        int r, major, minor;
> ++        struct utsname uts;
> ++        bool old = false;
> + 
> +         assert(m);
> +         assert(ns_info);
> + 
> +         entry_path = mount_entry_path(m);
> + 
> ++        /* If uname says that the system is older than v5.8, then the textual hidepid= stuff is not
> ++         * supported by the kernel, and thus the per-instance hidepid= neither, which means we
> ++         * really don't want to use it, since it would affect our host's /proc * mount. Hence let's
> ++         * gracefully fallback to a classic, unrestricted version. */
> ++
> ++        r = uname(&uts);
> ++        if (r < 0)
> ++               return errno;
> ++
> ++        major = atoi(uts.release);
> ++        minor = atoi(strchr(uts.release, '.') + 1);
> ++
> ++        if (major < 5 || (major == 5 && minor < 8)) {
> ++                log_debug("Pre v5.8 kernel detected [v%d.%d] - skipping hidepid=", major, minor);
> ++                old = true;
> ++        }
> ++
> +         /* Mount a new instance, so that we get the one that matches our user namespace, if we are running in
> +          * one. i.e we don't reuse existing mounts here under any condition, we want a new instance owned by
> +          * our user namespace and with our hidepid= settings applied. Hence, let's get rid of everything
> +@@ -875,9 +897,8 @@ static int mount_procfs(const MountEntry *m, const NamespaceInfo *ns_info) {
> +         (void) mkdir_p_label(entry_path, 0755);
> +         (void) umount_recursive(entry_path, 0);
> + 
> +-        if (ns_info->protect_proc != PROTECT_PROC_DEFAULT ||
> +-            ns_info->proc_subset != PROC_SUBSET_ALL) {
> +-                _cleanup_free_ char *opts = NULL;
> ++        if (!old && (ns_info->protect_proc != PROTECT_PROC_DEFAULT ||
> ++            ns_info->proc_subset != PROC_SUBSET_ALL)) {
> + 
> +                 /* Starting with kernel 5.8 procfs' hidepid= logic is truly per-instance (previously it
> +                  * pretended to be per-instance but actually was per-namespace), hence let's make use of it
> +@@ -891,21 +912,9 @@ static int mount_procfs(const MountEntry *m, const NamespaceInfo *ns_info) {
> +                                ns_info->proc_subset == PROC_SUBSET_PID ? ",subset=pid" : "");
> +                 if (!opts)
> +                         return -ENOMEM;
> +-
> +-                r = mount_nofollow_verbose(LOG_DEBUG, "proc", entry_path, "proc", MS_NOSUID|MS_NOEXEC|MS_NODEV, opts);
> +-                if (r < 0) {
> +-                        if (r != -EINVAL)
> +-                                return r;
> +-
> +-                        /* If this failed with EINVAL then this likely means the textual hidepid= stuff is
> +-                         * not supported by the kernel, and thus the per-instance hidepid= neither, which
> +-                         * means we really don't want to use it, since it would affect our host's /proc
> +-                         * mount. Hence let's gracefully fallback to a classic, unrestricted version. */
> +-                } else
> +-                        return 1;

Why is it necessary to remove all of the above? It's already skipped,
so it seems this patch could be at least half the size - give it's
permanent technical debt, that does make a difference.

> +         }
> + 
> +-        r = mount_nofollow_verbose(LOG_DEBUG, "proc", entry_path, "proc", MS_NOSUID|MS_NOEXEC|MS_NODEV, NULL);
> ++        r = mount_nofollow_verbose(LOG_DEBUG, "proc", entry_path, "proc", MS_NOSUID|MS_NOEXEC|MS_NODEV, opts);
> +         if (r < 0)
> +                 return r;
> + 
> +-- 
> +2.29.2
> +
> diff --git a/meta/recipes-core/systemd/systemd_247.2.bb b/meta/recipes-core/systemd/systemd_247.2.bb
> index 5eea78eff353..84d997196cb6 100644
> --- a/meta/recipes-core/systemd/systemd_247.2.bb
> +++ b/meta/recipes-core/systemd/systemd_247.2.bb
> @@ -23,6 +23,7 @@ SRC_URI += "file://touchscreen.rules \
>             file://0003-implment-systemd-sysv-install-for-OE.patch \
>             file://0001-systemd.pc.in-use-ROOTPREFIX-without-suffixed-slash.patch \
>             file://0001-logind-Restore-chvt-as-non-root-user-without-polkit.patch \
> +           file://0027-proc-dont-trigger-mount-error-with-invalid-options-o.patch \
>             "
>  
>  # patches needed by musl

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 499 bytes --]