All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
@ 2021-01-15  5:26 Paul Gortmaker
  2021-01-15  6:02 ` [OE-core] " Konrad Weihmann
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Paul Gortmaker @ 2021-01-15  5:26 UTC (permalink / raw)
  To: openembedded-core; +Cc: Paul Gortmaker, Luca Boccassi, Richard Purdie

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;
+         }
+ 
+-        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
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [OE-core] [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15  5:26 [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8 Paul Gortmaker
@ 2021-01-15  6:02 ` Konrad Weihmann
  2021-01-15  8:37 ` Richard Purdie
  2021-01-15 15:27 ` Luca Boccassi
  2 siblings, 0 replies; 15+ messages in thread
From: Konrad Weihmann @ 2021-01-15  6:02 UTC (permalink / raw)
  To: openembedded-core



On 15.01.21 06:26, 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

Just a tiny bit of nitpick, although I really like this new 
classification (as it pretty much sums up many things when it comes to 
systemd), it should be `Denied`
Nonetheless I think that this is a very useful patch

> +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;
> +         }
> +
> +-        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
> 
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15  5:26 [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8 Paul Gortmaker
  2021-01-15  6:02 ` [OE-core] " Konrad Weihmann
@ 2021-01-15  8:37 ` Richard Purdie
  2021-01-15  9:57   ` Luca Boccassi
  2021-01-15 15:27 ` Luca Boccassi
  2 siblings, 1 reply; 15+ messages in thread
From: Richard Purdie @ 2021-01-15  8:37 UTC (permalink / raw)
  To: Paul Gortmaker, openembedded-core; +Cc: Luca Boccassi

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

The status needs to be

Upstream-Status: Denied [Actively hostile https://github.com/systemd/systemd/issues/16896]

(so our tools have an idea of what status patches are in)

https://wiki.yoctoproject.org/wiki/Best_Known_Methods_(BKMs)_for_Package_Updating#Patch_Comments

Cheers,

Richard



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15  8:37 ` Richard Purdie
@ 2021-01-15  9:57   ` Luca Boccassi
  2021-01-15 10:14     ` Richard Purdie
  2021-01-15 14:47     ` Paul Gortmaker
  0 siblings, 2 replies; 15+ messages in thread
From: Luca Boccassi @ 2021-01-15  9:57 UTC (permalink / raw)
  To: richard.purdie, openembedded-core, paul.gortmaker

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

On Fri, 2021-01-15 at 08:37 +0000, Richard Purdie wrote:
> 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:~#

Wouldn't it be better to patch the kernel to downgrade that message to
debug level?

> > 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
> 
> The status needs to be
> 
> Upstream-Status: Denied [Actively hostile 
> https://github.com/systemd/systemd/issues/16896]
> 
> (so our tools have an idea of what status patches are in)

Paul, please, let's avoid loaded language - Denied is fine by itself
and conveys what it needs to. I understand it can be frustrating when
upstream maintainers do not agree with user assessments, but the linked
discussion was polite and professional and there's no need to call it
"hostile".

Also, speaking as an upstream maintainer, I'd be willing to review a
patch that adds a log_debug notice to advise to ignore the error if the
fallback path is taken. It's low cost and reasonable, so I don't think
it would be a problem to merge it.

> https://wiki.yoctoproject.org/wiki/Best_Known_Methods#Patch_Comments

Richard, FYI this appears to be an empty page.

-- 
Kind regards,
Luca Boccassi

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15  9:57   ` Luca Boccassi
@ 2021-01-15 10:14     ` Richard Purdie
  2021-01-15 10:20       ` Luca Boccassi
  2021-01-15 14:47     ` Paul Gortmaker
  1 sibling, 1 reply; 15+ messages in thread
From: Richard Purdie @ 2021-01-15 10:14 UTC (permalink / raw)
  To: Luca Boccassi, openembedded-core, paul.gortmaker

On Fri, 2021-01-15 at 09:57 +0000, Luca Boccassi wrote:
> > https://wiki.yoctoproject.org/wiki/Best_Known_Methods#Patch_Comments
> 
> Richard, FYI this appears to be an empty page.

That wasn't the link in my email, the link was:

https://wiki.yoctoproject.org/wiki/Best_Known_Methods_(BKMs)_for_Package_Updating#Patch_Comments

The brackets in there may be causing some kind of issue?

In case it doesn't work, the page in question is titled 
"Best Known Methods (BKMs) for Package Updating"

Cheers,

Richard



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15 10:14     ` Richard Purdie
@ 2021-01-15 10:20       ` Luca Boccassi
  0 siblings, 0 replies; 15+ messages in thread
From: Luca Boccassi @ 2021-01-15 10:20 UTC (permalink / raw)
  To: richard.purdie, openembedded-core, paul.gortmaker

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

On Fri, 2021-01-15 at 10:14 +0000, Richard Purdie wrote:
> On Fri, 2021-01-15 at 09:57 +0000, Luca Boccassi wrote:
> > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwiki.yoctoproject.org%2Fwiki%2FBest_Known_Methods%23Patch_Comments&amp;data=04%7C01%7CLuca.Boccassi%40microsoft.com%7Cd96a07e0ec0740e98b9a08d8b93e6409%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637463024985923638%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=aK4%2F8oasYb3NJ84rVUq6ptf0nRnwDKniKj4jIB283EY%3D&amp;reserved=0
> > 
> > Richard, FYI this appears to be an empty page.
> 
> That wasn't the link in my email, the link was:
> 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwiki.yoctoproject.org%2Fwiki%2FBest_Known_Methods_&amp;data=04%7C01%7CLuca.Boccassi%40microsoft.com%7Cd96a07e0ec0740e98b9a08d8b93e6409%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637463024985933630%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=TOAqow8KyiRT6i3LtpEeS46T0Dq8iixSNc3l6XpDzhk%3D&amp;reserved=0(BKMs)_for_Package_Updating#Patch_Comments
> 
> The brackets in there may be causing some kind of issue?
> 
> In case it doesn't work, the page in question is titled 
> "Best Known Methods (BKMs) for Package Updating"
> 
> Cheers,
> 
> Richard

Sorry my bad, it looks like the Exchange mail server mangling of the
link ("safelinks" feature) removes the brackets and makes it invalid. I
should really stop using the Microsoft address...

-- 
Kind regards,
Luca Boccassi

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15  9:57   ` Luca Boccassi
  2021-01-15 10:14     ` Richard Purdie
@ 2021-01-15 14:47     ` Paul Gortmaker
  2021-01-15 15:37       ` Luca Boccassi
  1 sibling, 1 reply; 15+ messages in thread
From: Paul Gortmaker @ 2021-01-15 14:47 UTC (permalink / raw)
  To: Luca Boccassi; +Cc: richard.purdie, openembedded-core

[Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8] On 15/01/2021 (Fri 09:57) Luca Boccassi wrote:

> On Fri, 2021-01-15 at 08:37 +0000, Richard Purdie wrote:
> > 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:~#
> 
> Wouldn't it be better to patch the kernel to downgrade that message to
> debug level?

The problem is that the breakage is forced upon older kernels, so you'd
have to effectively mainline that kind of "fix" to v5.12 (where there is
no problem) and then you could in theory request it for v5.4 linux-stable
according to "stable rules".

Besides, if something with root/mount priv. is consistently trying to
drive a square peg into a round hole - by trying to mount something and
failing -- it should be something that a sysadmin would want to
investigate.  Which is precisely why I am here now.  I think burying it
at debug level is counterproductive to that.


> > > +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
> > 
> > The status needs to be
> > 
> > Upstream-Status: Denied [Actively hostile 
> > https://github.com/systemd/systemd/issues/16896]
> > 
> > (so our tools have an idea of what status patches are in)
> 
> Paul, please, let's avoid loaded language - Denied is fine by itself
> and conveys what it needs to. I understand it can be frustrating when
> upstream maintainers do not agree with user assessments, but the linked
> discussion was polite and professional and there's no need to call it
> "hostile".

Normally I'd agree with you, but it isn't just this one thread/instance,
but instead *years* of continued "my way or the highway" behaviour
demonstrated by systemd on various lists like LKML, for all to see.  In
any case, in the interest of not breaking existing tooling, and getting
the fix to our users, I am fine with it being changed to simply be:

Upstream-Status: Denied [https://github.com/systemd/systemd/issues/16896]

Paul.
--

> 
> Also, speaking as an upstream maintainer, I'd be willing to review a
> patch that adds a log_debug notice to advise to ignore the error if the
> fallback path is taken. It's low cost and reasonable, so I don't think
> it would be a problem to merge it.
> 
> > https://wiki.yoctoproject.org/wiki/Best_Known_Methods#Patch_Comments
> 
> Richard, FYI this appears to be an empty page.
> 
> -- 
> Kind regards,
> Luca Boccassi



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15  5:26 [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8 Paul Gortmaker
  2021-01-15  6:02 ` [OE-core] " Konrad Weihmann
  2021-01-15  8:37 ` Richard Purdie
@ 2021-01-15 15:27 ` Luca Boccassi
  2 siblings, 0 replies; 15+ messages in thread
From: Luca Boccassi @ 2021-01-15 15:27 UTC (permalink / raw)
  To: openembedded-core, paul.gortmaker; +Cc: richard.purdie

[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15 14:47     ` Paul Gortmaker
@ 2021-01-15 15:37       ` Luca Boccassi
  2021-01-15 16:07         ` [OE-core] " Bruce Ashfield
  0 siblings, 1 reply; 15+ messages in thread
From: Luca Boccassi @ 2021-01-15 15:37 UTC (permalink / raw)
  To: paul.gortmaker; +Cc: richard.purdie, openembedded-core

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

On Fri, 2021-01-15 at 09:47 -0500, Paul Gortmaker wrote:
> [Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8] On 15/01/2021 (Fri 09:57) Luca Boccassi wrote:
> 
> > On Fri, 2021-01-15 at 08:37 +0000, Richard Purdie wrote:
> > > 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:~#
> > 
> > Wouldn't it be better to patch the kernel to downgrade that message to
> > debug level?
> 
> The problem is that the breakage is forced upon older kernels, so you'd
> have to effectively mainline that kind of "fix" to v5.12 (where there is
> no problem) and then you could in theory request it for v5.4 linux-stable
> according to "stable rules".

The patch could be downstream for older kernels, just like this one is.
With the difference that it would be temporary.

> Besides, if something with root/mount priv. is consistently trying to
> drive a square peg into a round hole - by trying to mount something and
> failing -- it should be something that a sysadmin would want to
> investigate.  Which is precisely why I am here now.  I think burying it
> at debug level is counterproductive to that.

Well the real issue is that there's no way to get a clean "we don't
support this option" out of certain kernel APIs, so one has to guess
and see what happens. Sometimes it's even worse, and flags like
NOSYMFOLLOW get silently ignored if they are not supported, which is
not great for security-related settings...

Anyway, these warnings only appear if ProtectProc and/or ProcSubset are
set to something other than the default value. Why not simply add a
top-level drop-in in /etc that forces them to be disabled in all
services if you have an older kernel? Something like this:

/etc/systemd/system/service.d/10-override-protect-proc.conf
[Service]
ProtectProc=default
ProcSubset=all

And then you can drop it when you upgrade your kernel. Isn't this a
better option than taking on permanent technical debt?

> > > > +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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fsystemd%2Fsystemd%2Fissues%2F16896&amp;data=04%7C01%7CLuca.Boccassi%40microsoft.com%7Ce972c325854743b67f0b08d8b9647bf1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637463188548752517%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=r%2FCh5m3ZLautkN1PLg99HqGdfl4VXqiNjJUsJhDPmCI%3D&amp;reserved=0
> > > > +
> > > > +Upstream-Status: Actively hostile
> > > 
> > > The status needs to be
> > > 
> > > Upstream-Status: Denied [Actively hostile 
> > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fsystemd%2Fsystemd%2Fissues%2F16896&amp;data=04%7C01%7CLuca.Boccassi%40microsoft.com%7Ce972c325854743b67f0b08d8b9647bf1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637463188548752517%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=r%2FCh5m3ZLautkN1PLg99HqGdfl4VXqiNjJUsJhDPmCI%3D&amp;reserved=0]
> > > 
> > > (so our tools have an idea of what status patches are in)
> > 
> > Paul, please, let's avoid loaded language - Denied is fine by itself
> > and conveys what it needs to. I understand it can be frustrating when
> > upstream maintainers do not agree with user assessments, but the linked
> > discussion was polite and professional and there's no need to call it
> > "hostile".
> 
> Normally I'd agree with you, but it isn't just this one thread/instance,
> but instead *years* of continued "my way or the highway" behaviour
> demonstrated by systemd on various lists like LKML, for all to see.  In
> any case, in the interest of not breaking existing tooling, and getting
> the fix to our users, I am fine with it being changed to simply be:
> 
> Upstream-Status: Denied [https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fsystemd%2Fsystemd%2Fissues%2F16896&amp;data=04%7C01%7CLuca.Boccassi%40microsoft.com%7Ce972c325854743b67f0b08d8b9647bf1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637463188548752517%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=r%2FCh5m3ZLautkN1PLg99HqGdfl4VXqiNjJUsJhDPmCI%3D&amp;reserved=0]

That's not entirely accurate, but let's leave it at that.

-- 
Kind regards,
Luca Boccassi

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OE-core] [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15 15:37       ` Luca Boccassi
@ 2021-01-15 16:07         ` Bruce Ashfield
  2021-01-15 16:31           ` Luca Boccassi
  0 siblings, 1 reply; 15+ messages in thread
From: Bruce Ashfield @ 2021-01-15 16:07 UTC (permalink / raw)
  To: luca.boccassi; +Cc: paul.gortmaker, richard.purdie, openembedded-core

On Fri, Jan 15, 2021 at 10:37 AM Luca Boccassi via
lists.openembedded.org
<luca.boccassi=microsoft.com@lists.openembedded.org> wrote:
>
> On Fri, 2021-01-15 at 09:47 -0500, Paul Gortmaker wrote:
> > [Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8] On 15/01/2021 (Fri 09:57) Luca Boccassi wrote:
> >
> > > On Fri, 2021-01-15 at 08:37 +0000, Richard Purdie wrote:
> > > > 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:~#
> > >
> > > Wouldn't it be better to patch the kernel to downgrade that message to
> > > debug level?
> >
> > The problem is that the breakage is forced upon older kernels, so you'd
> > have to effectively mainline that kind of "fix" to v5.12 (where there is
> > no problem) and then you could in theory request it for v5.4 linux-stable
> > according to "stable rules".
>
> The patch could be downstream for older kernels, just like this one is.
> With the difference that it would be temporary.

But the coverage is impossible, since there are so many
different kernel trees. So a kernel solution is a non-starter.

>
> > Besides, if something with root/mount priv. is consistently trying to
> > drive a square peg into a round hole - by trying to mount something and
> > failing -- it should be something that a sysadmin would want to
> > investigate.  Which is precisely why I am here now.  I think burying it
> > at debug level is counterproductive to that.
>
> Well the real issue is that there's no way to get a clean "we don't
> support this option" out of certain kernel APIs, so one has to guess
> and see what happens. Sometimes it's even worse, and flags like
> NOSYMFOLLOW get silently ignored if they are not supported, which is
> not great for security-related settings...
>
> Anyway, these warnings only appear if ProtectProc and/or ProcSubset are
> set to something other than the default value. Why not simply add a
> top-level drop-in in /etc that forces them to be disabled in all
> services if you have an older kernel? Something like this:
>
> /etc/systemd/system/service.d/10-override-protect-proc.conf
> [Service]
> ProtectProc=default
> ProcSubset=all
>
> And then you can drop it when you upgrade your kernel. Isn't this a
> better option than taking on permanent technical debt?

That's even more fiddly than Paul's patch. It relies on much more
of someone's distro configuration.

But it is true that you can throw it away eventually, assuming
someone actually knows that it is there.

I wouldn't call this patch much of a technical debt, but if it starts
gumming up systemd upgrades, it's easy to revisit.

>
> > > > > +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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fsystemd%2Fsystemd%2Fissues%2F16896&amp;data=04%7C01%7CLuca.Boccassi%40microsoft.com%7Ce972c325854743b67f0b08d8b9647bf1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637463188548752517%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=r%2FCh5m3ZLautkN1PLg99HqGdfl4VXqiNjJUsJhDPmCI%3D&amp;reserved=0
> > > > > +
> > > > > +Upstream-Status: Actively hostile
> > > >
> > > > The status needs to be
> > > >
> > > > Upstream-Status: Denied [Actively hostile
> > > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fsystemd%2Fsystemd%2Fissues%2F16896&amp;data=04%7C01%7CLuca.Boccassi%40microsoft.com%7Ce972c325854743b67f0b08d8b9647bf1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637463188548752517%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=r%2FCh5m3ZLautkN1PLg99HqGdfl4VXqiNjJUsJhDPmCI%3D&amp;reserved=0]
> > > >
> > > > (so our tools have an idea of what status patches are in)
> > >
> > > Paul, please, let's avoid loaded language - Denied is fine by itself
> > > and conveys what it needs to. I understand it can be frustrating when
> > > upstream maintainers do not agree with user assessments, but the linked
> > > discussion was polite and professional and there's no need to call it
> > > "hostile".
> >
> > Normally I'd agree with you, but it isn't just this one thread/instance,
> > but instead *years* of continued "my way or the highway" behaviour
> > demonstrated by systemd on various lists like LKML, for all to see.  In
> > any case, in the interest of not breaking existing tooling, and getting
> > the fix to our users, I am fine with it being changed to simply be:
> >
> > Upstream-Status: Denied [https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fsystemd%2Fsystemd%2Fissues%2F16896&amp;data=04%7C01%7CLuca.Boccassi%40microsoft.com%7Ce972c325854743b67f0b08d8b9647bf1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637463188548752517%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=r%2FCh5m3ZLautkN1PLg99HqGdfl4VXqiNjJUsJhDPmCI%3D&amp;reserved=0]
>
> That's not entirely accurate, but let's leave it at that.

I'm with Paul :D

Bruce

>
> --
> Kind regards,
> Luca Boccassi
>
> 
>


-- 
- Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end
- "Use the force Harry" - Gandalf, Star Trek II

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OE-core] [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15 16:07         ` [OE-core] " Bruce Ashfield
@ 2021-01-15 16:31           ` Luca Boccassi
  2021-01-15 16:59             ` Bruce Ashfield
  0 siblings, 1 reply; 15+ messages in thread
From: Luca Boccassi @ 2021-01-15 16:31 UTC (permalink / raw)
  To: bruce.ashfield; +Cc: richard.purdie, openembedded-core, paul.gortmaker

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

On Fri, 2021-01-15 at 11:07 -0500, Bruce Ashfield wrote:
> On Fri, Jan 15, 2021 at 10:37 AM Luca Boccassi via
> lists.openembedded.org
> <luca.boccassi=microsoft.com@lists.openembedded.org> wrote:
> > On Fri, 2021-01-15 at 09:47 -0500, Paul Gortmaker wrote:
> > > [Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8] On 15/01/2021 (Fri 09:57) Luca Boccassi wrote:
> > > 
> > > > On Fri, 2021-01-15 at 08:37 +0000, Richard Purdie wrote:
> > > > > 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:~#
> > > > 
> > > > Wouldn't it be better to patch the kernel to downgrade that message to
> > > > debug level?
> > > 
> > > The problem is that the breakage is forced upon older kernels, so you'd
> > > have to effectively mainline that kind of "fix" to v5.12 (where there is
> > > no problem) and then you could in theory request it for v5.4 linux-stable
> > > according to "stable rules".
> > 
> > The patch could be downstream for older kernels, just like this one is.
> > With the difference that it would be temporary.
> 
> But the coverage is impossible, since there are so many
> different kernel trees. So a kernel solution is a non-starter.

IMHO a long term solution can only go through the kernel. There will be
more instances like this in the future, given the fundamental issue is
the lack of a clear EOPNOTSUPP-like interface. I realise the mount
interface is ancient at this point, and difficult to change. We are
keeping an eye on the evolution of the new interface, but it's not
there yet (eg: no read-only bind mounts).

> > > Besides, if something with root/mount priv. is consistently trying to
> > > drive a square peg into a round hole - by trying to mount something and
> > > failing -- it should be something that a sysadmin would want to
> > > investigate.  Which is precisely why I am here now.  I think burying it
> > > at debug level is counterproductive to that.
> > 
> > Well the real issue is that there's no way to get a clean "we don't
> > support this option" out of certain kernel APIs, so one has to guess
> > and see what happens. Sometimes it's even worse, and flags like
> > NOSYMFOLLOW get silently ignored if they are not supported, which is
> > not great for security-related settings...
> > 
> > Anyway, these warnings only appear if ProtectProc and/or ProcSubset are
> > set to something other than the default value. Why not simply add a
> > top-level drop-in in /etc that forces them to be disabled in all
> > services if you have an older kernel? Something like this:
> > 
> > /etc/systemd/system/service.d/10-override-protect-proc.conf
> > [Service]
> > ProtectProc=default
> > ProcSubset=all
> > 
> > And then you can drop it when you upgrade your kernel. Isn't this a
> > better option than taking on permanent technical debt?
> 
> That's even more fiddly than Paul's patch. It relies on much more
> of someone's distro configuration.

But what is the combination of a kernel version + systemd version if
not someone's distro configuration?
I mean, it could even be added dynamically by the poky recipe at build
time, given the kernel version is known at that point.

Surely a three lines piece of configuration is better to handle than a
downstream patch?

> But it is true that you can throw it away eventually, assuming
> someone actually knows that it is there.
> 
> I wouldn't call this patch much of a technical debt, but if it starts
> gumming up systemd upgrades, it's easy to revisit.

It's in a security-critical area that we are constantly improving and
expanding.

-- 
Kind regards,
Luca Boccassi

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OE-core] [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15 16:31           ` Luca Boccassi
@ 2021-01-15 16:59             ` Bruce Ashfield
  2021-01-15 17:35               ` Luca Boccassi
  0 siblings, 1 reply; 15+ messages in thread
From: Bruce Ashfield @ 2021-01-15 16:59 UTC (permalink / raw)
  To: Luca Boccassi; +Cc: richard.purdie, openembedded-core, paul.gortmaker

On Fri, Jan 15, 2021 at 11:31 AM Luca Boccassi
<Luca.Boccassi@microsoft.com> wrote:
>
> On Fri, 2021-01-15 at 11:07 -0500, Bruce Ashfield wrote:
> > On Fri, Jan 15, 2021 at 10:37 AM Luca Boccassi via
> > lists.openembedded.org
> > <luca.boccassi=microsoft.com@lists.openembedded.org> wrote:
> > > On Fri, 2021-01-15 at 09:47 -0500, Paul Gortmaker wrote:
> > > > [Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8] On 15/01/2021 (Fri 09:57) Luca Boccassi wrote:
> > > >
> > > > > On Fri, 2021-01-15 at 08:37 +0000, Richard Purdie wrote:
> > > > > > 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:~#
> > > > >
> > > > > Wouldn't it be better to patch the kernel to downgrade that message to
> > > > > debug level?
> > > >
> > > > The problem is that the breakage is forced upon older kernels, so you'd
> > > > have to effectively mainline that kind of "fix" to v5.12 (where there is
> > > > no problem) and then you could in theory request it for v5.4 linux-stable
> > > > according to "stable rules".
> > >
> > > The patch could be downstream for older kernels, just like this one is.
> > > With the difference that it would be temporary.
> >
> > But the coverage is impossible, since there are so many
> > different kernel trees. So a kernel solution is a non-starter.
>
> IMHO a long term solution can only go through the kernel. There will be
> more instances like this in the future, given the fundamental issue is
> the lack of a clear EOPNOTSUPP-like interface. I realise the mount
> interface is ancient at this point, and difficult to change. We are
> keeping an eye on the evolution of the new interface, but it's not
> there yet (eg: no read-only bind mounts).

I don't disagree .. but I'm only commenting on this one older kernel
issue, so there's nothing to see in the new kernels where I spend
99% of my time. The interfaces in the kernel will absolutely evolve
in the newer kernels, and that's a good thing.

>
> > > > Besides, if something with root/mount priv. is consistently trying to
> > > > drive a square peg into a round hole - by trying to mount something and
> > > > failing -- it should be something that a sysadmin would want to
> > > > investigate.  Which is precisely why I am here now.  I think burying it
> > > > at debug level is counterproductive to that.
> > >
> > > Well the real issue is that there's no way to get a clean "we don't
> > > support this option" out of certain kernel APIs, so one has to guess
> > > and see what happens. Sometimes it's even worse, and flags like
> > > NOSYMFOLLOW get silently ignored if they are not supported, which is
> > > not great for security-related settings...
> > >
> > > Anyway, these warnings only appear if ProtectProc and/or ProcSubset are
> > > set to something other than the default value. Why not simply add a
> > > top-level drop-in in /etc that forces them to be disabled in all
> > > services if you have an older kernel? Something like this:
> > >
> > > /etc/systemd/system/service.d/10-override-protect-proc.conf
> > > [Service]
> > > ProtectProc=default
> > > ProcSubset=all
> > >
> > > And then you can drop it when you upgrade your kernel. Isn't this a
> > > better option than taking on permanent technical debt?
> >
> > That's even more fiddly than Paul's patch. It relies on much more
> > of someone's distro configuration.
>
> But what is the combination of a kernel version + systemd version if
> not someone's distro configuration?
> I mean, it could even be added dynamically by the poky recipe at build
> time, given the kernel version is known at that point.

The kernel isn't currently a dependency of systemd and you'd be going
outside of the recipe namespace (but maybe I'm missing something, I
spent about a minute refreshing my memory on the recipes) .
Something at the image/rootfs level, gets brittle and doesn't scale
(but again, maybe I'm overlooking some technique).

>
> Surely a three lines piece of configuration is better to handle than a
> downstream patch?
>

Not in my opinion. I have no interest in extra moving parts and having
something injected into my userspace/init config.

> > But it is true that you can throw it away eventually, assuming
> > someone actually knows that it is there.
> >
> > I wouldn't call this patch much of a technical debt, but if it starts
> > gumming up systemd upgrades, it's easy to revisit.
>
> It's in a security-critical area that we are constantly improving and
> expanding.

That code block isn't particularly complex and it is self contained,
so really, there's not much to see there. But I'm not in the business
of predicting anything, I'm in the wait and see camp (AUH will be the
first to pick it up if it causes patch problems, etc).

Richard can either take the change or not, I'm just supporting how
it has currently been submitted as chances are that no further
revisions are coming from Paul, so it can live or die as-is.

Cheers,

Bruce

>
> --
> Kind regards,
> Luca Boccassi



-- 
- Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end
- "Use the force Harry" - Gandalf, Star Trek II

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OE-core] [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15 16:59             ` Bruce Ashfield
@ 2021-01-15 17:35               ` Luca Boccassi
  2021-01-18 11:07                 ` Leon Woestenberg
  2021-01-18 12:36                 ` Richard Purdie
  0 siblings, 2 replies; 15+ messages in thread
From: Luca Boccassi @ 2021-01-15 17:35 UTC (permalink / raw)
  To: bruce.ashfield; +Cc: richard.purdie, openembedded-core, paul.gortmaker

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

On Fri, 2021-01-15 at 11:59 -0500, Bruce Ashfield wrote:
> On Fri, Jan 15, 2021 at 11:31 AM Luca Boccassi
> <Luca.Boccassi@microsoft.com> wrote:
> > On Fri, 2021-01-15 at 11:07 -0500, Bruce Ashfield wrote:
> > > On Fri, Jan 15, 2021 at 10:37 AM Luca Boccassi via
> > > lists.openembedded.org
> > > <luca.boccassi=microsoft.com@lists.openembedded.org> wrote:
> > > > On Fri, 2021-01-15 at 09:47 -0500, Paul Gortmaker wrote:
> > > > > [Re: [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8] On 15/01/2021 (Fri 09:57) Luca Boccassi wrote:
> > > > > 
> > > > > > On Fri, 2021-01-15 at 08:37 +0000, Richard Purdie wrote:
> > > > > > > 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:~#
> > > > > > 
> > > > > > Wouldn't it be better to patch the kernel to downgrade that message to
> > > > > > debug level?
> > > > > 
> > > > > The problem is that the breakage is forced upon older kernels, so you'd
> > > > > have to effectively mainline that kind of "fix" to v5.12 (where there is
> > > > > no problem) and then you could in theory request it for v5.4 linux-stable
> > > > > according to "stable rules".
> > > > 
> > > > The patch could be downstream for older kernels, just like this one is.
> > > > With the difference that it would be temporary.
> > > 
> > > But the coverage is impossible, since there are so many
> > > different kernel trees. So a kernel solution is a non-starter.
> > 
> > IMHO a long term solution can only go through the kernel. There will be
> > more instances like this in the future, given the fundamental issue is
> > the lack of a clear EOPNOTSUPP-like interface. I realise the mount
> > interface is ancient at this point, and difficult to change. We are
> > keeping an eye on the evolution of the new interface, but it's not
> > there yet (eg: no read-only bind mounts).
> 
> I don't disagree .. but I'm only commenting on this one older kernel
> issue, so there's nothing to see in the new kernels where I spend
> 99% of my time. The interfaces in the kernel will absolutely evolve
> in the newer kernels, and that's a good thing.
> 
> > > > > Besides, if something with root/mount priv. is consistently trying to
> > > > > drive a square peg into a round hole - by trying to mount something and
> > > > > failing -- it should be something that a sysadmin would want to
> > > > > investigate.  Which is precisely why I am here now.  I think burying it
> > > > > at debug level is counterproductive to that.
> > > > 
> > > > Well the real issue is that there's no way to get a clean "we don't
> > > > support this option" out of certain kernel APIs, so one has to guess
> > > > and see what happens. Sometimes it's even worse, and flags like
> > > > NOSYMFOLLOW get silently ignored if they are not supported, which is
> > > > not great for security-related settings...
> > > > 
> > > > Anyway, these warnings only appear if ProtectProc and/or ProcSubset are
> > > > set to something other than the default value. Why not simply add a
> > > > top-level drop-in in /etc that forces them to be disabled in all
> > > > services if you have an older kernel? Something like this:
> > > > 
> > > > /etc/systemd/system/service.d/10-override-protect-proc.conf
> > > > [Service]
> > > > ProtectProc=default
> > > > ProcSubset=all
> > > > 
> > > > And then you can drop it when you upgrade your kernel. Isn't this a
> > > > better option than taking on permanent technical debt?
> > > 
> > > That's even more fiddly than Paul's patch. It relies on much more
> > > of someone's distro configuration.
> > 
> > But what is the combination of a kernel version + systemd version if
> > not someone's distro configuration?
> > I mean, it could even be added dynamically by the poky recipe at build
> > time, given the kernel version is known at that point.
> 
> The kernel isn't currently a dependency of systemd and you'd be going
> outside of the recipe namespace (but maybe I'm missing something, I
> spent about a minute refreshing my memory on the recipes) .
> Something at the image/rootfs level, gets brittle and doesn't scale
> (but again, maybe I'm overlooking some technique).

There's the get_kernelversion* functions available to recipes,
reasonably widely used already - so something that checks it and
conditionally installs the drop-in should work fine, in theory?

> > Surely a three lines piece of configuration is better to handle than a
> > downstream patch?
> > 
> 
> Not in my opinion. I have no interest in extra moving parts and having
> something injected into my userspace/init config.

There's half a million things "injected in your userspace/init
config"... I mean, that's what a distro does? This is exactly why this
config knobs were added, so that distro builders can customize the
behaviour to their liking.

> > > But it is true that you can throw it away eventually, assuming
> > > someone actually knows that it is there.
> > > 
> > > I wouldn't call this patch much of a technical debt, but if it starts
> > > gumming up systemd upgrades, it's easy to revisit.
> > 
> > It's in a security-critical area that we are constantly improving and
> > expanding.
> 
> That code block isn't particularly complex and it is self contained,
> so really, there's not much to see there. But I'm not in the business
> of predicting anything, I'm in the wait and see camp (AUH will be the
> first to pick it up if it causes patch problems, etc).
> 
> Richard can either take the change or not, I'm just supporting how
> it has currently been submitted as chances are that no further
> revisions are coming from Paul, so it can live or die as-is.

Even if a patch was preferred to a config, the current patch changes
too much and should really be reduced in scope, as commented in the
other email.

-- 
Kind regards,
Luca Boccassi

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OE-core] [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15 17:35               ` Luca Boccassi
@ 2021-01-18 11:07                 ` Leon Woestenberg
  2021-01-18 12:36                 ` Richard Purdie
  1 sibling, 0 replies; 15+ messages in thread
From: Leon Woestenberg @ 2021-01-18 11:07 UTC (permalink / raw)
  To: luca.boccassi
  Cc: bruce.ashfield, openembedded-core, paul.gortmaker, richard.purdie

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

On Fri, 15 Jan 2021 at 18:35, Luca Boccassi via lists.openembedded.org
<luca.boccassi=microsoft.com@lists.openembedded.org> wrote:

>
>
> There's the get_kernelversion* functions available to recipes,
> reasonably widely used already - so something that checks it and
> conditionally installs the drop-in should work fine, in theory?
>

That would make the workaround a initial build-time dependency. As I
understand the proposed solution was a run-time check.

What if the kernel is upgraded in the field?

>
>
> --
-- 
Leon Woestenberg
leon@sidebranch.com
T: +31 40 711 42 76
M: +31 6 472 30 372

Sidebranch Embedded Systems
Eindhoven, The Netherlands
http://www.sidebranch.com

[-- Attachment #2: Type: text/html, Size: 2446 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [OE-core] [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8
  2021-01-15 17:35               ` Luca Boccassi
  2021-01-18 11:07                 ` Leon Woestenberg
@ 2021-01-18 12:36                 ` Richard Purdie
  1 sibling, 0 replies; 15+ messages in thread
From: Richard Purdie @ 2021-01-18 12:36 UTC (permalink / raw)
  To: Luca Boccassi, bruce.ashfield; +Cc: openembedded-core, paul.gortmaker

On Fri, 2021-01-15 at 17:35 +0000, Luca Boccassi wrote:
> On Fri, 2021-01-15 at 11:59 -0500, Bruce Ashfield wrote:
> > That code block isn't particularly complex and it is self contained,
> > so really, there's not much to see there. But I'm not in the business
> > of predicting anything, I'm in the wait and see camp (AUH will be the
> > first to pick it up if it causes patch problems, etc).
> > 
> > Richard can either take the change or not, I'm just supporting how
> > it has currently been submitted as chances are that no further
> > revisions are coming from Paul, so it can live or die as-is.
> 
> Even if a patch was preferred to a config, the current patch changes
> too much and should really be reduced in scope, as commented in the
> other email.

Doing this with a patch is probably 'safer' or less ugly than the other
options.

I had meant to test this whilst as the same time,  suggest we reduce
the context in the patch as mentioned, then merge. Unfortunately I
confused the systemd patches and this has sneaked in without that
tweak.

If we could reduce the noise in the diff, that would be useful though. 

Cheers,

Richard







^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-01-18 12:36 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-15  5:26 [PATCH] systemd: dont spew hidepid mount errors for kernels < v5.8 Paul Gortmaker
2021-01-15  6:02 ` [OE-core] " Konrad Weihmann
2021-01-15  8:37 ` Richard Purdie
2021-01-15  9:57   ` Luca Boccassi
2021-01-15 10:14     ` Richard Purdie
2021-01-15 10:20       ` Luca Boccassi
2021-01-15 14:47     ` Paul Gortmaker
2021-01-15 15:37       ` Luca Boccassi
2021-01-15 16:07         ` [OE-core] " Bruce Ashfield
2021-01-15 16:31           ` Luca Boccassi
2021-01-15 16:59             ` Bruce Ashfield
2021-01-15 17:35               ` Luca Boccassi
2021-01-18 11:07                 ` Leon Woestenberg
2021-01-18 12:36                 ` Richard Purdie
2021-01-15 15:27 ` Luca Boccassi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.