From fdf0876bc7f547c1c1e571a42f010bac4231b3f8 Mon Sep 17 00:00:00 2001 From: Chris Down Date: Thu, 4 Jun 2026 12:08:57 +0800 Subject: [PATCH] core: Don't racily stop Type=dbus services when the bus broker restarts If dbus dies, it is then immediately reactivated by dbus.socket as soon as a client reconnects. In that case pid1's own connection to the bus drops and is reestablished by manager_recheck_dbus() (which comes from the unit_notify() when dbus.services goes back to being running). As part of bringing the bus back up, bus_setup_api() issues GetNameOwner() for every name we watch on behalf of a BusName= unit. For a Type=dbus service this is a problem. Consider this scenario: 1. spearmint.service is Type=dbus, owns com.example.Spearmint, and is running. 2. The broker crashes and is re-activated through dbus.socket. 3. spearmint's client connection dropped too, so it is now reconnecting to the new broker to grab its name again. 4. pid1 racily reconnects first given it is triggered the moment dbus.service reaches SERVICE_RUNNING, whereas spearmint is an ordinary client that must first notice its own disconnection and thus when we do GetNameOwner() com.example.Spearmint is reported as having no owner. 5. service_bus_name_owner_change() sets bus_name_good = false, and since service_good() returns false for a Type=dbus service without its name, service_enter_running() stops the (perfectly healthy, other than dbus kicking the bucket temporarily) service. In other words, a broker restart can tear down running Type=dbus services that did nothing wrong, really. So, to fix this, we can use the fact that a NameOwnerChanged signal is definitive. That is, the name's owner changed while we were connected, so a service that crashes or drops its name can still be stopped immediately. By contrast a GetNameOwner() reply is less authoritative. There is, importantly, no change to how losing the name on a live connection works, we only give grace on reconnect. --- src/core/service.c | 122 ++++++++++++++++++++++++++++++++++++++------- src/core/service.h | 2 + src/core/unit.c | 9 ++-- src/core/unit.h | 2 +- 4 files changed, 113 insertions(+), 22 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index bf6e91016c..5c097c139d 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -70,6 +70,8 @@ #define SERVICE_FD_STORE_POPULATED(s) (!!(s)->fd_store) +#define SERVICE_BUS_NAME_GRACE_USEC (2 * USEC_PER_SEC) + static const UnitActiveState state_translation_table[_SERVICE_STATE_MAX] = { [SERVICE_DEAD] = UNIT_INACTIVE, [SERVICE_CONDITION] = UNIT_ACTIVATING, @@ -142,6 +144,7 @@ static int service_dispatch_inotify_io(sd_event_source *source, int fd, uint32_t static int service_dispatch_timer(sd_event_source *source, usec_t usec, void *userdata); static int service_dispatch_watchdog(sd_event_source *source, usec_t usec, void *userdata); static int service_dispatch_exec_io(sd_event_source *source, int fd, uint32_t events, void *userdata); +static int service_dispatch_bus_name_grace(sd_event_source *source, usec_t usec, void *userdata); static void service_enter_signal(Service *s, ServiceState state, ServiceResult f); @@ -215,6 +218,7 @@ static void service_init(Unit *u) { s->oom_policy = _OOM_POLICY_INVALID; s->reload_begin_usec = USEC_INFINITY; + s->revalidate_runtime_begin_usec = USEC_INFINITY; s->reload_signal = SIGHUP; s->fd_store_preserve_mode = EXEC_PRESERVE_RESTART; @@ -620,6 +624,7 @@ static void service_done(Unit *u) { service_stop_watchdog(s); s->timer_event_source = sd_event_source_disable_unref(s->timer_event_source); + s->bus_name_grace_event_source = sd_event_source_disable_unref(s->bus_name_grace_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); @@ -1670,6 +1675,9 @@ static void service_set_state(Service *s, ServiceState state) { SERVICE_CLEANING)) s->timer_event_source = sd_event_source_disable_unref(s->timer_event_source); + if (state != SERVICE_RUNNING_REVALIDATING) + s->bus_name_grace_event_source = sd_event_source_disable_unref(s->bus_name_grace_event_source); + if (!SERVICE_STATE_WITH_MAIN_PROCESS(state)) { service_unwatch_main_pid(s); s->main_command = NULL; @@ -1783,6 +1791,14 @@ static int service_coldplug(Unit *u) { if (r < 0) return r; + if (s->deserialized_state == SERVICE_RUNNING_REVALIDATING) { + r = unit_arm_timer(UNIT(s), &s->bus_name_grace_event_source, /* relative= */ true, + SERVICE_BUS_NAME_GRACE_USEC, + service_dispatch_bus_name_grace); + if (r < 0) + return r; + } + if (pidref_is_set(&s->main_pid) && pidref_is_unwaited(&s->main_pid) > 0 && SERVICE_STATE_WITH_MAIN_PROCESS(s->deserialized_state)) { @@ -2741,14 +2757,11 @@ static void service_enter_stop(Service *s, ServiceResult f) { service_enter_signal(s, SERVICE_STOP_SIGTERM, SERVICE_SUCCESS); } -static bool service_good(Service *s) { +static bool service_good_except_bus_name(Service *s) { int main_pid_ok; assert(s); - if (s->type == SERVICE_DBUS && !s->bus_name_good) - return false; - main_pid_ok = main_pid_good(s); if (main_pid_ok > 0) /* It's alive */ return true; @@ -2762,6 +2775,26 @@ static bool service_good(Service *s) { return cgroup_good(s) != 0; } +static bool service_good(Service *s) { + assert(s); + + if (s->type == SERVICE_DBUS && !s->bus_name_good) + return false; + + return service_good_except_bus_name(s); +} + +static void service_enter_exited_or_stop(Service *s) { + assert(s); + + s->revalidate_runtime_begin_usec = USEC_INFINITY; + + if (s->remain_after_exit) + service_set_state(s, SERVICE_EXITED); + else + service_enter_stop(s, SERVICE_SUCCESS); +} + static void service_enter_running(Service *s, ServiceResult f) { int r; @@ -2784,7 +2817,17 @@ static void service_enter_running(Service *s, ServiceResult f) { else { service_set_state(s, SERVICE_RUNNING); - r = service_arm_timer(s, /* relative= */ false, service_running_timeout(s)); + /* We're resuming. Recompute the budget from the current config (which was measured + * from when we entered revalidation). */ + usec_t remaining = USEC_INFINITY; + if (s->revalidate_runtime_begin_usec != USEC_INFINITY) + remaining = usec_sub_unsigned(service_running_timeout(s), s->revalidate_runtime_begin_usec); + s->revalidate_runtime_begin_usec = USEC_INFINITY; + + if (remaining != USEC_INFINITY) + r = service_arm_timer(s, /* relative= */ true, remaining); + else + r = service_arm_timer(s, /* relative= */ false, service_running_timeout(s)); if (r < 0) { log_unit_warning_errno(UNIT(s), r, "Failed to install timer: %m"); service_enter_running(s, SERVICE_FAILURE_RESOURCES); @@ -2792,10 +2835,33 @@ static void service_enter_running(Service *s, ServiceResult f) { } } - } else if (s->remain_after_exit) - service_set_state(s, SERVICE_EXITED); - else - service_enter_stop(s, SERVICE_SUCCESS); + } else if (s->type == SERVICE_DBUS && !s->bus_name_good && service_good_except_bus_name(s)) { + + /* If we're already revalidating, just keep waiting on the timer we armed on the way in, so a + * flapping broker that reconnects repeatedly can't keep pushing the deadline out. */ + if (s->state == SERVICE_RUNNING_REVALIDATING) + return; + + log_unit_warning(UNIT(s), + "D-Bus name %s vanished (possibly from an unsupported broker restart), giving it %s to return.", + s->bus_name, + FORMAT_TIMESPAN(SERVICE_BUS_NAME_GRACE_USEC, USEC_PER_SEC)); + + r = unit_arm_timer(UNIT(s), &s->bus_name_grace_event_source, /* relative= */ true, + SERVICE_BUS_NAME_GRACE_USEC, service_dispatch_bus_name_grace); + if (r < 0) { + log_unit_warning_errno(UNIT(s), r, "Failed to arm D-Bus name grace timer: %m"); + service_enter_stop(s, SERVICE_FAILURE_RESOURCES); + return; + } + + if (s->state == SERVICE_RUNNING) + s->revalidate_runtime_begin_usec = now(CLOCK_MONOTONIC); + + service_set_state(s, SERVICE_RUNNING_REVALIDATING); + + } else + service_enter_exited_or_stop(s); } static void service_enter_start_post(Service *s) { @@ -3561,6 +3627,7 @@ static int service_start(Unit *u) { s->result = SERVICE_SUCCESS; s->reload_result = SERVICE_SUCCESS; s->reload_begin_usec = USEC_INFINITY; + s->revalidate_runtime_begin_usec = USEC_INFINITY; s->status_text = mfree(s->status_text); s->status_errno = 0; @@ -3926,6 +3993,7 @@ static int service_serialize(Unit *u, FILE *f, FDSet *fds) { (void) serialize_usec(f, "watchdog-override-usec", s->watchdog_override_usec); (void) serialize_usec(f, "reload-begin-usec", s->reload_begin_usec); + (void) serialize_usec(f, "revalidate-runtime-begin-usec", s->revalidate_runtime_begin_usec); if (s->refreshed_mask > 0) { _cleanup_strv_free_ char **l = NULL; @@ -4361,6 +4429,8 @@ static int service_deserialize_item(Unit *u, const char *key, const char *value, } else if (streq(key, "reload-begin-usec")) (void) deserialize_usec(value, &s->reload_begin_usec); + else if (streq(key, "revalidate-runtime-begin-usec")) + (void) deserialize_usec(value, &s->revalidate_runtime_begin_usec); else if (streq(key, "refreshed-mask")) { r = service_refresh_on_reload_from_string_many(value, &s->refreshed_mask); if (r < 0) @@ -5446,7 +5516,7 @@ static void service_notify_message_process_state(Service *s, char * const *tags) if (strv_contains(tags, "STOPPING=1")) { s->notify_state = NOTIFY_STOPPING; - if (IN_SET(s->state, SERVICE_RUNNING, + if (IN_SET(s->state, SERVICE_RUNNING, SERVICE_RUNNING_REVALIDATING, SERVICE_REFRESH_EXTENSIONS, SERVICE_REFRESH_CREDENTIALS, SERVICE_RELOAD_SIGNAL, SERVICE_RELOAD_NOTIFY)) service_enter_stop_by_notify(s); @@ -5483,7 +5553,7 @@ static void service_notify_message_process_state(Service *s, char * const *tags) /* Valid Type=notify-reload protocol? Then we're all good. */ service_enter_reload_post(s); - else if (s->state == SERVICE_RUNNING) { + else if (IN_SET(s->state, SERVICE_RUNNING, SERVICE_RUNNING_REVALIDATING)) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; /* Propagate a reload explicitly for plain RELOADING=1 (semantically equivalent to @@ -5524,6 +5594,8 @@ static void service_notify_message_process_state(Service *s, char * const *tags) * don't need reload propagation nor do we want to restart the timeout. */ service_set_state(s, SERVICE_RELOAD_NOTIFY); + /* The reload will still be preserved by service_enter_running() replaying it once the bus + * name returns. */ if (s->state == SERVICE_RUNNING) service_enter_reload_by_notify(s); } @@ -5558,7 +5630,7 @@ static void service_notify_message( r = service_notify_message_parse_new_pid(u, tags, fds, &new_main_pid); if (r > 0 && - IN_SET(s->state, SERVICE_START, SERVICE_START_POST, SERVICE_RUNNING, + IN_SET(s->state, SERVICE_START, SERVICE_START_POST, SERVICE_RUNNING, SERVICE_RUNNING_REVALIDATING, SERVICE_REFRESH_EXTENSIONS, SERVICE_REFRESH_CREDENTIALS, SERVICE_RELOAD, SERVICE_RELOAD_SIGNAL, SERVICE_RELOAD_NOTIFY, SERVICE_RELOAD_POST, SERVICE_STOP, SERVICE_STOP_SIGTERM) && @@ -5702,7 +5774,7 @@ static void service_notify_message( } /* Interpret RESTART_RESET=1 */ - if (strv_contains(tags, "RESTART_RESET=1") && IN_SET(s->state, SERVICE_RUNNING, SERVICE_STOP)) { + if (strv_contains(tags, "RESTART_RESET=1") && IN_SET(s->state, SERVICE_RUNNING, SERVICE_RUNNING_REVALIDATING, SERVICE_STOP)) { log_unit_struct(u, LOG_NOTICE, LOG_UNIT_MESSAGE(u, "Got RESTART_RESET=1, resetting restart counter from %u.", s->n_restarts), LOG_ITEM("N_RESTARTS=0"), @@ -5877,7 +5949,19 @@ static int bus_name_pid_lookup_callback(sd_bus_message *reply, void *userdata, s return 1; } -static void service_bus_name_owner_change(Unit *u, const char *new_owner) { +static int service_dispatch_bus_name_grace(sd_event_source *source, usec_t usec, void *userdata) { + Service *s = ASSERT_PTR(SERVICE(userdata)); + + assert(source == s->bus_name_grace_event_source); + assert(s->state == SERVICE_RUNNING_REVALIDATING); + + log_unit_warning(UNIT(s), "D-Bus name %s still not owned after grace period, giving up.", s->bus_name); + service_enter_exited_or_stop(s); + + return 0; +} + +static void service_bus_name_owner_change(Unit *u, const char *new_owner, bool from_signal) { Service *s = ASSERT_PTR(SERVICE(u)); int r; @@ -5889,10 +5973,12 @@ static void service_bus_name_owner_change(Unit *u, const char *new_owner) { s->bus_name_good = new_owner; if (s->type == SERVICE_DBUS) { - /* service_enter_running() will figure out what to do */ - if (s->state == SERVICE_RUNNING) - service_enter_running(s, SERVICE_SUCCESS); - else if (s->state == SERVICE_START && new_owner) + if (IN_SET(s->state, SERVICE_RUNNING, SERVICE_RUNNING_REVALIDATING)) { + if (!new_owner && from_signal) + service_enter_exited_or_stop(s); + else + service_enter_running(s, SERVICE_SUCCESS); + } else if (s->state == SERVICE_START && new_owner) service_enter_start_post(s); } else if (new_owner && pick_up_pid_from_bus_name(s)) { diff --git a/src/core/service.h b/src/core/service.h index c028248bba..c6002eb1ba 100644 --- a/src/core/service.h +++ b/src/core/service.h @@ -163,6 +163,7 @@ typedef struct Service { bool timeout_abort_set; usec_t runtime_max_usec; usec_t runtime_rand_extra_usec; + usec_t revalidate_runtime_begin_usec; ServiceTimeoutFailureMode timeout_start_failure_mode; ServiceTimeoutFailureMode timeout_stop_failure_mode; @@ -226,6 +227,7 @@ typedef struct Service { bool exec_fd_hot:1; char *bus_name; + sd_event_source *bus_name_grace_event_source; char *status_text; char *status_bus_error; diff --git a/src/core/unit.c b/src/core/unit.c index 1d5fe8859e..31d3740d0f 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -3593,7 +3593,7 @@ static int signal_name_owner_changed_install_handler(sd_bus_message *message, vo u->get_name_owner_slot = sd_bus_slot_unref(u->get_name_owner_slot); if (UNIT_VTABLE(u)->bus_name_owner_change) - UNIT_VTABLE(u)->bus_name_owner_change(u, NULL); + UNIT_VTABLE(u)->bus_name_owner_change(u, NULL, /* from_signal= */ true); return 0; } @@ -3612,7 +3612,7 @@ static int signal_name_owner_changed(sd_bus_message *message, void *userdata, sd } if (UNIT_VTABLE(u)->bus_name_owner_change) - UNIT_VTABLE(u)->bus_name_owner_change(u, empty_to_null(new_owner)); + UNIT_VTABLE(u)->bus_name_owner_change(u, empty_to_null(new_owner), /* from_signal= */ true); return 0; } @@ -3645,8 +3645,11 @@ static int get_name_owner_handler(sd_bus_message *message, void *userdata, sd_bu assert(!isempty(new_owner)); } + /* Given we're in a GetNameOwner() refresh, which we issue right after (re)connecting to the broker, + * a NULL owner here may simply mean the owning client is still getting its bearings, so treat it as + * non-definitive and worth waiting a bit for. */ if (UNIT_VTABLE(u)->bus_name_owner_change) - UNIT_VTABLE(u)->bus_name_owner_change(u, new_owner); + UNIT_VTABLE(u)->bus_name_owner_change(u, new_owner, /* from_signal= */ false); return 0; } diff --git a/src/core/unit.h b/src/core/unit.h index f96ae27936..c07588ac1e 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -670,7 +670,7 @@ typedef struct UnitVTable { void (*notify_pidref)(Unit *u, PidRef *parent_pidref, PidRef *child_pidref); /* Called whenever a name this Unit registered for comes or goes away. */ - void (*bus_name_owner_change)(Unit *u, const char *new_owner); + void (*bus_name_owner_change)(Unit *u, const char *new_owner, bool from_signal); /* Called for each property that is being set */ int (*bus_set_property)(Unit *u, const char *name, sd_bus_message *message, UnitWriteFlags flags, sd_bus_error *reterr_error);