Merge pull request #9504 from poettering/nss-deadlock

some nss deadlock love

9 files changed, 134 insertions, 9 deletions

diff --git a/NEWS b/NEWS
index 0d5b95a42a..242f55af29 100644
--- a/NEWS
+++ b/NEWS
@@ -109,7 +109,28 @@ CHANGES WITH 239:
 
         * systemd-resolved.service and systemd-networkd.service now set
           DynamicUser=yes. The users systemd-resolve and systemd-network are
-          not created by systemd-sysusers.
+          not created by systemd-sysusers anymore.
+
+          NOTE: This has a chance of breaking nss-ldap and similar NSS modules
+          that embedd a network facing module into any process using getpwuid()
+          or related call: the dynamic allocation of the user ID for
+          systemd-resolved.service means the service manager has to check NSS
+          if the user name is already taken when forking off the service. Since
+          the user in the common case won't be defined in /etc/passwd the
+          lookup is likely to trigger nss-ldap which in turn might use NSS to
+          ask systemd-resolved for hostname lookups. This will hence result in
+          a deadlock: a user name lookup in order to start
+          systemd-resolved.service will result in a host name lookup for which
+          systemd-resolved.service needs to be started already. There are
+          multiple ways to work around this problem: pre-allocate the
+          "systemd-resolve" user on such systems, so that nss-ldap won't be
+          triggered; or use a different NSS package that doesn't do networking
+          in-process but provides a local asynchronous name cache; or configure
+          the NSS package to avoid lookups for UIDs in the range `pkg-config
+          systemd --variable=dynamicuidmin` … `pkg-config systemd
+          --variable=dynamicuidmax`, so that it does not consider itself
+          authoritative for the same UID range systemd allocates dynamic users
+          from.
 
         * The systemd-resolve tool has been renamed to resolvectl (it also
           remains available under the old name, for compatibility), and its
diff --git a/doc/ENVIRONMENT.md b/doc/ENVIRONMENT.md
index 641a03d5d7..c69bf9b664 100644
--- a/doc/ENVIRONMENT.md
+++ b/doc/ENVIRONMENT.md
@@ -101,3 +101,21 @@ systemd-timedated:
   NTP client services. If set, `timedatectl set-ntp on` enables and starts the
   first existing unit listed in the environment variable, and
   `timedatectl set-ntp off` disables and stops all listed units.
+
+systemd itself:
+
+* `$SYSTEMD_ACTIVATION_UNIT` — set for all NSS and PAM module invocations that
+  are done by the service manager on behalf of a specific unit, in child
+  processes that are later (after execve()) going to become unit
+  processes. Contains the full unit name (e.g. "foobar.service"). NSS and PAM
+  modules can use this information to determine in which context and on whose
+  behalf they are being called, which may be useful to avoid deadlocks, for
+  example to bypass IPC calls to the very service that is about to be
+  started. Note that NSS and PAM modules should be careful to only rely on this
+  data when invoked privileged, or possibly only when getppid() returns 1, as
+  setting environment variables is of course possible in any even unprivileged
+  contexts.
+
+* `$SYSTEMD_ACTIVATION_SCOPE` — closely related to `$SYSTEMD_ACTIVATION_UNIT`,
+  it is either set to `system` or `user` depending on whether the NSS/PAM
+  module is called by systemd in `--system` or `--user` mode.
diff --git a/src/basic/env-util.h b/src/basic/env-util.h
index ef9398e618..174433ea91 100644
--- a/src/basic/env-util.h
+++ b/src/basic/env-util.h
@@ -6,6 +6,7 @@
 #include <stdio.h>
 
 #include "macro.h"
+#include "string.h"
 
 bool env_name_is_valid(const char *e);
 bool env_value_is_valid(const char *e);
diff --git a/src/basic/path-util.h b/src/basic/path-util.h
index 8277c6b916..49604eab80 100644
--- a/src/basic/path-util.h
+++ b/src/basic/path-util.h
@@ -58,10 +58,10 @@ static inline bool path_equal_ptr(const char *a, const char *b) {
 /* Note: the search terminates on the first NULL item. */
 #define PATH_IN_SET(p, ...)                                     \
         ({                                                      \
-                char **s;                                       \
+                char **_s;                                      \
                 bool _found = false;                            \
-                STRV_FOREACH(s, STRV_MAKE(__VA_ARGS__))         \
-                        if (path_equal(p, *s)) {                \
+                STRV_FOREACH(_s, STRV_MAKE(__VA_ARGS__))        \
+                        if (path_equal(p, *_s)) {               \
                                _found = true;                   \
                                break;                           \
                         }                                       \
diff --git a/src/core/execute.c b/src/core/execute.c
index 0f36f6c825..ed3e1459df 100644
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -2838,10 +2838,22 @@ static int exec_child(
                 }
         }
 
+        /* We are about to invoke NSS and PAM modules. Let's tell them what we are doing here, maybe they care. This is
+         * used by nss-resolve to disable itself when we are about to start systemd-resolved, to avoid deadlocks. Note
+         * that these env vars do not survive the execve(), which means they really only apply to the PAM and NSS
+         * invocations themselves. Also note that while we'll only invoke NSS modules involved in user management they
+         * might internally call into other NSS modules that are involved in hostname resolution, we never know. */
+        if (setenv("SYSTEMD_ACTIVATION_UNIT", unit->id, true) != 0 ||
+            setenv("SYSTEMD_ACTIVATION_SCOPE", MANAGER_IS_SYSTEM(unit->manager) ? "system" : "user", true) != 0) {
+                *exit_status = EXIT_MEMORY;
+                return log_unit_error_errno(unit, errno, "Failed to update environment: %m");
+        }
+
         if (context->dynamic_user && dcreds) {
                 _cleanup_strv_free_ char **suggested_paths = NULL;
 
-                /* Make sure we bypass our own NSS module for any NSS checks */
+                /* On top of that, make sure we bypass our own NSS module nss-systemd comprehensively for any NSS
+                 * checks, if DynamicUser=1 is used, as we shouldn't create a feedback loop with ourselves here.*/
                 if (putenv((char*) "SYSTEMD_NSS_DYNAMIC_BYPASS=1") != 0) {
                         *exit_status = EXIT_USER;
                         return log_unit_error_errno(unit, errno, "Failed to update environment: %m");
diff --git a/src/nss-mymachines/nss-mymachines.c b/src/nss-mymachines/nss-mymachines.c
index 9b81cd9ad1..3d1fc28353 100644
--- a/src/nss-mymachines/nss-mymachines.c
+++ b/src/nss-mymachines/nss-mymachines.c
@@ -63,6 +63,20 @@ static int count_addresses(sd_bus_message *m, int af, unsigned *ret) {
         return 0;
 }
 
+static bool avoid_deadlock(void) {
+
+        /* Check whether this lookup might have a chance of deadlocking because we are called from the service manager
+         * code activating systemd-machined.service. After all, we shouldn't synchronously do lookups to
+         * systemd-machined if we are required to finish before it can be started. This of course won't detect all
+         * possible dead locks of this kind, but it should work for the most obvious cases. */
+
+        if (geteuid() != 0) /* Ignore the env vars unless we are privileged. */
+                return false;
+
+        return streq_ptr(getenv("SYSTEMD_ACTIVATION_UNIT"), "systemd-machined.service") &&
+               streq_ptr(getenv("SYSTEMD_ACTIVATION_SCOPE"), "system");
+}
+
 enum nss_status _nss_mymachines_gethostbyname4_r(
                 const char *name,
                 struct gaih_addrtuple **pat,
@@ -103,6 +117,11 @@ enum nss_status _nss_mymachines_gethostbyname4_r(
                 goto fail;
         }
 
+        if (avoid_deadlock()) {
+                r = -EDEADLK;
+                goto fail;
+        }
+
         r = sd_bus_open_system(&bus);
         if (r < 0)
                 goto fail;
@@ -255,6 +274,11 @@ enum nss_status _nss_mymachines_gethostbyname3_r(
                 goto fail;
         }
 
+        if (avoid_deadlock()) {
+                r = -EDEADLK;
+                goto fail;
+        }
+
         r = sd_bus_open_system(&bus);
         if (r < 0)
                 goto fail;
@@ -425,6 +449,11 @@ enum nss_status _nss_mymachines_getpwnam_r(
                  * running on the host. */
                 return NSS_STATUS_NOTFOUND;
 
+        if (avoid_deadlock()) {
+                r = -EDEADLK;
+                goto fail;
+        }
+
         r = sd_bus_open_system(&bus);
         if (r < 0)
                 goto fail;
@@ -502,6 +531,11 @@ enum nss_status _nss_mymachines_getpwuid_r(
         if (getenv_bool_secure("SYSTEMD_NSS_BYPASS_BUS") > 0)
                 return NSS_STATUS_NOTFOUND;
 
+        if (avoid_deadlock()) {
+                r = -EDEADLK;
+                goto fail;
+        }
+
         r = sd_bus_open_system(&bus);
         if (r < 0)
                 goto fail;
@@ -594,6 +628,11 @@ enum nss_status _nss_mymachines_getgrnam_r(
         if (getenv_bool_secure("SYSTEMD_NSS_BYPASS_BUS") > 0)
                 return NSS_STATUS_NOTFOUND;
 
+        if (avoid_deadlock()) {
+                r = -EDEADLK;
+                goto fail;
+        }
+
         r = sd_bus_open_system(&bus);
         if (r < 0)
                 goto fail;
@@ -668,6 +707,11 @@ enum nss_status _nss_mymachines_getgrgid_r(
         if (getenv_bool_secure("SYSTEMD_NSS_BYPASS_BUS") > 0)
                 return NSS_STATUS_NOTFOUND;
 
+        if (avoid_deadlock()) {
+                r = -EDEADLK;
+                goto fail;
+        }
+
         r = sd_bus_open_system(&bus);
         if (r < 0)
                 goto fail;
diff --git a/src/nss-resolve/nss-resolve.c b/src/nss-resolve/nss-resolve.c
index b2bb698ded..a28b5d8ba8 100644
--- a/src/nss-resolve/nss-resolve.c
+++ b/src/nss-resolve/nss-resolve.c
@@ -91,6 +91,20 @@ static uint32_t ifindex_to_scopeid(int family, const void *a, int ifindex) {
         return IN6_IS_ADDR_LINKLOCAL(&in6) ? ifindex : 0;
 }
 
+static bool avoid_deadlock(void) {
+
+        /* Check whether this lookup might have a chance of deadlocking because we are called from the service manager
+         * code activating systemd-resolved.service. After all, we shouldn't synchronously do lookups to
+         * systemd-resolved if we are required to finish before it can be started. This of course won't detect all
+         * possible dead locks of this kind, but it should work for the most obvious cases. */
+
+        if (geteuid() != 0) /* Ignore the env vars unless we are privileged. */
+                return false;
+
+        return streq_ptr(getenv("SYSTEMD_ACTIVATION_UNIT"), "systemd-resolved.service") &&
+               streq_ptr(getenv("SYSTEMD_ACTIVATION_SCOPE"), "system");
+}
+
 enum nss_status _nss_resolve_gethostbyname4_r(
                 const char *name,
                 struct gaih_addrtuple **pat,
@@ -117,6 +131,11 @@ enum nss_status _nss_resolve_gethostbyname4_r(
         assert(errnop);
         assert(h_errnop);
 
+        if (avoid_deadlock()) {
+                r = -EDEADLK;
+                goto fail;
+        }
+
         r = sd_bus_open_system(&bus);
         if (r < 0)
                 goto fail;
@@ -292,6 +311,11 @@ enum nss_status _nss_resolve_gethostbyname3_r(
                 goto fail;
         }
 
+        if (avoid_deadlock()) {
+                r = -EDEADLK;
+                goto fail;
+        }
+
         r = sd_bus_open_system(&bus);
         if (r < 0)
                 goto fail;
@@ -479,6 +503,11 @@ enum nss_status _nss_resolve_gethostbyaddr2_r(
                 return NSS_STATUS_UNAVAIL;
         }
 
+        if (avoid_deadlock()) {
+                r = -EDEADLK;
+                goto fail;
+        }
+
         r = sd_bus_open_system(&bus);
         if (r < 0)
                 goto fail;
diff --git a/src/partition/growfs.c b/src/partition/growfs.c
index d2053cd391..6f1aa28933 100644
--- a/src/partition/growfs.c
+++ b/src/partition/growfs.c
@@ -24,8 +24,8 @@
 #include "path-util.h"
 #include "strv.h"
 
-const char *arg_target = NULL;
-bool arg_dry_run = false;
+static const char *arg_target = NULL;
+static bool arg_dry_run = false;
 
 static int resize_ext4(const char *path, int mountfd, int devfd, uint64_t numblocks, uint64_t blocksize) {
         assert((uint64_t) (int) blocksize == blocksize);
diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c
index c607b5319b..28b830bd41 100644
--- a/src/shared/bus-unit-util.c
+++ b/src/shared/bus-unit-util.c
@@ -505,9 +505,9 @@ static int bus_append_cgroup_property(sd_bus_message *m, const char *field, cons
                         path = strndupa(eq, e - eq);
                         bandwidth = e+1;
 
-                        if (streq(bandwidth, "infinity")) {
+                        if (streq(bandwidth, "infinity"))
                                 bytes = CGROUP_LIMIT_MAX;
-                        } else {
+                        else {
                                 r = parse_size(bandwidth, 1000, &bytes);
                                 if (r < 0)
                                         return log_error_errno(r, "Failed to parse byte value %s: %m", bandwidth);