From 47266ea224a3b43b364d6f80e3ec0315fadc699a Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 30 May 2024 20:44:58 +0800 Subject: [PATCH 1/5] core/dbus-manager: only allow Freeze/ThawUnit() if loaded --- src/core/dbus-manager.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 2aa4336121..2515f54ada 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -834,11 +834,13 @@ static int method_clean_unit(sd_bus_message *message, void *userdata, sd_bus_err } static int method_freeze_unit(sd_bus_message *message, void *userdata, sd_bus_error *error) { - return method_generic_unit_operation(message, userdata, error, bus_unit_method_freeze, 0); + /* Only active units can be frozen, which must be properly loaded already */ + return method_generic_unit_operation(message, userdata, error, bus_unit_method_freeze, GENERIC_UNIT_VALIDATE_LOADED); } static int method_thaw_unit(sd_bus_message *message, void *userdata, sd_bus_error *error) { - return method_generic_unit_operation(message, userdata, error, bus_unit_method_thaw, 0); + /* Same as freeze above */ + return method_generic_unit_operation(message, userdata, error, bus_unit_method_thaw, GENERIC_UNIT_VALIDATE_LOADED); } static int method_reset_failed_unit(sd_bus_message *message, void *userdata, sd_bus_error *error) { From 759ac0dec33bbd9d307f19e4565d4548215fe5b4 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Fri, 24 May 2024 22:53:49 +0800 Subject: [PATCH 2/5] core/dbus-unit: modernize bus_unit_method_freezer_generic a bit --- src/core/dbus-unit.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 20fa43af9e..953cd512a3 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -723,15 +723,13 @@ int bus_unit_method_clean(sd_bus_message *message, void *userdata, sd_bus_error } static int bus_unit_method_freezer_generic(sd_bus_message *message, void *userdata, sd_bus_error *error, FreezerAction action) { - const char* perm; Unit *u = ASSERT_PTR(userdata); - bool reply_no_delay = false; int r; assert(message); assert(IN_SET(action, FREEZER_FREEZE, FREEZER_THAW)); - perm = action == FREEZER_FREEZE ? "stop" : "start"; + const char *perm = action == FREEZER_FREEZE ? "stop" : "start"; r = mac_selinux_unit_access_check(u, message, perm, error); if (r < 0) @@ -750,19 +748,19 @@ static int bus_unit_method_freezer_generic(sd_bus_message *message, void *userda r = unit_freezer_action(u, action); if (r == -EOPNOTSUPP) - return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, "Unit does not support freeze/thaw."); + return sd_bus_error_set(error, SD_BUS_ERROR_NOT_SUPPORTED, "Unit does not support freeze/thaw"); if (r == -EBUSY) - return sd_bus_error_set(error, BUS_ERROR_UNIT_BUSY, "Unit has a pending job."); + return sd_bus_error_set(error, BUS_ERROR_UNIT_BUSY, "Unit has a pending job"); if (r == -EHOSTDOWN) - return sd_bus_error_set(error, BUS_ERROR_UNIT_INACTIVE, "Unit is inactive."); + return sd_bus_error_set(error, BUS_ERROR_UNIT_INACTIVE, "Unit is not active"); if (r == -EALREADY) - return sd_bus_error_setf(error, SD_BUS_ERROR_FAILED, "Previously requested freezer operation for unit is still in progress."); + return sd_bus_error_set(error, BUS_ERROR_UNIT_BUSY, "Previously requested freezer operation for unit is still in progress"); if (r == -ECHILD) - return sd_bus_error_setf(error, SD_BUS_ERROR_FAILED, "Unit is frozen by a parent slice."); + return sd_bus_error_set(error, SD_BUS_ERROR_FAILED, "Unit is frozen by a parent slice"); if (r < 0) return r; - if (r == 0) - reply_no_delay = true; + + bool reply_now = r == 0; if (u->pending_freezer_invocation) { bus_unit_send_pending_freezer_message(u, true); @@ -771,7 +769,7 @@ static int bus_unit_method_freezer_generic(sd_bus_message *message, void *userda u->pending_freezer_invocation = sd_bus_message_ref(message); - if (reply_no_delay) { + if (reply_now) { r = bus_unit_send_pending_freezer_message(u, false); if (r < 0) return r; From 12904d5387059dec6e148fc7f33fc0e3087009d4 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 30 May 2024 14:43:41 +0800 Subject: [PATCH 3/5] bus-unit-util: rework UnitFreezer, explicitly thaw unit Currently, we don't explicitly call unit_freezer_thaw(), but rely on the destructor to thaw the frozen unit on return. This has several problems though, one of them being that we ignore the return value of ThawUnit(), which is something we really shouldn't do here, since such failure can easily leave the whole system in unusable state. Moreover, the logging is kinda messy, e.g. homed might log "Everything completed" yet immediately followed by "Failed to thaw unit". Instead, we should log consistently and at higher level, to make things more debuggable. Therefore, let's step away from the practice. Plus, make UnitFreezer object heap-allocated, to match with existing unit_freezer_new() and allow us to use NULL to denote that the freezer is disabled. --- src/home/homework.c | 37 ++++++++------- src/shared/bus-unit-util.c | 92 +++++++++++++++++++++----------------- src/shared/bus-unit-util.h | 15 +++---- src/sleep/sleep.c | 15 +++---- 4 files changed, 87 insertions(+), 72 deletions(-) diff --git a/src/home/homework.c b/src/home/homework.c index e10922cefd..428f832911 100644 --- a/src/home/homework.c +++ b/src/home/homework.c @@ -1866,10 +1866,13 @@ static int home_inspect(UserRecord *h, UserRecord **ret_home) { return 1; } -static int user_session_freezer(uid_t uid, bool freeze_now, UnitFreezer *ret) { +static int user_session_freezer(uid_t uid, bool freeze_now, UnitFreezer **ret) { _cleanup_free_ char *unit = NULL; int r; + assert(uid_is_valid(uid)); + assert(ret); + r = getenv_bool("SYSTEMD_HOME_LOCK_FREEZE_SESSION"); if (r < 0 && r != -ENXIO) log_warning_errno(r, "Cannot parse value of $SYSTEMD_HOME_LOCK_FREEZE_SESSION, ignoring."); @@ -1878,7 +1881,8 @@ static int user_session_freezer(uid_t uid, bool freeze_now, UnitFreezer *ret) { log_notice("Session remains unfrozen on explicit request ($SYSTEMD_HOME_LOCK_FREEZE_SESSION " "is set to false). This is not recommended, and might result in unexpected behavior " "including data loss!"); - *ret = (UnitFreezer) {}; + + *ret = NULL; return 0; } @@ -1891,12 +1895,12 @@ static int user_session_freezer(uid_t uid, bool freeze_now, UnitFreezer *ret) { r = unit_freezer_new(unit, ret); if (r < 0) return r; + return 1; } static int home_lock(UserRecord *h) { _cleanup_(home_setup_done) HomeSetup setup = HOME_SETUP_INIT; - _cleanup_(unit_freezer_done_thaw) UnitFreezer freezer = {}; int r; assert(h); @@ -1912,19 +1916,19 @@ static int home_lock(UserRecord *h) { if (r != USER_TEST_MOUNTED) return log_error_errno(SYNTHETIC_ERRNO(ENOEXEC), "Home directory of %s is not mounted, can't lock.", h->user_name); - r = user_session_freezer(h->uid, /* freeze_now= */ true, &freezer); - if (r < 0) - log_warning_errno(r, "Failed to freeze user session, ignoring: %m"); - else if (r == 0) - log_info("User session freeze disabled, skipping."); - else - log_info("Froze user session."); + _cleanup_(unit_freezer_freep) UnitFreezer *f = NULL; - r = home_lock_luks(h, &setup); + r = user_session_freezer(h->uid, /* freeze_now= */ true, &f); if (r < 0) return r; - unit_freezer_done(&freezer); /* Don't thaw the user session. */ + r = home_lock_luks(h, &setup); + if (r < 0) { + if (f) + (void) unit_freezer_thaw(f); + + return r; + } /* Explicitly flush any per-user key from the keyring */ (void) keyring_flush(h); @@ -1935,7 +1939,6 @@ static int home_lock(UserRecord *h) { static int home_unlock(UserRecord *h) { _cleanup_(home_setup_done) HomeSetup setup = HOME_SETUP_INIT; - _cleanup_(unit_freezer_done_thaw) UnitFreezer freezer = {}; _cleanup_(password_cache_free) PasswordCache cache = {}; int r; @@ -1957,10 +1960,14 @@ static int home_unlock(UserRecord *h) { if (r < 0) return r; + _cleanup_(unit_freezer_freep) UnitFreezer *f = NULL; + /* We want to thaw the session only after it's safe to access $HOME */ - r = user_session_freezer(h->uid, /* freeze_now= */ false, &freezer); + r = user_session_freezer(h->uid, /* freeze_now= */ false, &f); + if (r > 0) + r = unit_freezer_thaw(f); if (r < 0) - log_warning_errno(r, "Failed to recover freezer for user session, ignoring: %m"); + return r; log_info("Everything completed."); return 1; diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index fd46332821..8de9f374e3 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -2,6 +2,7 @@ #include "af-list.h" #include "alloc-util.h" +#include "bus-common-errors.h" #include "bus-error.h" #include "bus-locator.h" #include "bus-unit-util.h" @@ -2939,41 +2940,51 @@ int bus_service_manager_reload(sd_bus *bus) { return 0; } +typedef struct UnitFreezer { + char *name; + sd_bus *bus; +} UnitFreezer; + /* Wait for 1.5 seconds at maximum for freeze operation */ #define FREEZE_BUS_CALL_TIMEOUT (1500 * USEC_PER_MSEC) -int unit_freezer_new(const char *name, UnitFreezer *ret) { - _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; - _cleanup_free_ char *namedup = NULL; +UnitFreezer* unit_freezer_free(UnitFreezer *f) { + if (!f) + return NULL; + + free(f->name); + sd_bus_flush_close_unref(f->bus); + + return mfree(f); +} + +int unit_freezer_new(const char *name, UnitFreezer **ret) { + _cleanup_(unit_freezer_freep) UnitFreezer *f = NULL; int r; assert(name); assert(ret); - namedup = strdup(name); - if (!namedup) - return log_oom_debug(); + f = new(UnitFreezer, 1); + if (!f) + return log_oom(); - r = bus_connect_system_systemd(&bus); - if (r < 0) - return log_debug_errno(r, "Failed to open connection to systemd: %m"); - - (void) sd_bus_set_method_call_timeout(bus, FREEZE_BUS_CALL_TIMEOUT); - - *ret = (UnitFreezer) { - .name = TAKE_PTR(namedup), - .bus = TAKE_PTR(bus), + *f = (UnitFreezer) { + .name = strdup(name), }; + if (!f->name) + return log_oom(); + + r = bus_connect_system_systemd(&f->bus); + if (r < 0) + return log_error_errno(r, "Failed to open connection to systemd: %m"); + + (void) sd_bus_set_method_call_timeout(f->bus, FREEZE_BUS_CALL_TIMEOUT); + + *ret = TAKE_PTR(f); return 0; } -void unit_freezer_done(UnitFreezer *f) { - assert(f); - - f->name = mfree(f->name); - f->bus = sd_bus_flush_close_unref(f->bus); -} - static int unit_freezer_action(UnitFreezer *f, bool freeze) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; int r; @@ -2988,11 +2999,22 @@ static int unit_freezer_action(UnitFreezer *f, bool freeze) { /* reply = */ NULL, "s", f->name); - if (r < 0) - return log_debug_errno(r, "Failed to %s unit %s: %s", - freeze ? "freeze" : "thaw", f->name, bus_error_message(&error, r)); + if (r < 0) { + if (sd_bus_error_has_names(&error, + BUS_ERROR_NO_SUCH_UNIT, + BUS_ERROR_UNIT_INACTIVE, + SD_BUS_ERROR_NOT_SUPPORTED)) { - return 0; + log_debug_errno(r, "Skipping freezer for '%s': %s", f->name, bus_error_message(&error, r)); + return 0; + } + + return log_error_errno(r, "Failed to %s unit '%s': %s", + freeze ? "freeze" : "thaw", f->name, bus_error_message(&error, r)); + } + + log_info("Successfully %s unit '%s'.", freeze ? "froze" : "thawed", f->name); + return 1; } int unit_freezer_freeze(UnitFreezer *f) { @@ -3003,8 +3025,8 @@ int unit_freezer_thaw(UnitFreezer *f) { return unit_freezer_action(f, false); } -int unit_freezer_new_freeze(const char *name, UnitFreezer *ret) { - _cleanup_(unit_freezer_done) UnitFreezer f = {}; +int unit_freezer_new_freeze(const char *name, UnitFreezer **ret) { + _cleanup_(unit_freezer_freep) UnitFreezer *f = NULL; int r; assert(name); @@ -3014,20 +3036,10 @@ int unit_freezer_new_freeze(const char *name, UnitFreezer *ret) { if (r < 0) return r; - r = unit_freezer_freeze(&f); + r = unit_freezer_freeze(f); if (r < 0) return r; - *ret = TAKE_STRUCT(f); + *ret = TAKE_PTR(f); return 0; } - -void unit_freezer_done_thaw(UnitFreezer *f) { - assert(f); - - if (!f->name) - return; - - (void) unit_freezer_thaw(f); - unit_freezer_done(f); -} diff --git a/src/shared/bus-unit-util.h b/src/shared/bus-unit-util.h index e4168a4425..ea4056c691 100644 --- a/src/shared/bus-unit-util.h +++ b/src/shared/bus-unit-util.h @@ -36,17 +36,14 @@ int unit_info_compare(const UnitInfo *a, const UnitInfo *b); int bus_service_manager_reload(sd_bus *bus); -typedef struct UnitFreezer { - char *name; - sd_bus *bus; -} UnitFreezer; +typedef struct UnitFreezer UnitFreezer; -int unit_freezer_new(const char *name, UnitFreezer *ret); -void unit_freezer_done(UnitFreezer *f); +UnitFreezer* unit_freezer_free(UnitFreezer *f); +DEFINE_TRIVIAL_CLEANUP_FUNC(UnitFreezer*, unit_freezer_free); + +int unit_freezer_new(const char *name, UnitFreezer **ret); int unit_freezer_freeze(UnitFreezer *f); int unit_freezer_thaw(UnitFreezer *f); -int unit_freezer_new_freeze(const char *name, UnitFreezer *ret); - -void unit_freezer_done_thaw(UnitFreezer *f); +int unit_freezer_new_freeze(const char *name, UnitFreezer **ret); diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index c96207428d..1911ef256e 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -580,7 +580,7 @@ static int parse_argv(int argc, char *argv[]) { } static int run(int argc, char *argv[]) { - _cleanup_(unit_freezer_done_thaw) UnitFreezer user_slice_freezer = {}; + _cleanup_(unit_freezer_freep) UnitFreezer *user_slice_freezer = NULL; _cleanup_(sleep_config_freep) SleepConfig *sleep_config = NULL; int r; @@ -603,13 +603,9 @@ static int run(int argc, char *argv[]) { r = getenv_bool("SYSTEMD_SLEEP_FREEZE_USER_SESSIONS"); if (r < 0 && r != -ENXIO) log_warning_errno(r, "Cannot parse value of $SYSTEMD_SLEEP_FREEZE_USER_SESSIONS, ignoring."); - if (r != 0) { - r = unit_freezer_new_freeze(SPECIAL_USER_SLICE, &user_slice_freezer); - if (r < 0) - log_warning_errno(r, "Failed to freeze user sessions, ignoring: %m"); - else - log_info("Froze user sessions"); - } else + if (r != 0) + (void) unit_freezer_new_freeze(SPECIAL_USER_SLICE, &user_slice_freezer); + else log_notice("User sessions remain unfrozen on explicit request " "($SYSTEMD_SLEEP_FREEZE_USER_SESSIONS is set to false). This is not recommended, " "and might result in unexpected behavior, particularly in suspend-then-hibernate " @@ -641,6 +637,9 @@ static int run(int argc, char *argv[]) { } + if (user_slice_freezer) + RET_GATHER(r, unit_freezer_thaw(user_slice_freezer)); + return r; } From 61fbc58c46fd7776ab6cf33d3bb91c0f15136dc2 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 30 May 2024 14:45:21 +0800 Subject: [PATCH 4/5] sleep,home: clean up logs for session freezer a bit --- src/home/homework.c | 7 +++---- src/sleep/sleep.c | 7 +++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/home/homework.c b/src/home/homework.c index 428f832911..b556904651 100644 --- a/src/home/homework.c +++ b/src/home/homework.c @@ -1875,12 +1875,11 @@ static int user_session_freezer(uid_t uid, bool freeze_now, UnitFreezer **ret) { r = getenv_bool("SYSTEMD_HOME_LOCK_FREEZE_SESSION"); if (r < 0 && r != -ENXIO) - log_warning_errno(r, "Cannot parse value of $SYSTEMD_HOME_LOCK_FREEZE_SESSION, ignoring."); + log_warning_errno(r, "Cannot parse value of $SYSTEMD_HOME_LOCK_FREEZE_SESSION, ignoring: %m"); else if (r == 0) { if (freeze_now) - log_notice("Session remains unfrozen on explicit request ($SYSTEMD_HOME_LOCK_FREEZE_SESSION " - "is set to false). This is not recommended, and might result in unexpected behavior " - "including data loss!"); + log_notice("Session remains unfrozen on explicit request ($SYSTEMD_HOME_LOCK_FREEZE_SESSION=0).\n" + "This is not recommended, and might result in unexpected behavior including data loss!"); *ret = NULL; return 0; diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index 1911ef256e..0402bb07f3 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -606,10 +606,9 @@ static int run(int argc, char *argv[]) { if (r != 0) (void) unit_freezer_new_freeze(SPECIAL_USER_SLICE, &user_slice_freezer); else - log_notice("User sessions remain unfrozen on explicit request " - "($SYSTEMD_SLEEP_FREEZE_USER_SESSIONS is set to false). This is not recommended, " - "and might result in unexpected behavior, particularly in suspend-then-hibernate " - "operations or setups with encrypted home directories."); + log_notice("User sessions remain unfrozen on explicit request ($SYSTEMD_SLEEP_FREEZE_USER_SESSIONS=0).\n" + "This is not recommended, and might result in unexpected behavior, particularly\n" + "in suspend-then-hibernate operations or setups with encrypted home directories."); switch (arg_operation) { From 1d617b35fef5f7783287965f766c8bb85e932b8e Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 30 May 2024 15:09:28 +0800 Subject: [PATCH 5/5] bus-unit-util: extend the bus call timeout for UnitFreezer Might fix #33083 There's another report available on Mastodon: https://m.otter.homes/@kaixin/112510783502516607 https://m.otter.homes/@kaixin/112510803023469180 --- 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 8de9f374e3..22dbf62ab2 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -2945,8 +2945,8 @@ typedef struct UnitFreezer { sd_bus *bus; } UnitFreezer; -/* Wait for 1.5 seconds at maximum for freeze operation */ -#define FREEZE_BUS_CALL_TIMEOUT (1500 * USEC_PER_MSEC) +/* Wait for 10 seconds at maximum for freezer operation */ +#define FREEZE_BUS_CALL_TIMEOUT (10 * USEC_PER_SEC) UnitFreezer* unit_freezer_free(UnitFreezer *f) { if (!f)