From ccfc19c960fca86317eb44d777a6bd20342bac80 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Jun 2018 17:37:32 +0200 Subject: [PATCH 1/8] env-util: make env-util.h self contained The header file references strlen(), hence it should include string.h --- src/basic/env-util.h | 1 + 1 file changed, 1 insertion(+) 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 #include "macro.h" +#include "string.h" bool env_name_is_valid(const char *e); bool env_value_is_valid(const char *e); From 6b228852bccefd515a45f04c801c31ed99607958 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Jun 2018 17:37:53 +0200 Subject: [PATCH 2/8] path-util: avoid name clashes One of those days we should rework this to use the UNIQ macros, but for now, an underscore should be enough. --- src/basic/path-util.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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; \ } \ From 088c49c3da948ca1558d2a14280d10669fc9b8af Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 29 Jun 2018 16:50:34 +0200 Subject: [PATCH 3/8] growfs: make global variables that don't need to be exported static --- src/partition/growfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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); From abc291aafbcbbd28e4d8f42cb1550f29f0f77d7b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 4 Jul 2018 15:27:29 +0200 Subject: [PATCH 4/8] NEWS: document nss-ldap incompatibilities --- NEWS | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 537c4b6131..da6142b9a3 100644 --- a/NEWS +++ b/NEWS @@ -82,7 +82,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 From d521916d0f8fa2a10efbc3aa2b0f700e27f3d21b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 4 Jul 2018 15:35:28 +0200 Subject: [PATCH 5/8] pid1: tell PAM/NSS modules why we are calling them --- src/core/execute.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/core/execute.c b/src/core/execute.c index a08e3105bf..87add14a73 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2835,10 +2835,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"); From 399222176728f6d1b4eacc501c2a6b54a6a76190 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 4 Jul 2018 15:36:06 +0200 Subject: [PATCH 6/8] doc: document the two new env vars set by the service manager --- doc/ENVIRONMENT.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) 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. From 2f28018cb8f3224e19f26a795d4c79fc80cc793a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 4 Jul 2018 15:37:03 +0200 Subject: [PATCH 7/8] nss: never become IPC clients for services that are about to be started This is an attempt to automatically detect and avoid certain kinds of NSS deadlocks as discussed in this thread: https://lists.freedesktop.org/archives/systemd-devel/2018-July/040975.html --- src/nss-mymachines/nss-mymachines.c | 44 +++++++++++++++++++++++++++++ src/nss-resolve/nss-resolve.c | 29 +++++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/src/nss-mymachines/nss-mymachines.c b/src/nss-mymachines/nss-mymachines.c index 8d6caa0ada..d56d3b4c21 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, @@ -102,6 +116,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; @@ -254,6 +273,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; @@ -424,6 +448,11 @@ enum nss_status _nss_mymachines_getpwnam_r( * running on the host. */ goto not_found; + if (avoid_deadlock()) { + r = -EDEADLK; + goto fail; + } + r = sd_bus_open_system(&bus); if (r < 0) goto fail; @@ -505,6 +534,11 @@ enum nss_status _nss_mymachines_getpwuid_r( if (getenv_bool_secure("SYSTEMD_NSS_BYPASS_BUS") > 0) goto not_found; + if (avoid_deadlock()) { + r = -EDEADLK; + goto fail; + } + r = sd_bus_open_system(&bus); if (r < 0) goto fail; @@ -601,6 +635,11 @@ enum nss_status _nss_mymachines_getgrnam_r( if (getenv_bool_secure("SYSTEMD_NSS_BYPASS_BUS") > 0) goto not_found; + if (avoid_deadlock()) { + r = -EDEADLK; + goto fail; + } + r = sd_bus_open_system(&bus); if (r < 0) goto fail; @@ -679,6 +718,11 @@ enum nss_status _nss_mymachines_getgrgid_r( if (getenv_bool_secure("SYSTEMD_NSS_BYPASS_BUS") > 0) goto not_found; + 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 eb3d2d977f..f67a28076c 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, @@ -116,6 +130,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; @@ -294,6 +313,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; @@ -484,6 +508,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; From 79d53eb8f76c83464e8ab0d4dc2680fe9bf3cb81 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 4 Jul 2018 15:38:53 +0200 Subject: [PATCH 8/8] bus-unit-util: tiny coding style fix --- src/shared/bus-unit-util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index 0c713678e2..a405d22809 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -503,9 +503,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);