From 6524f1a8eb7c04b2eb4b2bc7b6bcda352e069d8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 10 Apr 2018 11:23:46 +0200 Subject: [PATCH 1/6] shared/sleep-config: get rid of explicit allocation size calculation --- src/shared/sleep-config.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index af1c2ced40..79cd90a744 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -274,11 +274,11 @@ static bool enough_memory_for_hibernation(void) { int read_fiemap(int fd, struct fiemap **ret) { _cleanup_free_ struct fiemap *fiemap = NULL, *result_fiemap = NULL; - int extents_size; struct stat statinfo; uint32_t result_extents = 0; uint64_t fiemap_start = 0, fiemap_length; - size_t fiemap_size = 1, result_fiemap_size = 1; + const size_t n_extra = DIV_ROUND_UP(sizeof(struct fiemap), sizeof(struct fiemap_extent)); + size_t fiemap_allocated = n_extra, result_fiemap_allocated = n_extra; if (fstat(fd, &statinfo) < 0) return log_debug_errno(errno, "Cannot determine file size: %m"); @@ -286,12 +286,12 @@ int read_fiemap(int fd, struct fiemap **ret) { return -ENOTTY; fiemap_length = statinfo.st_size; - /* zero this out in case we run on a file with no extents */ - fiemap = new0(struct fiemap, 1); + /* Zero this out in case we run on a file with no extents */ + fiemap = calloc(n_extra, sizeof(struct fiemap_extent)); if (!fiemap) return -ENOMEM; - result_fiemap = new(struct fiemap, 1); + result_fiemap = malloc_multiply(n_extra, sizeof(struct fiemap_extent)); if (!result_fiemap) return -ENOMEM; @@ -315,12 +315,10 @@ int read_fiemap(int fd, struct fiemap **ret) { if (fiemap->fm_mapped_extents == 0) break; - /* Result fiemap has to hold all the extents for the whole file */ - extents_size = DIV_ROUND_UP(sizeof(struct fiemap_extent) * fiemap->fm_mapped_extents, - sizeof(struct fiemap)); - - /* Resize fiemap to allow us to read in the extents */ - if (!GREEDY_REALLOC0(fiemap, fiemap_size, extents_size)) + /* Resize fiemap to allow us to read in the extents, result fiemap has to hold all + * the extents for the whole file. Add space for the initial struct fiemap. */ + if (!greedy_realloc0((void**) &fiemap, &fiemap_allocated, + n_extra + fiemap->fm_mapped_extents, sizeof(struct fiemap_extent))) return -ENOMEM; fiemap->fm_extent_count = fiemap->fm_mapped_extents; @@ -329,12 +327,9 @@ int read_fiemap(int fd, struct fiemap **ret) { if (ioctl(fd, FS_IOC_FIEMAP, fiemap) < 0) return log_debug_errno(errno, "Failed to read extents: %m"); - extents_size = DIV_ROUND_UP(sizeof(struct fiemap_extent) * (result_extents + fiemap->fm_mapped_extents), - sizeof(struct fiemap)); - - /* Resize result_fiemap to allow us to read in the extents */ - if (!GREEDY_REALLOC(result_fiemap, result_fiemap_size, - extents_size)) + /* Resize result_fiemap to allow us to copy in the extents */ + if (!greedy_realloc((void**) &result_fiemap, &result_fiemap_allocated, + n_extra + result_extents + fiemap->fm_mapped_extents, sizeof(struct fiemap_extent))) return -ENOMEM; memcpy(result_fiemap->fm_extents + result_extents, @@ -344,7 +339,7 @@ int read_fiemap(int fd, struct fiemap **ret) { result_extents += fiemap->fm_mapped_extents; /* Highly unlikely that it is zero */ - if (fiemap->fm_mapped_extents > 0) { + if (_likely_(fiemap->fm_mapped_extents > 0)) { uint32_t i = fiemap->fm_mapped_extents - 1; fiemap_start = fiemap->fm_extents[i].fe_logical + From c863dc05881fd5afbc3c7ce700980354530cee8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 10 Apr 2018 11:39:14 +0200 Subject: [PATCH 2/6] shared/sleep-fix: fix check if s-then-h is possible can_sleep() returns 0 if the operation is impossible, but the code assumed that negative is returned in that case, in effect reporting s2h was possible even if hibernation or suspend were not possible. --- src/shared/sleep-config.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index 79cd90a744..03fd497f52 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -357,6 +357,7 @@ int read_fiemap(int fd, struct fiemap **ret) { } static bool can_s2h(void) { + const char *p; int r; r = access("/sys/class/rtc/rtc0/wakealarm", W_OK); @@ -366,16 +367,14 @@ static bool can_s2h(void) { return false; } - r = can_sleep("suspend"); - if (r < 0) { - log_debug_errno(r, "Unable to suspend system."); - return false; - } - - r = can_sleep("hibernate"); - if (r < 0) { - log_debug_errno(r, "Unable to hibernate system."); - return false; + FOREACH_STRING(p, "suspend", "hibernate") { + r = can_sleep(p); + if (r < 0) + return log_debug_errno(r, "Failed to check if %s is possible: %m", p); + if (r == 0) { + log_debug("Unable to %s system.", p); + return false; + } } return true; From bd062910c844a3aa823e1651436d694266c248b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 10 Apr 2018 12:35:36 +0200 Subject: [PATCH 3/6] Move utility function to query unit state from systemctl to shared/ --- src/shared/bus-unit-util.c | 26 ++++++++++++++++++++++++++ src/shared/bus-unit-util.h | 2 ++ src/systemctl/systemctl.c | 18 +++--------------- 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index a9c17d29e2..24efd48ce7 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -2462,3 +2462,29 @@ finish: return r; } + +int unit_load_state(sd_bus *bus, const char *name, char **load_state) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + _cleanup_free_ char *path = NULL; + int r; + + path = unit_dbus_path_from_name(name); + if (!path) + return log_oom(); + + /* This function warns on it's own, because otherwise it'd be awkward to pass + * the dbus error message around. */ + + r = sd_bus_get_property_string( + bus, + "org.freedesktop.systemd1", + path, + "org.freedesktop.systemd1.Unit", + "LoadState", + &error, + load_state); + if (r < 0) + return log_error_errno(r, "Failed to get load state of %s: %s", name, bus_error_message(&error, r)); + + return 0; +} diff --git a/src/shared/bus-unit-util.h b/src/shared/bus-unit-util.h index 514e6edb76..704c526f3f 100644 --- a/src/shared/bus-unit-util.h +++ b/src/shared/bus-unit-util.h @@ -57,3 +57,5 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(BusWaitForJobs*, bus_wait_for_jobs_free); int bus_deserialize_and_dump_unit_file_changes(sd_bus_message *m, bool quiet, UnitFileChange **changes, unsigned *n_changes); int unit_show_processes(sd_bus *bus, const char *unit, const char *cgroup_path, const char *prefix, unsigned n_columns, OutputFlags flags, sd_bus_error *error); + +int unit_load_state(sd_bus *bus, const char *name, char **load_state); diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 16a28edaa2..9f63bbc77e 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -2649,24 +2649,12 @@ static int get_state_one_unit(sd_bus *bus, const char *name, UnitActiveState *ac } static int unit_is_masked(sd_bus *bus, const char *name) { - _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - _cleanup_free_ char *path = NULL, *load_state = NULL; + _cleanup_free_ char *load_state = NULL; int r; - path = unit_dbus_path_from_name(name); - if (!path) - return log_oom(); - - r = sd_bus_get_property_string( - bus, - "org.freedesktop.systemd1", - path, - "org.freedesktop.systemd1.Unit", - "LoadState", - &error, - &load_state); + r = unit_load_state(bus, name, &load_state); if (r < 0) - return log_error_errno(r, "Failed to get load state of %s: %s", name, bus_error_message(&error, r)); + return r; return streq(load_state, "masked"); } From c8c8ee85af35f1e9593a7397cfa289a126dfcb8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 10 Apr 2018 13:15:00 +0200 Subject: [PATCH 4/6] logind: refuse operations if the target unit is masked or unavailable If hibernate.target is masked, and systemctl hibernate is invoked, havoc ensues. logind starts the hibernation operation, but then doesn't go through with it; gnome-shell segfaults. Let's be nice to the user and refuse doing anything in that case. $ sudo systemctl mask hibernate.target $ busctl call org.freedesktop.login1 /org/freedesktop/login1 org.freedesktop.login1.Manager CanHibernate s "no" $ sudo systemctl hibernate Failed to hibernate system via logind: Access denied Failed to start hibernate.target: Unit hibernate.target is masked. https://bugzilla.redhat.com/show_bug.cgi?id=1468003#c4 --- src/login/logind-action.c | 37 +++++++++++++++++++++++-------------- src/login/logind-action.h | 1 + src/login/logind-dbus.c | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 14 deletions(-) diff --git a/src/login/logind-action.c b/src/login/logind-action.c index da38a2c145..1e40282004 100644 --- a/src/login/logind-action.c +++ b/src/login/logind-action.c @@ -33,6 +33,24 @@ #include "terminal-util.h" #include "user-util.h" +const char* manager_target_for_action(HandleAction handle) { + static const char * const target_table[_HANDLE_ACTION_MAX] = { + [HANDLE_POWEROFF] = SPECIAL_POWEROFF_TARGET, + [HANDLE_REBOOT] = SPECIAL_REBOOT_TARGET, + [HANDLE_HALT] = SPECIAL_HALT_TARGET, + [HANDLE_KEXEC] = SPECIAL_KEXEC_TARGET, + [HANDLE_SUSPEND] = SPECIAL_SUSPEND_TARGET, + [HANDLE_HIBERNATE] = SPECIAL_HIBERNATE_TARGET, + [HANDLE_HYBRID_SLEEP] = SPECIAL_HYBRID_SLEEP_TARGET, + [HANDLE_SUSPEND_THEN_HIBERNATE] = SPECIAL_SUSPEND_THEN_HIBERNATE_TARGET, + }; + + assert(handle >= 0); + if (handle < (ssize_t) ELEMENTSOF(target_table)) + return target_table[handle]; + return NULL; +} + int manager_handle_action( Manager *m, InhibitWhat inhibit_key, @@ -51,21 +69,11 @@ int manager_handle_action( [HANDLE_SUSPEND_THEN_HIBERNATE] = "Suspending, then hibernating...", }; - static const char * const target_table[_HANDLE_ACTION_MAX] = { - [HANDLE_POWEROFF] = SPECIAL_POWEROFF_TARGET, - [HANDLE_REBOOT] = SPECIAL_REBOOT_TARGET, - [HANDLE_HALT] = SPECIAL_HALT_TARGET, - [HANDLE_KEXEC] = SPECIAL_KEXEC_TARGET, - [HANDLE_SUSPEND] = SPECIAL_SUSPEND_TARGET, - [HANDLE_HIBERNATE] = SPECIAL_HIBERNATE_TARGET, - [HANDLE_HYBRID_SLEEP] = SPECIAL_HYBRID_SLEEP_TARGET, - [HANDLE_SUSPEND_THEN_HIBERNATE] = SPECIAL_SUSPEND_THEN_HIBERNATE_TARGET, - }; - _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; InhibitWhat inhibit_operation; Inhibitor *offending = NULL; bool supported; + const char *target; int r; assert(m); @@ -97,7 +105,6 @@ int manager_handle_action( /* Locking is handled differently from the rest. */ if (handle == HANDLE_LOCK) { - if (!is_edge) return 0; @@ -129,6 +136,8 @@ int manager_handle_action( return -EALREADY; } + assert_se(target = manager_target_for_action(handle)); + inhibit_operation = IN_SET(handle, HANDLE_SUSPEND, HANDLE_HIBERNATE, HANDLE_HYBRID_SLEEP, HANDLE_SUSPEND_THEN_HIBERNATE) ? INHIBIT_SLEEP : INHIBIT_SHUTDOWN; @@ -160,7 +169,7 @@ int manager_handle_action( log_info("%s", message_table[handle]); - r = bus_manager_shutdown_or_sleep_now_or_later(m, target_table[handle], inhibit_operation, &error); + r = bus_manager_shutdown_or_sleep_now_or_later(m, target, inhibit_operation, &error); if (r < 0) { log_error("Failed to execute operation: %s", bus_error_message(&error, r)); return r; @@ -179,7 +188,7 @@ static const char* const handle_action_table[_HANDLE_ACTION_MAX] = { [HANDLE_HIBERNATE] = "hibernate", [HANDLE_HYBRID_SLEEP] = "hybrid-sleep", [HANDLE_SUSPEND_THEN_HIBERNATE] = "suspend-then-hibernate", - [HANDLE_LOCK] = "lock" + [HANDLE_LOCK] = "lock", }; DEFINE_STRING_TABLE_LOOKUP(handle_action, HandleAction); diff --git a/src/login/logind-action.h b/src/login/logind-action.h index 9f5dee684c..d483e43478 100644 --- a/src/login/logind-action.h +++ b/src/login/logind-action.h @@ -48,4 +48,5 @@ int manager_handle_action( const char* handle_action_to_string(HandleAction h) _const_; HandleAction handle_action_from_string(const char *s) _pure_; +const char* manager_target_for_action(HandleAction handle); int config_parse_handle_action(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata); diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 94c8f1f142..acf05601a2 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -29,6 +29,7 @@ #include "audit-util.h" #include "bus-common-errors.h" #include "bus-error.h" +#include "bus-unit-util.h" #include "bus-util.h" #include "dirent-util.h" #include "efivars.h" @@ -1705,6 +1706,7 @@ int bus_manager_shutdown_or_sleep_now_or_later( InhibitWhat w, sd_bus_error *error) { + _cleanup_free_ char *load_state = NULL; bool delayed; int r; @@ -1714,6 +1716,15 @@ int bus_manager_shutdown_or_sleep_now_or_later( assert(w <= _INHIBIT_WHAT_MAX); assert(!m->action_job); + r = unit_load_state(m->bus, unit_name, &load_state); + if (r < 0) + return r; + + if (!streq(load_state, "loaded")) { + log_notice("Unit %s is %s, refusing operation.", unit_name, load_state); + return -EACCES; + } + /* Tell everybody to prepare for shutdown/sleep */ (void) send_prepare_for(m, w, true); @@ -2235,6 +2246,7 @@ static int method_can_shutdown_or_sleep( sd_bus_error *error) { _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; + HandleAction handle; bool multiple_sessions, challenge, blocked; const char *result = NULL; uid_t uid; @@ -2271,6 +2283,25 @@ static int method_can_shutdown_or_sleep( multiple_sessions = r > 0; blocked = manager_is_inhibited(m, w, INHIBIT_BLOCK, NULL, false, true, uid, NULL); + handle = handle_action_from_string(sleep_verb); + if (handle >= 0) { + const char *target; + + target = manager_target_for_action(handle); + if (target) { + _cleanup_free_ char *load_state = NULL; + + r = unit_load_state(m->bus, target, &load_state); + if (r < 0) + return r; + + if (!streq(load_state, "loaded")) { + result = "no"; + goto finish; + } + } + } + if (multiple_sessions) { r = bus_test_polkit(message, CAP_SYS_BOOT, action_multiple_sessions, NULL, UID_INVALID, &challenge, error); if (r < 0) @@ -2313,6 +2344,7 @@ static int method_can_shutdown_or_sleep( result = "no"; } + finish: return sd_bus_reply_method_return(message, "s", result); } From b71c9758d131e5f2b38fcbc6393579a4aab3cb82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 11 Apr 2018 08:51:06 +0200 Subject: [PATCH 5/6] shared/sleep-config: return a custom message when not enough swap for hibernation $ sudo swapoff -av swapoff /dev/vda4 $ sudo systemctl hibernate Failed to hibernate system via logind: Not enough swap space for hibernation Fixes #6729. --- src/login/logind-dbus.c | 13 ++++++++----- src/shared/sleep-config.c | 14 ++++++++++---- src/test/test-sleep.c | 15 +++++++++++---- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index acf05601a2..83f1aa76b9 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -1835,11 +1835,14 @@ static int method_do_shutdown_or_sleep( if (sleep_verb) { r = can_sleep(sleep_verb); + if (r == -ENOSPC) + return sd_bus_error_set(error, BUS_ERROR_SLEEP_VERB_NOT_SUPPORTED, + "Not enough swap space for hibernation"); + if (r == 0) + return sd_bus_error_setf(error, BUS_ERROR_SLEEP_VERB_NOT_SUPPORTED, + "Sleep verb \"%s\" not supported", sleep_verb); if (r < 0) return r; - - if (r == 0) - return sd_bus_error_setf(error, BUS_ERROR_SLEEP_VERB_NOT_SUPPORTED, "Sleep verb not supported"); } r = verify_shutdown_creds(m, message, w, interactive, action, action_multiple_sessions, @@ -2262,10 +2265,10 @@ static int method_can_shutdown_or_sleep( if (sleep_verb) { r = can_sleep(sleep_verb); + if (IN_SET(r, 0, -ENOSPC)) + return sd_bus_reply_method_return(message, "s", "na"); if (r < 0) return r; - if (r == 0) - return sd_bus_reply_method_return(message, "s", "na"); } r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_EUID, &creds); diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index 03fd497f52..1b684b90cb 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -369,12 +369,12 @@ static bool can_s2h(void) { FOREACH_STRING(p, "suspend", "hibernate") { r = can_sleep(p); - if (r < 0) - return log_debug_errno(r, "Failed to check if %s is possible: %m", p); - if (r == 0) { + if (IN_SET(r, 0, -ENOSPC)) { log_debug("Unable to %s system.", p); return false; } + if (r < 0) + return log_debug_errno(r, "Failed to check if %s is possible: %m", p); } return true; @@ -396,5 +396,11 @@ int can_sleep(const char *verb) { if (!can_sleep_state(states) || !can_sleep_disk(modes)) return false; - return streq(verb, "suspend") || enough_memory_for_hibernation(); + if (streq(verb, "suspend")) + return true; + + if (!enough_memory_for_hibernation()) + return -ENOSPC; + + return true; } diff --git a/src/test/test-sleep.c b/src/test/test-sleep.c index 05fd6c0abf..d9326c9474 100644 --- a/src/test/test-sleep.c +++ b/src/test/test-sleep.c @@ -65,7 +65,9 @@ static void test_sleep(void) { **platform = strv_new("platform", NULL), **shutdown = strv_new("shutdown", NULL), **freez = strv_new("freeze", NULL); + int r; + log_info("/* configuration */"); log_info("Standby configured: %s", yes_no(can_sleep_state(standby) > 0)); log_info("Suspend configured: %s", yes_no(can_sleep_state(mem) > 0)); log_info("Hibernate configured: %s", yes_no(can_sleep_state(disk) > 0)); @@ -75,10 +77,15 @@ static void test_sleep(void) { log_info("Hibernate+Shutdown configured: %s", yes_no(can_sleep_disk(shutdown) > 0)); log_info("Freeze configured: %s", yes_no(can_sleep_state(freez) > 0)); - log_info("Suspend configured and possible: %s", yes_no(can_sleep("suspend") > 0)); - log_info("Hibernation configured and possible: %s", yes_no(can_sleep("hibernate") > 0)); - log_info("Hybrid-sleep configured and possible: %s", yes_no(can_sleep("hybrid-sleep") > 0)); - log_info("Suspend-then-Hibernate configured and possible: %s", yes_no(can_sleep("suspend-then-hibernate") > 0)); + log_info("/* running system */"); + r = can_sleep("suspend"); + log_info("Suspend configured and possible: %s", r >= 0 ? yes_no(r) : strerror(-r)); + r = can_sleep("hibernate"); + log_info("Hibernation configured and possible: %s", r >= 0 ? yes_no(r) : strerror(-r)); + r = can_sleep("hybrid-sleep"); + log_info("Hybrid-sleep configured and possible: %s", r >= 0 ? yes_no(r) : strerror(-r)); + r = can_sleep("suspend-then-hibernate"); + log_info("Suspend-then-Hibernate configured and possible: %s", r >= 0 ? yes_no(r) : strerror(-r)); } int main(int argc, char* argv[]) { From 4638cd39af495d89014c31b449f62a2618c390f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 11 Apr 2018 09:27:32 +0200 Subject: [PATCH 6/6] shared/sleep-config: rename misnamed function --- src/shared/sleep-config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index 1b684b90cb..26703b3236 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -239,7 +239,7 @@ int find_hibernate_location(char **device, char **type, size_t *size, size_t *us return -ENOSYS; } -static bool enough_memory_for_hibernation(void) { +static bool enough_swap_for_hibernation(void) { _cleanup_free_ char *active = NULL; unsigned long long act = 0; size_t size = 0, used = 0; @@ -399,7 +399,7 @@ int can_sleep(const char *verb) { if (streq(verb, "suspend")) return true; - if (!enough_memory_for_hibernation()) + if (!enough_swap_for_hibernation()) return -ENOSPC; return true;