From cbc373502f1d6e2df01e43538006e31975f555ec Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Wed, 23 Aug 2017 15:58:03 +0100 Subject: [PATCH 1/6] logind: remember to remove '/run/systemd/shutdown/scheduled' --- src/login/logind-dbus.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index cd22ff4fca..0f8d0205ae 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -1496,6 +1496,7 @@ static void reset_scheduled_shutdown(Manager *m) { (void) unlink("/run/nologin"); m->unlink_nologin = false; } + (void) unlink("/run/systemd/shutdown/scheduled"); } static int execute_shutdown_or_sleep( From f8169e62dfc9ea903bc880064a5d384cdc68db3e Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Tue, 22 Aug 2017 13:54:21 +0100 Subject: [PATCH 2/6] logind: method_schedule_shutdown() already rejects empty `type` Don't test for an empty `type` afterwards. This is not how you cancel scheduled shutdowns - there's a separate method for that. --- src/login/logind-dbus.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 0f8d0205ae..3e18064e19 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -2051,12 +2051,9 @@ static int method_schedule_shutdown(sd_bus_message *message, void *userdata, sd_ if (r < 0) return r; - if (!isempty(type)) { - r = update_schedule_file(m); - if (r < 0) - return r; - } else - (void) unlink("/run/systemd/shutdown/scheduled"); + r = update_schedule_file(m); + if (r < 0) + return r; return sd_bus_reply_method_return(message, NULL); } From b498d6ea9f72520c579035928d16c527d992bca8 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Mon, 21 Aug 2017 11:49:25 +0100 Subject: [PATCH 3/6] logind: add missing check for conflicting operation v.s. scheduled shutdown > We don't want to shutdown while a suspend is running, and vice versa. > This would be confusing and could lead to data loss in the worst case. https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1441253/comments/4 According to the above comment, if the conflicting operation is hung, we don't want to force things when the admin has not passed a force option. Similarly if you're not an admin, you probably shouldn't get to sneak around this check by using a scheduled shutdown instead of an unscheduled one. (And no-one so far thought it necessary to add such a permission in PolKit). Note that if the conflicting operation was _not_ hung, and we lost the race with suspend, the system might not have shut down at the scheduled time anyway. Which is no good if you were scheduling a power outage. And scheduling a shutdown for an arbitrary time when the system is resumed, does not seem a very useful semantic. More likely, scheduled shutdowns are useful on systems which do not use suspend, such as multi-user servers. (In which case even PolKit defaults likely don't let the users trigger suspend). --- src/login/logind-dbus.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 3e18064e19..e08500e040 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -1946,9 +1946,15 @@ static int manager_scheduled_shutdown_handler( else target = SPECIAL_REBOOT_TARGET; - r = execute_shutdown_or_sleep(m, 0, target, &error); + /* Don't allow multiple jobs being executed at the same time */ + if (m->action_what) { + log_error("Scheduled shutdown to %s failed: shutdown or sleep operation already in progress", target); + return -EALREADY; + } + + r = execute_shutdown_or_sleep(m, INHIBIT_SHUTDOWN, target, &error); if (r < 0) - return log_error_errno(r, "Unable to execute transition to %s: %m", target); + return log_error_errno(r, "Scheduled shutdown to %s failed: %m", target); return 0; } From df75a1a8aa5420335a56093077fa8cfcbfffac78 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Mon, 21 Aug 2017 17:28:35 +0100 Subject: [PATCH 4/6] logind: respect "delay" inhibitors in scheduled shutdowns There is no justification not to wait an extra (default) 5 seconds, for a more graceful shutdown of user programs. Again, you don't get to ignore delay inhibitors for unscheduled shutdowns, short of `systemctl poweroff -f`. It is simplest if we move the test for `m->shutdown_dry_run` into manager_scheduled_shutdown_handler(). However we need to not add such delays during a "dry run". Otherwise, we would still have to be considered "in progress" for some seconds after our admin has seen the final wall message. If they go to `poweroff`, we would have blocked them with a misleading error message. Note this `poweroff` will still process delay inhibitors as needed. If the admin planned to use a more forceful method... eh. It's their responsibility to assess whether that's safe. There is an argument that the alternative behaviour could be used (racily!) to kludge around them not being able to shutdown to "single user mode". If we cared about that case, we would have easily preserved non-racy support for it in `shutdown`. Additionally, though I think this code does read more easily by reducing inconsistencies, we didn't come up with any use case for delay inhibitors v.s. shutdown.[1] The SIGTERM v.s. SIGKILL delay is more general, and we allow a whole 90 seconds for it, not just 5. So I don't think keeping this approach bears a risk of significant damage. [1] https://www.freedesktop.org/wiki/Software/systemd/inhibit/ --- src/login/logind-dbus.c | 91 ++++++++++++++++++++++------------------- 1 file changed, 48 insertions(+), 43 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index e08500e040..6793274e6e 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -1399,7 +1399,6 @@ static int have_multiple_sessions( static int bus_manager_log_shutdown( Manager *m, - InhibitWhat w, const char *unit_name) { const char *p, *q; @@ -1407,9 +1406,6 @@ static int bus_manager_log_shutdown( assert(m); assert(unit_name); - if (w != INHIBIT_SHUTDOWN) - return 0; - if (streq(unit_name, SPECIAL_POWEROFF_TARGET)) { p = "MESSAGE=System is powering down"; q = "SHUTDOWN=power-off"; @@ -1484,21 +1480,6 @@ int manager_set_lid_switch_ignore(Manager *m, usec_t until) { return r; } -static void reset_scheduled_shutdown(Manager *m) { - m->scheduled_shutdown_timeout_source = sd_event_source_unref(m->scheduled_shutdown_timeout_source); - m->wall_message_timeout_source = sd_event_source_unref(m->wall_message_timeout_source); - m->nologin_timeout_source = sd_event_source_unref(m->nologin_timeout_source); - m->scheduled_shutdown_type = mfree(m->scheduled_shutdown_type); - m->scheduled_shutdown_timeout = 0; - m->shutdown_dry_run = false; - - if (m->unlink_nologin) { - (void) unlink("/run/nologin"); - m->unlink_nologin = false; - } - (void) unlink("/run/systemd/shutdown/scheduled"); -} - static int execute_shutdown_or_sleep( Manager *m, InhibitWhat w, @@ -1515,32 +1496,28 @@ static int execute_shutdown_or_sleep( assert(w < _INHIBIT_WHAT_MAX); assert(unit_name); - bus_manager_log_shutdown(m, w, unit_name); + if (w == INHIBIT_SHUTDOWN) + bus_manager_log_shutdown(m, unit_name); - if (m->shutdown_dry_run) { - log_info("Running in dry run, suppressing action."); - reset_scheduled_shutdown(m); - } else { - r = sd_bus_call_method( - m->bus, - "org.freedesktop.systemd1", - "/org/freedesktop/systemd1", - "org.freedesktop.systemd1.Manager", - "StartUnit", - error, - &reply, - "ss", unit_name, "replace-irreversibly"); - if (r < 0) - return r; + r = sd_bus_call_method( + m->bus, + "org.freedesktop.systemd1", + "/org/freedesktop/systemd1", + "org.freedesktop.systemd1.Manager", + "StartUnit", + error, + &reply, + "ss", unit_name, "replace-irreversibly"); + if (r < 0) + return r; - r = sd_bus_message_read(reply, "o", &p); - if (r < 0) - return r; + r = sd_bus_message_read(reply, "o", &p); + if (r < 0) + return r; - c = strdup(p); - if (!c) - return -ENOMEM; - } + c = strdup(p); + if (!c) + return -ENOMEM; m->action_unit = unit_name; free(m->action_job); @@ -1924,6 +1901,21 @@ fail: return log_error_errno(r, "Failed to write information about scheduled shutdowns: %m"); } +static void reset_scheduled_shutdown(Manager *m) { + m->scheduled_shutdown_timeout_source = sd_event_source_unref(m->scheduled_shutdown_timeout_source); + m->wall_message_timeout_source = sd_event_source_unref(m->wall_message_timeout_source); + m->nologin_timeout_source = sd_event_source_unref(m->nologin_timeout_source); + m->scheduled_shutdown_type = mfree(m->scheduled_shutdown_type); + m->scheduled_shutdown_timeout = 0; + m->shutdown_dry_run = false; + + if (m->unlink_nologin) { + (void) unlink("/run/nologin"); + m->unlink_nologin = false; + } + (void) unlink("/run/systemd/shutdown/scheduled"); +} + static int manager_scheduled_shutdown_handler( sd_event_source *s, uint64_t usec, @@ -1952,7 +1944,20 @@ static int manager_scheduled_shutdown_handler( return -EALREADY; } - r = execute_shutdown_or_sleep(m, INHIBIT_SHUTDOWN, target, &error); + if (m->shutdown_dry_run) { + /* We do not process delay inhibitors here. Otherwise, we + * would have to be considered "in progress" (like the check + * above) for some seconds after our admin has seen the final + * wall message. */ + + bus_manager_log_shutdown(m, target); + log_info("Running in dry run, suppressing action."); + reset_scheduled_shutdown(m); + + return 0; + } + + r = bus_manager_shutdown_or_sleep_now_or_later(m, target, INHIBIT_SHUTDOWN, &error); if (r < 0) return log_error_errno(r, "Scheduled shutdown to %s failed: %m", target); From 6d7f7fd49f8a094fc36e750de1e80afea80c8228 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Thu, 24 Aug 2017 10:33:24 +0100 Subject: [PATCH 5/6] logind: add missing resume signal when we fail to initiate sleep/shutdown This fixed https://bugzilla.redhat.com/show_bug.cgi?id=1476313 as much as I was able to reproduce it in a VM, at least. E.g. this signal might wake the screen back up, providing a more visible indicator of suspend failure. In my VM testing, it was also required in order to unblock keyboard input in gnome-shell after the failed suspend. At the same time, fix the error handling for scheduled shutdowns. This now mirrors the behaviour of when you use `shutdown -k` - it sends all the scary messages about shutting down, "but you'll have to do it [shut down the system] yourself". It also avoids the risk of locking out the admin (nologin file), in case they logged out for some reason (and they use `sudo` instead of root). Not that I have any idea why you'd want to use `shutdown -k`, but the code is easier to analyze if it rolls back on error (in the absence of any code comment as to why that's not wanted). --- src/login/logind-dbus.c | 76 +++++++++++++++++++++++++---------------- 1 file changed, 46 insertions(+), 30 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 6793274e6e..77dd278bdd 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -1480,6 +1480,28 @@ int manager_set_lid_switch_ignore(Manager *m, usec_t until) { return r; } +static int send_prepare_for(Manager *m, InhibitWhat w, bool _active) { + + static const char * const signal_name[_INHIBIT_WHAT_MAX] = { + [INHIBIT_SHUTDOWN] = "PrepareForShutdown", + [INHIBIT_SLEEP] = "PrepareForSleep" + }; + + int active = _active; + + assert(m); + assert(w >= 0); + assert(w < _INHIBIT_WHAT_MAX); + assert(signal_name[w]); + + return sd_bus_emit_signal(m->bus, + "/org/freedesktop/login1", + "org.freedesktop.login1.Manager", + signal_name[w], + "b", + active); +} + static int execute_shutdown_or_sleep( Manager *m, InhibitWhat w, @@ -1509,15 +1531,17 @@ static int execute_shutdown_or_sleep( &reply, "ss", unit_name, "replace-irreversibly"); if (r < 0) - return r; + goto error; r = sd_bus_message_read(reply, "o", &p); if (r < 0) - return r; + goto error; c = strdup(p); - if (!c) - return -ENOMEM; + if (!c) { + r = -ENOMEM; + goto error; + } m->action_unit = unit_name; free(m->action_job); @@ -1528,6 +1552,12 @@ static int execute_shutdown_or_sleep( manager_set_lid_switch_ignore(m, now(CLOCK_MONOTONIC) + m->holdoff_timeout_usec); return 0; + +error: + /* Tell people that they now may take a lock again */ + send_prepare_for(m, m->action_what, false); + + return r; } int manager_dispatch_delayed(Manager *manager, bool timeout) { @@ -1558,7 +1588,8 @@ int manager_dispatch_delayed(Manager *manager, bool timeout) { /* Actually do the operation */ r = execute_shutdown_or_sleep(manager, manager->action_what, manager->action_unit, &error); if (r < 0) { - log_warning("Failed to send delayed message: %s", bus_error_message(&error, r)); + log_warning("Error during inhibitor-delayed operation (already returned success to client): %s", + bus_error_message(&error, r)); manager->action_unit = NULL; manager->action_what = 0; @@ -1619,28 +1650,6 @@ static int delay_shutdown_or_sleep( return 0; } -static int send_prepare_for(Manager *m, InhibitWhat w, bool _active) { - - static const char * const signal_name[_INHIBIT_WHAT_MAX] = { - [INHIBIT_SHUTDOWN] = "PrepareForShutdown", - [INHIBIT_SLEEP] = "PrepareForSleep" - }; - - int active = _active; - - assert(m); - assert(w >= 0); - assert(w < _INHIBIT_WHAT_MAX); - assert(signal_name[w]); - - return sd_bus_emit_signal(m->bus, - "/org/freedesktop/login1", - "org.freedesktop.login1.Manager", - signal_name[w], - "b", - active); -} - int bus_manager_shutdown_or_sleep_now_or_later( Manager *m, const char *unit_name, @@ -1940,8 +1949,9 @@ static int manager_scheduled_shutdown_handler( /* Don't allow multiple jobs being executed at the same time */ if (m->action_what) { + r = -EALREADY; log_error("Scheduled shutdown to %s failed: shutdown or sleep operation already in progress", target); - return -EALREADY; + goto error; } if (m->shutdown_dry_run) { @@ -1958,10 +1968,16 @@ static int manager_scheduled_shutdown_handler( } r = bus_manager_shutdown_or_sleep_now_or_later(m, target, INHIBIT_SHUTDOWN, &error); - if (r < 0) - return log_error_errno(r, "Scheduled shutdown to %s failed: %m", target); + if (r < 0) { + log_error_errno(r, "Scheduled shutdown to %s failed: %m", target); + goto error; + } return 0; + +error: + reset_scheduled_shutdown(m); + return r; } static int method_schedule_shutdown(sd_bus_message *message, void *userdata, sd_bus_error *error) { From b61fa4e00166b2bf593e0f6edbb277543f03915f Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Thu, 24 Aug 2017 15:21:21 +0100 Subject: [PATCH 6/6] logind: tighten assertion in execute_shutdown_or_sleep() Following commit b498d6ea, I belated realized we should tighten the assertions as well, to make sure that we're setting `m->action_what` to represent an action in progress. (The check for an action in progress is to compare `m->action_what` to zero) --- src/login/logind-dbus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 77dd278bdd..d66022f802 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -1514,7 +1514,7 @@ static int execute_shutdown_or_sleep( int r; assert(m); - assert(w >= 0); + assert(w > 0); assert(w < _INHIBIT_WHAT_MAX); assert(unit_name); @@ -1661,7 +1661,7 @@ int bus_manager_shutdown_or_sleep_now_or_later( assert(m); assert(unit_name); - assert(w >= 0); + assert(w > 0); assert(w <= _INHIBIT_WHAT_MAX); assert(!m->action_job);