Merge ~enr0n/ubuntu/+source/systemd:focal-lp1958284-and-lp1871538 into ~ubuntu-core-dev/ubuntu/+source/systemd:ubuntu-focal

Proposed by Nick Rosbrook
Status: Merged
Merged at revision: 420fcc5e44b1effd48dcd1c9f96672134f95b1eb
Proposed branch: ~enr0n/ubuntu/+source/systemd:focal-lp1958284-and-lp1871538
Merge into: ~ubuntu-core-dev/ubuntu/+source/systemd:ubuntu-focal
Diff against target: 185 lines (+153/-1)
4 files modified
debian/changelog (+12/-1)
debian/patches/lp1958284-core-move-reset_arguments-to-the-end-of-main-s-finish.patch (+48/-0)
debian/patches/pid1-set-SYSTEMD_NSS_DYNAMIC_BYPASS-1-env-var-for-dbus-da.patch (+91/-0)
debian/patches/series (+2/-0)
Reviewer Review Type Date Requested Status
Lukas Märdian Approve
Review via email: mp+417577@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Lukas Märdian (slyon) wrote :

Thank you very much. This LGTM!
Also the PPA build and PPA autopkgtests in https://autopkgtest.ubuntu.com/results/autopkgtest-focal-enr0n-systemd-245/?format=plain are all green!

Sponsoring & Merging!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index c133951..daf4f12 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -10,7 +10,18 @@ systemd (245.4-4ubuntu3.16) UNRELEASED; urgency=medium
6 File: debian/patches/lp1955997-add-a-allowlist-to-unblock-intel-hid-on-HP-mach.patch
7 https://git.launchpad.net/~ubuntu-core-dev/ubuntu/+source/systemd/commit/?id=88a859eaddb6c9a611fcbc44edab441aef4c4355
8
9- -- Lukas Märdian <slyon@ubuntu.com> Tue, 08 Feb 2022 17:39:41 +0100
10+ [ Nick Rosbrook ]
11+ * Prevent arguments from being overwritten with defaults at shutdown (LP: #1958284)
12+ File: debian/patches/lp1958284-core-move-reset_arguments-to-the-end-of-main-s-finish.patch
13+ https://git.launchpad.net/~ubuntu-core-dev/ubuntu/+source/systemd/commit/?id=e61052bd1f20bcc54e7417542c6d445cf5040f56
14+
15+ [ Lukas Märdian ]
16+ * Fix deadlock between pid1 and dbus-daemon (LP: #1871538)
17+ Author: Lukas Märdian
18+ File: debian/patches/pid1-set-SYSTEMD_NSS_DYNAMIC_BYPASS-1-env-var-for-dbus-da.patch
19+ https://git.launchpad.net/~ubuntu-core-dev/ubuntu/+source/systemd/commit/?id=e3aacfa26e3fc6df369e6f28e740389ae0020907
20+
21+ -- Nick Rosbrook <nick.rosbrook@canonical.com> Wed, 23 Mar 2022 09:29:33 -0400
22
23 systemd (245.4-4ubuntu3.15) focal-security; urgency=medium
24
25diff --git a/debian/patches/lp1958284-core-move-reset_arguments-to-the-end-of-main-s-finish.patch b/debian/patches/lp1958284-core-move-reset_arguments-to-the-end-of-main-s-finish.patch
26new file mode 100644
27index 0000000..1ae2fb6
28--- /dev/null
29+++ b/debian/patches/lp1958284-core-move-reset_arguments-to-the-end-of-main-s-finish.patch
30@@ -0,0 +1,48 @@
31+Description: core: move reset_arguments() to the end of main's finish
32+Origin: upstream, https://github.com/systemd/systemd/commit/7d9eea2bd3d4f83668c7a78754d201b22
33+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1958284
34+---
35+From: Anita Zhang <the.anitazha@gmail.com>
36+Date: Thu, 17 Sep 2020 01:49:17 -0700
37+Subject: core: move reset_arguments() to the end of main's finish
38+
39+Fixes #16991
40+
41+fb39af4ce42d7ef9af63009f271f404038703704 replaced `free_arguments()` with
42+`reset_arguments()`, which frees arg_* variables as before, but also resets all
43+of them to the default values. `reset_arguments()` was positioned
44+in such a way that it overrode some arg_* values still in use at shutdown.
45+
46+To avoid further unintentional resets, I moved `reset_arguments()`
47+right before the return, when nothing else will be using the arg_* variables.
48+---
49+ src/core/main.c | 3 ++-
50+ 1 file changed, 2 insertions(+), 1 deletion(-)
51+
52+diff --git a/src/core/main.c b/src/core/main.c
53+index e32ed02..615949b 100644
54+--- a/src/core/main.c
55++++ b/src/core/main.c
56+@@ -2717,7 +2717,6 @@ finish:
57+ m = manager_free(m);
58+ }
59+
60+- reset_arguments();
61+ mac_selinux_finish();
62+
63+ if (reexecute)
64+@@ -2744,6 +2743,7 @@ finish:
65+ * in become_shutdown() so normally we cannot free them yet. */
66+ watchdog_free_device();
67+ arg_watchdog_device = mfree(arg_watchdog_device);
68++ reset_arguments();
69+ return retval;
70+ }
71+ #endif
72+@@ -2769,5 +2769,6 @@ finish:
73+ freeze_or_exit_or_reboot();
74+ }
75+
76++ reset_arguments();
77+ return retval;
78+ }
79diff --git a/debian/patches/pid1-set-SYSTEMD_NSS_DYNAMIC_BYPASS-1-env-var-for-dbus-da.patch b/debian/patches/pid1-set-SYSTEMD_NSS_DYNAMIC_BYPASS-1-env-var-for-dbus-da.patch
80new file mode 100644
81index 0000000..df7456a
82--- /dev/null
83+++ b/debian/patches/pid1-set-SYSTEMD_NSS_DYNAMIC_BYPASS-1-env-var-for-dbus-da.patch
84@@ -0,0 +1,91 @@
85+From: Lennart Poettering <lennart@poettering.net>
86+Date: Thu, 17 Feb 2022 14:49:54 +0100
87+Subject: pid1: set SYSTEMD_NSS_DYNAMIC_BYPASS=1 env var for dbus-daemon
88+MIME-Version: 1.0
89+Content-Type: text/plain; charset="utf-8"
90+Content-Transfer-Encoding: 8bit
91+
92+There's currently a deadlock between PID 1 and dbus-daemon: in some
93+cases dbus-daemon will do NSS lookups (which are blocking) at the same
94+time PID 1 synchronously blocks on some call to dbus-daemon. Let's break
95+that by setting SYSTEMD_NSS_DYNAMIC_BYPASS=1 env var for dbus-daemon,
96+which will disable synchronously blocking varlink calls from nss-systemd
97+to PID 1.
98+
99+In the long run we should fix this differently: remove all synchronous
100+calls to dbus-daemon from PID 1. This is not trivial however: so far we
101+had the rule that synchronous calls from PID 1 to the dbus broker are OK
102+as long as they only go to interfaces implemented by the broke itself
103+rather than services reachable through it. Given that the relationship
104+between PID 1 and dbus is kinda special anyway, this was considered
105+acceptable for the sake of simplicity, since we quite often need
106+metadata about bus peers from the broker, and the asynchronous logic
107+would substantially complicate even the simplest method handlers.
108+
109+This mostly reworks the existing code that sets SYSTEMD_NSS_BYPASS_BUS=
110+(which is a similar hack to deal with deadlocks between nss-systemd and
111+dbus-daemon itself) to set SYSTEMD_NSS_DYNAMIC_BYPASS=1 instead. No code
112+was checking SYSTEMD_NSS_BYPASS_BUS= anymore anyway, and it used to
113+solve a similar problem, hence it's an obvious piece of code to rework
114+like this.
115+
116+Issue originally tracked down by Lukas Märdian. This patch is inspired
117+and closely based on his patch:
118+
119+ https://github.com/systemd/systemd/pull/22038
120+
121+Fixes: #15316
122+Co-authored-by: Lukas Märdian <slyon@ubuntu.com>
123+---
124+ src/core/execute.c | 10 +++++-----
125+ src/core/execute.h | 2 +-
126+ src/core/service.c | 2 +-
127+ 3 files changed, 7 insertions(+), 7 deletions(-)
128+
129+diff --git a/src/core/execute.c b/src/core/execute.c
130+index ca40874..b8d1ae4 100644
131+--- a/src/core/execute.c
132++++ b/src/core/execute.c
133+@@ -1829,11 +1829,11 @@ static int build_environment(
134+ our_env[n_env++] = x;
135+ }
136+
137+- /* If this is D-Bus, tell the nss-systemd module, since it relies on being able to use D-Bus look up dynamic
138+- * users via PID 1, possibly dead-locking the dbus daemon. This way it will not use D-Bus to resolve names, but
139+- * check the database directly. */
140+- if (p->flags & EXEC_NSS_BYPASS_BUS) {
141+- x = strdup("SYSTEMD_NSS_BYPASS_BUS=1");
142++ /* If this is D-Bus, tell the nss-systemd module, since it relies on being able to use blocking
143++ * Varlink calls back to us for look up dynamic users in PID 1. Break the deadlock between D-Bus and
144++ * PID 1 by disabling use of PID1' NSS interface for looking up dynamic users. */
145++ if (p->flags & EXEC_NSS_DYNAMIC_BYPASS) {
146++ x = strdup("SYSTEMD_NSS_DYNAMIC_BYPASS=1");
147+ if (!x)
148+ return -ENOMEM;
149+ our_env[n_env++] = x;
150+diff --git a/src/core/execute.h b/src/core/execute.h
151+index 4c7a5b8..2a261f3 100644
152+--- a/src/core/execute.h
153++++ b/src/core/execute.h
154+@@ -348,7 +348,7 @@ typedef enum ExecFlags {
155+ EXEC_APPLY_TTY_STDIN = 1 << 2,
156+ EXEC_PASS_LOG_UNIT = 1 << 3, /* Whether to pass the unit name to the service's journal stream connection */
157+ EXEC_CHOWN_DIRECTORIES = 1 << 4, /* chown() the runtime/state/cache/log directories to the user we run as, under all conditions */
158+- EXEC_NSS_BYPASS_BUS = 1 << 5, /* Set the SYSTEMD_NSS_BYPASS_BUS environment variable, to disable nss-systemd for dbus */
159++ EXEC_NSS_DYNAMIC_BYPASS = 1 << 5, /* Set the SYSTEMD_NSS_DYNAMIC_BYPASS environment variable, to disable nss-systemd blocking on PID 1, for use by dbus-daemon */
160+ EXEC_CGROUP_DELEGATE = 1 << 6,
161+ EXEC_IS_CONTROL = 1 << 7,
162+ EXEC_CONTROL_CGROUP = 1 << 8, /* Place the process not in the indicated cgroup but in a subcgroup '/.control', but only EXEC_CGROUP_DELEGATE and EXEC_IS_CONTROL is set, too */
163+diff --git a/src/core/service.c b/src/core/service.c
164+index 7b90822..debd9d6 100644
165+--- a/src/core/service.c
166++++ b/src/core/service.c
167+@@ -1569,7 +1569,7 @@ static int service_spawn(
168+ return -ENOMEM;
169+
170+ /* System D-Bus needs nss-systemd disabled, so that we don't deadlock */
171+- SET_FLAG(exec_params.flags, EXEC_NSS_BYPASS_BUS,
172++ SET_FLAG(exec_params.flags, EXEC_NSS_DYNAMIC_BYPASS,
173+ MANAGER_IS_SYSTEM(UNIT(s)->manager) && unit_has_name(UNIT(s), SPECIAL_DBUS_SERVICE));
174+
175+ strv_free_and_replace(exec_params.environment, final_env);
176diff --git a/debian/patches/series b/debian/patches/series
177index deef92d..e0a85c5 100644
178--- a/debian/patches/series
179+++ b/debian/patches/series
180@@ -171,3 +171,5 @@ CVE-2021-3997-2.patch
181 CVE-2021-3997-3.patch
182 lp1946388-sd-journal-don-t-check-namespaces-if-we-have-no-name.patch
183 lp1955997-add-a-allowlist-to-unblock-intel-hid-on-HP-mach.patch
184+lp1958284-core-move-reset_arguments-to-the-end-of-main-s-finish.patch
185+pid1-set-SYSTEMD_NSS_DYNAMIC_BYPASS-1-env-var-for-dbus-da.patch

Subscribers

People subscribed via source and target branches