From 7483708131b474d92c9207c8c6340b450b58cb94 Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Sat, 23 Dec 2023 16:57:47 -0500 Subject: [PATCH 1/4] bus-unit-util: Add utility to freeze/thaw units This utility lets us freeze units, and then automatically thaw them when via a _cleanup_ handler. For example, you can now write something like: ``` _cleanup_(unit_freezer_thaw) UnitFreezer freezer = UNIT_FREEZER_NULL; r = unit_freezer_freeze("myunit.service", &freezer); if (r < 0) return r; // Freeze is thawed once this scope ends ``` Aside from the basic _freeze and _thaw methods, there's also _cancel and _restore. Cancel destroys the UnitFreezer without thawing the unit. Restore creates a UnitFreezer without freezing it. The idea of these two methods is that it allows the freeze/thaw to be separated from each other (i.e. done in response to two separate DBus method calls). For example: ``` _cleanup_(unit_freezer_thaw) UnitFreezer freezer = UNIT_FREEZER_NULL; r = unit_freezer_freeze("myunit.service", &freezer); if (r < 0) return r; // Freeze is thawed once this scope ends r = do_something() if (r < 0) return r; // Freeze is thawed unit_freezer_cancel(&freezer); // Thaw is canceled. ``` Then in another scope: ``` // Bring back a UnitFreezer object for the already-frozen service _cleanup_(unit_freezer_thaw) UnitFreezer freezer = UNIT_FREEZER_NULL; r = unit_freezer_restore("myunit.service", &freezer); if (r < 0) return r; // Freeze is thawed once this scope ends ``` --- src/shared/bus-unit-util.c | 87 ++++++++++++++++++++++++++++++++++++++ src/shared/bus-unit-util.h | 17 ++++++++ 2 files changed, 104 insertions(+) diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index 783ce31b3d..2f485f810d 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -10,6 +10,7 @@ #include "cgroup-setup.h" #include "cgroup-util.h" #include "condition.h" +#include "constants.h" #include "coredump-util.h" #include "cpu-set-util.h" #include "dissect-image.h" @@ -2937,3 +2938,89 @@ int bus_service_manager_reload(sd_bus *bus) { return 0; } + +int unit_freezer_new(const char *name, UnitFreezer *ret) { + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; + _cleanup_free_ char *namedup = NULL; + int r; + + assert(name); + assert(ret); + + namedup = strdup(name); + if (!namedup) + return log_oom_debug(); + + 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_TIMEOUT); + + *ret = (UnitFreezer) { + .name = TAKE_PTR(namedup), + .bus = TAKE_PTR(bus), + }; + return 0; +} + +static int unit_freezer_action(bool freeze, UnitFreezer *f) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + int r; + + assert(f); + assert(f->bus); + assert(f->name); + + r = bus_call_method(f->bus, bus_systemd_mgr, freeze ? "FreezeUnit" : "ThawUnit", + &error, 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)); + + return 0; +} + +int unit_freezer_freeze(UnitFreezer *f) { + return unit_freezer_action(true, f); +} + +int unit_freezer_thaw(UnitFreezer *f) { + return unit_freezer_action(false, f); +} + +void unit_freezer_done(UnitFreezer *f) { + assert(f); + + f->name = mfree(f->name); + f->bus = sd_bus_flush_close_unref(f->bus); +} + +int unit_freezer_new_freeze(const char *name, UnitFreezer *ret) { + _cleanup_(unit_freezer_done) UnitFreezer f = {}; + int r; + + assert(name); + assert(ret); + + r = unit_freezer_new(name, &f); + if (r < 0) + return r; + + r = unit_freezer_freeze(&f); + if (r < 0) + return r; + + *ret = TAKE_STRUCT(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 d52c8475ca..3c6a001069 100644 --- a/src/shared/bus-unit-util.h +++ b/src/shared/bus-unit-util.h @@ -35,3 +35,20 @@ int unit_load_state(sd_bus *bus, const char *name, char **load_state); 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; + +int unit_freezer_new(const char *name, UnitFreezer *ret); + +int unit_freezer_freeze(UnitFreezer *freezer); + +int unit_freezer_thaw(UnitFreezer *freezer); + +void unit_freezer_done(UnitFreezer *freezer); + +int unit_freezer_new_freeze(const char *name, UnitFreezer *ret); + +void unit_freezer_done_thaw(UnitFreezer *freezer); From 0b958bb3eeae61dd25a1f6735477ea239981b162 Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Sat, 23 Dec 2023 17:03:42 -0500 Subject: [PATCH 2/4] sleep: Always freeze user.slice Previously, we'd only freeze user.slice in the case of s2h, because we didn't want the user session to resume while systemd was transitioning from suspend to hibernate. This commit extends this freezing behavior to all sleep modes. We also have an environment variable to disable the freezing behavior outright. This is a necessary workaround for someone that has hooks in /usr/lib/systemd/system-sleep/ which communicate with some process running under user.slice, or if someone is using the proprietary NVIDIA driver which breaks when user.slice is frozen (issue #27559) Fixes #27559 --- docs/ENVIRONMENT.md | 8 ++++++ man/systemd-suspend.service.xml | 9 ++++++- src/sleep/sleep.c | 46 ++++++++++++++------------------- 3 files changed, 35 insertions(+), 28 deletions(-) diff --git a/docs/ENVIRONMENT.md b/docs/ENVIRONMENT.md index 00492829bd..dc6a7b4b17 100644 --- a/docs/ENVIRONMENT.md +++ b/docs/ENVIRONMENT.md @@ -630,6 +630,14 @@ SYSTEMD_HOME_DEBUG_SUFFIX=foo \ file (containing firmware measurement data) to read. This allows overriding the default of `/sys/kernel/security/tpm0/binary_bios_measurements`. +`systemd-sleep`: + +* `$SYSTEMD_SLEEP_FREEZE_USER_SESSIONS` - Takes a boolean. When true (the default), + `user.slice` will be frozen during sleep. When false it will not be. We recommend + against using this variable, because it can lead to undesired behavior, especially + for systems that use home directory encryption and for + `systemd-suspend-then-hibernate.service`. + Tools using the Varlink protocol (such as `varlinkctl`) or sd-bus (such as `busctl`): diff --git a/man/systemd-suspend.service.xml b/man/systemd-suspend.service.xml index d8ea8f5f81..9fbca6193f 100644 --- a/man/systemd-suspend.service.xml +++ b/man/systemd-suspend.service.xml @@ -66,7 +66,9 @@ same executables are run, but the first argument is now post. All executables in this directory are executed in parallel, and execution of the action is not continued - until all executables have finished. + until all executables have finished. Note that user.slice will + be frozen while the executables are running, so they should not attempt to + communicate with any user services expecting a reply. Note that scripts or binaries dropped in /usr/lib/systemd/system-sleep/ are intended @@ -90,6 +92,11 @@ sleep.conf.d file. See systemd-sleep.conf5. + + Note that by default these services freeze user.slice while they run. This prevents + the execution of any process in any of the user sessions while the system is entering into and resuming from + sleep. Thus, this prevents the hooks in /usr/lib/systemd/system-sleep/, or any other process + for that matter, from communicating with any user session process during sleep. diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index f1b6f1bcdc..16ad48b386 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -23,10 +23,12 @@ #include "build.h" #include "bus-error.h" #include "bus-locator.h" +#include "bus-unit-util.h" #include "bus-util.h" #include "constants.h" #include "devnum-util.h" #include "efivars.h" +#include "env-util.h" #include "exec-util.h" #include "fd-util.h" #include "fileio.h" @@ -444,38 +446,11 @@ static int custom_timer_suspend(const SleepConfig *sleep_config) { return 1; } -/* Freeze when invoked and thaw on cleanup */ -static int freeze_thaw_user_slice(const char **method) { - _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; - int r; - - if (!method || !*method) - return 0; - - 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_TIMEOUT); - - r = bus_call_method(bus, bus_systemd_mgr, *method, &error, NULL, "s", SPECIAL_USER_SLICE); - if (r < 0) - return log_debug_errno(r, "Failed to execute operation: %s", bus_error_message(&error, r)); - - return 1; -} - static int execute_s2h(const SleepConfig *sleep_config) { - _unused_ _cleanup_(freeze_thaw_user_slice) const char *auto_method_thaw = "ThawUnit"; int r; assert(sleep_config); - r = freeze_thaw_user_slice(&(const char*) { "FreezeUnit" }); - if (r < 0) - log_warning_errno(r, "Failed to freeze unit user.slice, ignoring: %m"); - /* Only check if we have automated battery alarms if HibernateDelaySec= is not set, as in that case * we'll busy poll for the configured interval instead */ if (!timestamp_is_set(sleep_config->hibernate_delay_usec)) { @@ -599,6 +574,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_(sleep_config_freep) SleepConfig *sleep_config = NULL; int r; @@ -617,6 +593,22 @@ static int run(int argc, char *argv[]) { "Sleep operation \"%s\" is disabled by configuration, refusing.", sleep_operation_to_string(arg_operation)); + /* Freeze the user sessions */ + 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 + 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 sysupend-then-hibernate " + "operations or setups with encrypted home directories."); + switch (arg_operation) { case SLEEP_SUSPEND_THEN_HIBERNATE: From a5b009d9355595b30c4da6f4499dd05f6e23ba8c Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Sat, 23 Dec 2023 18:00:48 -0500 Subject: [PATCH 3/4] homework: Lock/Unlock: Freeze/Thaw user session Whenever a home directory is in a locked state, accessing the files of the home directory is extremely likely to cause the thread to hang. This will put the session in a strange state, where some threads are hanging due to file access and others are not hanging because they are not trying to access any of the user's files. This can lead to a whole slew of consequences. For example, imagine a likely situation where the Wayland compositor is not hanging, but the user's open apps are. Eventually, the compositor will detect that none of the apps are responding to its pings, assume that they're frozen (which they are), and kill them. The systemd user instance can end up in a similarly confused state and start killing user services. In the worst case, killing an app at an unexpected moment can lead to data loss. The solution is to suspend execution of the whole user session by freezing the user's slice. --- docs/ENVIRONMENT.md | 8 ++++++++ src/home/homework.c | 48 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/docs/ENVIRONMENT.md b/docs/ENVIRONMENT.md index dc6a7b4b17..d2fb74dd96 100644 --- a/docs/ENVIRONMENT.md +++ b/docs/ENVIRONMENT.md @@ -557,6 +557,14 @@ SYSTEMD_HOME_DEBUG_SUFFIX=foo \ `mkfs` when formatting LUKS home directories. There's one variable for each of the supported file systems for the LUKS home directory backend. +* `$SYSTEMD_HOME_LOCK_FREEZE_SESSION` - Takes a boolean. When false, the user's + session will not be frozen when the home directory is locked. Note that the kernel + may still freeze any task that tries to access data from the user's locked home + directory. This can lead to data loss, security leaks, or other undesired behavior + caused by parts of the session becoming unresponsive due to disk I/O while other + parts of the session continue running. Thus, we highly recommend that this variable + isn't used unless necessary. Defaults to true. + `kernel-install`: * `$KERNEL_INSTALL_BYPASS` – If set to "1", execution of kernel-install is skipped diff --git a/src/home/homework.c b/src/home/homework.c index 39e0051486..531443e757 100644 --- a/src/home/homework.c +++ b/src/home/homework.c @@ -4,11 +4,14 @@ #include #include "blockdev-util.h" +#include "bus-unit-util.h" #include "chown-recursive.h" #include "copy.h" +#include "env-util.h" #include "fd-util.h" #include "fileio.h" #include "filesystems.h" +#include "format-util.h" #include "fs-util.h" #include "home-util.h" #include "homework-blob.h" @@ -1820,8 +1823,37 @@ static int home_inspect(UserRecord *h, UserRecord **ret_home) { return 1; } +static int user_session_freezer(uid_t uid, bool freeze_now, UnitFreezer *ret) { + _cleanup_free_ char *unit = NULL; + int r; + + 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."); + 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!"); + *ret = (UnitFreezer) {}; + return 0; + } + + if (asprintf(&unit, "user-" UID_FMT ".slice", uid) < 0) + return log_oom(); + + if (freeze_now) + r = unit_freezer_new_freeze(unit, ret); + else + 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); @@ -1837,16 +1869,27 @@ 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."); + r = home_lock_luks(h, &setup); if (r < 0) return r; + unit_freezer_done(&freezer); /* Don't thaw the user session. */ + log_info("Everything completed."); return 1; } 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; @@ -1868,6 +1911,11 @@ static int home_unlock(UserRecord *h) { if (r < 0) return r; + /* We want to thaw the session only after it's safe to access $HOME */ + r = user_session_freezer(h->uid, /* freeze_now= */ false, &freezer); + if (r < 0) + log_warning_errno(r, "Failed to recover freezer for user session, ignoring: %m"); + log_info("Everything completed."); return 1; } From 91448cf05015c2982318d9bb8ddeea8deeff430f Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Thu, 4 Jan 2024 14:08:35 -0500 Subject: [PATCH 4/4] NEWS: Add note about freezing user session changes These changes have the potential to break suspend on systems with proprietary NVIDIA drivers, so we should make a big NEWS entry about it --- NEWS | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/NEWS b/NEWS index cd9965aade..86b7ba7768 100644 --- a/NEWS +++ b/NEWS @@ -27,6 +27,15 @@ CHANGES WITH 256 in spe: mounted at some path, for example /boot/efi/ (this type of setup is obsolete but is still commonly found). + * The behavior of systemd-sleep and systemd-homed has been updated to + freeze user sessions when entering the various sleep modes or when + locking a homed-managed home area. This is known to cause issues with + the proprietary NVIDIA drivers. Packagers of the NVIDIA proprietary + drivers may want to add drop-in configuration files that set + SYSTEMD_SLEEP_FREEZE_USER_SESSION=false for systemd-suspend.service + and related services, and SYSTEMD_HOME_LOCK_FREEZE_SESSION=false for + systemd-homed.service. + Network Management: * systemd-networkd's proxy support gained a new option to configure