From 1e8b312e5a22538f91defb89cf2997e09e106297 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Feb 2022 17:23:48 +0100 Subject: [PATCH 1/4] pid1: watch bus name always when we have it Previously we'd only watch configured service bus names if Type=dbus was set. Let's also watch it for other types. This is useful to pick up the main PID of such a service. In fact the code to pick it up was already in place, alas it didn't do anything given the signal was never received for it. Fix that. (It's also useful for debugging) --- src/core/service.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index cb5233a5cf1..1344fb9e8b7 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -709,17 +709,19 @@ static int service_setup_bus_name(Service *s) { assert(s); /* If s->bus_name is not set, then the unit will be refused by service_verify() later. */ - if (s->type != SERVICE_DBUS || !s->bus_name) + if (!s->bus_name) return 0; - r = unit_add_dependency_by_name(UNIT(s), UNIT_REQUIRES, SPECIAL_DBUS_SOCKET, true, UNIT_DEPENDENCY_FILE); - if (r < 0) - return log_unit_error_errno(UNIT(s), r, "Failed to add dependency on " SPECIAL_DBUS_SOCKET ": %m"); + if (s->type == SERVICE_DBUS) { + r = unit_add_dependency_by_name(UNIT(s), UNIT_REQUIRES, SPECIAL_DBUS_SOCKET, true, UNIT_DEPENDENCY_FILE); + if (r < 0) + return log_unit_error_errno(UNIT(s), r, "Failed to add dependency on " SPECIAL_DBUS_SOCKET ": %m"); - /* We always want to be ordered against dbus.socket if both are in the transaction. */ - r = unit_add_dependency_by_name(UNIT(s), UNIT_AFTER, SPECIAL_DBUS_SOCKET, true, UNIT_DEPENDENCY_FILE); - if (r < 0) - return log_unit_error_errno(UNIT(s), r, "Failed to add dependency on " SPECIAL_DBUS_SOCKET ": %m"); + /* We always want to be ordered against dbus.socket if both are in the transaction. */ + r = unit_add_dependency_by_name(UNIT(s), UNIT_AFTER, SPECIAL_DBUS_SOCKET, true, UNIT_DEPENDENCY_FILE); + if (r < 0) + return log_unit_error_errno(UNIT(s), r, "Failed to add dependency on " SPECIAL_DBUS_SOCKET ": %m"); + } r = unit_watch_bus_name(UNIT(s), s->bus_name); if (r == -EEXIST) From e39eb045a502d599e6cd3fda7a46020dd438d018 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Feb 2022 14:40:25 +0100 Subject: [PATCH 2/4] pid1: lookup owning PID of BusName= name of services asynchronously A first step of removing blocking calls to the D-Bus broker from PID 1. There's a lot more to got (i.e. grep src/core/ for sd_bus_creds basically), but it's a start. Removing blocking calls to D-Bus broker deals systematicallly with deadlocks caused by dbus-daemon blocking on synchronous IPC calls back to PID1 (e.g. Varlink calls through nss-systemd). Bugs such as #15316. Also-see: https://github.com/systemd/systemd/pull/22038#issuecomment-1042958390 --- src/core/service.c | 89 ++++++++++++++++++++++++++++++++++++---------- src/core/service.h | 2 ++ 2 files changed, 73 insertions(+), 18 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 1344fb9e8b7..b8808ba4ec2 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -398,6 +398,8 @@ static void service_done(Unit *u) { s->timer_event_source = sd_event_source_disable_unref(s->timer_event_source); s->exec_fd_event_source = sd_event_source_disable_unref(s->exec_fd_event_source); + s->bus_name_pid_lookup_slot = sd_bus_slot_unref(s->bus_name_pid_lookup_slot); + service_release_resources(u); } @@ -4322,6 +4324,60 @@ static int service_get_timeout(Unit *u, usec_t *timeout) { return 1; } +static bool pick_up_pid_from_bus_name(Service *s) { + assert(s); + + /* If the service is running but we have no main PID yet, get it from the owner of the D-Bus name */ + + return !pid_is_valid(s->main_pid) && + IN_SET(s->state, + SERVICE_START, + SERVICE_START_POST, + SERVICE_RUNNING, + SERVICE_RELOAD); +} + +static int bus_name_pid_lookup_callback(sd_bus_message *reply, void *userdata, sd_bus_error *ret_error) { + const sd_bus_error *e; + Unit *u = userdata; + uint32_t pid; + Service *s; + int r; + + assert(reply); + assert(u); + + s = SERVICE(u); + s->bus_name_pid_lookup_slot = sd_bus_slot_unref(s->bus_name_pid_lookup_slot); + + if (!s->bus_name || !pick_up_pid_from_bus_name(s)) + return 1; + + e = sd_bus_message_get_error(reply); + if (e) { + r = sd_bus_error_get_errno(e); + log_warning_errno(r, "GetConnectionUnixProcessID() failed: %s", bus_error_message(e, r)); + return 1; + } + + r = sd_bus_message_read(reply, "u", &pid); + if (r < 0) { + bus_log_parse_error(r); + return 1; + } + + if (!pid_is_valid(pid)) { + log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "GetConnectionUnixProcessID() returned invalid PID"); + return 1; + } + + log_unit_debug(u, "D-Bus name %s is now owned by process " PID_FMT, s->bus_name, (pid_t) pid); + + service_set_main_pid(s, pid); + unit_watch_pid(UNIT(s), pid, false); + return 1; +} + static void service_bus_name_owner_change(Unit *u, const char *new_owner) { Service *s = SERVICE(u); @@ -4352,28 +4408,25 @@ static void service_bus_name_owner_change(Unit *u, const char *new_owner) { else if (s->state == SERVICE_START && new_owner) service_enter_start_post(s); - } else if (new_owner && - s->main_pid <= 0 && - IN_SET(s->state, - SERVICE_START, - SERVICE_START_POST, - SERVICE_RUNNING, - SERVICE_RELOAD)) { - - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; - pid_t pid; + } else if (new_owner && pick_up_pid_from_bus_name(s)) { /* Try to acquire PID from bus service */ - r = sd_bus_get_name_creds(u->manager->api_bus, s->bus_name, SD_BUS_CREDS_PID, &creds); - if (r >= 0) - r = sd_bus_creds_get_pid(creds, &pid); - if (r >= 0) { - log_unit_debug(u, "D-Bus name %s is now owned by process " PID_FMT, s->bus_name, pid); + s->bus_name_pid_lookup_slot = sd_bus_slot_unref(s->bus_name_pid_lookup_slot); - service_set_main_pid(s, pid); - unit_watch_pid(UNIT(s), pid, false); - } + r = sd_bus_call_method_async( + u->manager->api_bus, + &s->bus_name_pid_lookup_slot, + "org.freedesktop.DBus", + "/org/freedesktop/DBus", + "org.freedesktop.DBus", + "GetConnectionUnixProcessID", + bus_name_pid_lookup_callback, + s, + "s", + s->bus_name); + if (r < 0) + log_debug_errno(r, "Failed to request owner PID of service name, ignoring: %m"); } } diff --git a/src/core/service.h b/src/core/service.h index 778551d8441..4116e40d8f3 100644 --- a/src/core/service.h +++ b/src/core/service.h @@ -195,6 +195,8 @@ struct Service { NotifyAccess notify_access; NotifyState notify_state; + sd_bus_slot *bus_name_pid_lookup_slot; + sd_event_source *exec_fd_event_source; ServiceFDStore *fd_store; From cec16155e3dab4f123ba073223477a4ef2cf10f9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Feb 2022 14:47:34 +0100 Subject: [PATCH 3/4] docs: $SYSTEMD_NSS_BYPASS_BUS is not honoured anymore, don't document it It was removed back in 1684c56f40f020e685e70b3d1785d596ff16f892 Follow-up for: 1684c56f40f020e685e70b3d1785d596ff16f892 --- docs/ENVIRONMENT.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/docs/ENVIRONMENT.md b/docs/ENVIRONMENT.md index e8a2ce394a1..1391e9642e7 100644 --- a/docs/ENVIRONMENT.md +++ b/docs/ENVIRONMENT.md @@ -219,10 +219,6 @@ All tools: user/group records for dynamically registered service users (i.e. users registered through `DynamicUser=1`). -* `$SYSTEMD_NSS_BYPASS_BUS=1` — if set, `nss-systemd` won't use D-Bus to do - dynamic user lookups. This is primarily useful to make `nss-systemd` work - safely from within `dbus-daemon`. - `systemd-timedated`: * `$SYSTEMD_TIMEDATED_NTP_SERVICES=…` — colon-separated list of unit names of From de90700f36f2126528f7ce92df0b5b5d5e277558 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Feb 2022 14:49:54 +0100 Subject: [PATCH 4/4] pid1: set SYSTEMD_NSS_DYNAMIC_BYPASS=1 env var for dbus-daemon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There's currently a deadlock between PID 1 and dbus-daemon: in some cases dbus-daemon will do NSS lookups (which are blocking) at the same time PID 1 synchronously blocks on some call to dbus-daemon. Let's break that by setting SYSTEMD_NSS_DYNAMIC_BYPASS=1 env var for dbus-daemon, which will disable synchronously blocking varlink calls from nss-systemd to PID 1. In the long run we should fix this differently: remove all synchronous calls to dbus-daemon from PID 1. This is not trivial however: so far we had the rule that synchronous calls from PID 1 to the dbus broker are OK as long as they only go to interfaces implemented by the broke itself rather than services reachable through it. Given that the relationship between PID 1 and dbus is kinda special anyway, this was considered acceptable for the sake of simplicity, since we quite often need metadata about bus peers from the broker, and the asynchronous logic would substantially complicate even the simplest method handlers. This mostly reworks the existing code that sets SYSTEMD_NSS_BYPASS_BUS= (which is a similar hack to deal with deadlocks between nss-systemd and dbus-daemon itself) to set SYSTEMD_NSS_DYNAMIC_BYPASS=1 instead. No code was checking SYSTEMD_NSS_BYPASS_BUS= anymore anyway, and it used to solve a similar problem, hence it's an obvious piece of code to rework like this. Issue originally tracked down by Lukas Märdian. This patch is inspired and closely based on his patch: https://github.com/systemd/systemd/pull/22038 Fixes: #15316 Co-authored-by: Lukas Märdian --- src/core/execute.c | 10 +++++----- src/core/execute.h | 2 +- src/core/service.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 72f3534908a..c731dec0bd6 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1865,11 +1865,11 @@ static int build_environment( our_env[n_env++] = x; } - /* If this is D-Bus, tell the nss-systemd module, since it relies on being able to use D-Bus look up dynamic - * users via PID 1, possibly dead-locking the dbus daemon. This way it will not use D-Bus to resolve names, but - * check the database directly. */ - if (p->flags & EXEC_NSS_BYPASS_BUS) { - x = strdup("SYSTEMD_NSS_BYPASS_BUS=1"); + /* If this is D-Bus, tell the nss-systemd module, since it relies on being able to use blocking + * Varlink calls back to us for look up dynamic users in PID 1. Break the deadlock between D-Bus and + * PID 1 by disabling use of PID1' NSS interface for looking up dynamic users. */ + if (p->flags & EXEC_NSS_DYNAMIC_BYPASS) { + x = strdup("SYSTEMD_NSS_DYNAMIC_BYPASS=1"); if (!x) return -ENOMEM; our_env[n_env++] = x; diff --git a/src/core/execute.h b/src/core/execute.h index 4aff50b4424..43070ce1117 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -376,7 +376,7 @@ typedef enum ExecFlags { EXEC_APPLY_TTY_STDIN = 1 << 2, EXEC_PASS_LOG_UNIT = 1 << 3, /* Whether to pass the unit name to the service's journal stream connection */ EXEC_CHOWN_DIRECTORIES = 1 << 4, /* chown() the runtime/state/cache/log directories to the user we run as, under all conditions */ - EXEC_NSS_BYPASS_BUS = 1 << 5, /* Set the SYSTEMD_NSS_BYPASS_BUS environment variable, to disable nss-systemd for dbus */ + 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 */ EXEC_CGROUP_DELEGATE = 1 << 6, EXEC_IS_CONTROL = 1 << 7, 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 */ diff --git a/src/core/service.c b/src/core/service.c index b8808ba4ec2..785038e0085 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1681,7 +1681,7 @@ static int service_spawn( return -ENOMEM; /* System D-Bus needs nss-systemd disabled, so that we don't deadlock */ - SET_FLAG(exec_params.flags, EXEC_NSS_BYPASS_BUS, + SET_FLAG(exec_params.flags, EXEC_NSS_DYNAMIC_BYPASS, MANAGER_IS_SYSTEM(UNIT(s)->manager) && unit_has_name(UNIT(s), SPECIAL_DBUS_SERVICE)); strv_free_and_replace(exec_params.environment, final_env);