From a7e709567ee44c8f930c9b3ae650c74b9a98a645 Mon Sep 17 00:00:00 2001 From: Kelvin Fan Date: Wed, 2 Jun 2021 21:13:03 +0000 Subject: [PATCH] daemon: Respect systemd inhibitor locks Today, non-interactive processes, including rpm-ostree, are not inhibited by systemd inhibitor locks. systemd 248 added the `--check-inhibitors` option to `systemctl` commands, which can be used to make non-interactive proccesses check for shutdown/sleep inhibitors. However, it also checks for `delay` mode inhibitors, which isn't what we want. Replicate this logic prior to reboots, but ignore `delay` mode inhibitors. --- src/daemon/rpmostreed-daemon.cxx | 6 +++ src/daemon/rpmostreed-daemon.h | 1 + src/daemon/rpmostreed-transaction-types.cxx | 36 ++++++++++--- src/daemon/rpmostreed-utils.cxx | 58 +++++++++++++++++++++ src/daemon/rpmostreed-utils.h | 3 ++ tests/common/libvm.sh | 3 -- tests/vmcheck/test-misc-2.sh | 14 +++-- 7 files changed, 109 insertions(+), 12 deletions(-) diff --git a/src/daemon/rpmostreed-daemon.cxx b/src/daemon/rpmostreed-daemon.cxx index 439ca45fd3..5558c127b2 100644 --- a/src/daemon/rpmostreed-daemon.cxx +++ b/src/daemon/rpmostreed-daemon.cxx @@ -815,6 +815,12 @@ rpmostreed_daemon_is_rebooting (RpmostreedDaemon *self) return self->rebooting; } +GDBusConnection * +rpmostreed_daemon_connection(void) +{ + g_assert (_daemon_instance->connection); + return _daemon_instance->connection; +} void rpmostreed_daemon_run_until_idle_exit (RpmostreedDaemon *self) diff --git a/src/daemon/rpmostreed-daemon.h b/src/daemon/rpmostreed-daemon.h index 04182a8a94..269d40d9d8 100644 --- a/src/daemon/rpmostreed-daemon.h +++ b/src/daemon/rpmostreed-daemon.h @@ -38,6 +38,7 @@ G_BEGIN_DECLS GType rpmostreed_daemon_get_type (void) G_GNUC_CONST; RpmostreedDaemon * rpmostreed_daemon_get (void); +GDBusConnection * rpmostreed_daemon_connection (void); gboolean rpmostreed_get_client_uid (RpmostreedDaemon *self, const char *client, uid_t *out_uid); diff --git a/src/daemon/rpmostreed-transaction-types.cxx b/src/daemon/rpmostreed-transaction-types.cxx index ae67a56ae9..5eb3a6df24 100644 --- a/src/daemon/rpmostreed-transaction-types.cxx +++ b/src/daemon/rpmostreed-transaction-types.cxx @@ -546,7 +546,11 @@ rollback_transaction_execute (RpmostreedTransaction *transaction, } if (self->reboot) - rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); + { + if (!check_sd_inhibitor_locks (cancellable, error)) + return FALSE; + rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); + } return TRUE; } @@ -1526,7 +1530,11 @@ deploy_transaction_execute (RpmostreedTransaction *transaction, } if (deploy_has_bool_option (self, "reboot")) - rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); + { + if (!check_sd_inhibitor_locks (cancellable, error)) + return FALSE; + rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); + } } else { @@ -1928,7 +1936,11 @@ initramfs_etc_transaction_execute (RpmostreedTransaction *transaction, return FALSE; if (vardict_lookup_bool (self->options, "reboot", FALSE)) - rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); + { + if (!check_sd_inhibitor_locks (cancellable, error)) + return FALSE; + rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); + } return TRUE; } @@ -2066,7 +2078,11 @@ initramfs_state_transaction_execute (RpmostreedTransaction *transaction, return FALSE; if (vardict_lookup_bool (self->options, "reboot", FALSE)) - rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); + { + if (!check_sd_inhibitor_locks (cancellable, error)) + return FALSE; + rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); + } return TRUE; } @@ -2581,7 +2597,7 @@ finalize_deployment_transaction_execute (RpmostreedTransaction *transaction, FinalizeDeploymentTransaction *self = (FinalizeDeploymentTransaction *) transaction; OstreeSysroot *sysroot = rpmostreed_transaction_get_sysroot (transaction); OstreeRepo *repo = ostree_sysroot_repo (sysroot); - + auto command_line = (const char*) vardict_lookup_ptr (self->options, "initiating-command-line", "&s"); @@ -2610,6 +2626,10 @@ finalize_deployment_transaction_execute (RpmostreedTransaction *transaction, return glnx_throw (error, "Expected staged base checksum %s, but found %s", expected_checksum, checksum); + // Check for inhibitor locks before unlocking staged deployment. + if (!check_sd_inhibitor_locks (cancellable, error)) + return FALSE; + if (unlink (_OSTREE_SYSROOT_RUNSTATE_STAGED_LOCKED) < 0) { if (errno != ENOENT) @@ -2802,7 +2822,11 @@ kernel_arg_transaction_execute (RpmostreedTransaction *transaction, return FALSE; if (vardict_lookup_bool (self->options, "reboot", FALSE)) - rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); + { + if (!check_sd_inhibitor_locks (cancellable, error)) + return FALSE; + rpmostreed_daemon_reboot (rpmostreed_daemon_get ()); + } return TRUE; } diff --git a/src/daemon/rpmostreed-utils.cxx b/src/daemon/rpmostreed-utils.cxx index ccfccd9f75..a9ca0caba5 100644 --- a/src/daemon/rpmostreed-utils.cxx +++ b/src/daemon/rpmostreed-utils.cxx @@ -21,6 +21,7 @@ #include "rpmostree-util.h" #include "rpmostreed-utils.h" #include "rpmostreed-errors.h" +#include "rpmostreed-daemon.h" #include "libglnx.h" #include #include @@ -609,3 +610,60 @@ rpmostreed_parse_revision (const char *revision, out: return ret; } + +/* Throw an error if there exists systemd inhibitor locks in `block` mode only. + * Note: systemd 248 provides a `--check-inhibitors` option, but it also checks + * for inhibitors in `delay` mode, which isn't what we want. */ +gboolean +check_sd_inhibitor_locks (GCancellable *cancellable, + GError **error) +{ + GDBusConnection *connection = rpmostreed_daemon_connection (); + // https://www.freedesktop.org/software/systemd/man/org.freedesktop.login1.html + g_autoptr(GVariant) inhibitors_array_tuple = + g_dbus_connection_call_sync (connection, "org.freedesktop.login1", "/org/freedesktop/login1", + "org.freedesktop.login1.Manager", "ListInhibitors", + NULL, (const GVariantType*)"(a(ssssuu))", + G_DBUS_CALL_FLAGS_NONE, -1, cancellable, error); + if (!inhibitors_array_tuple) + return glnx_prefix_error (error, "Checking systemd inhibitor locks"); + else if (g_variant_n_children (inhibitors_array_tuple) < 1) + return glnx_throw (error, "ListInhibitors returned empty tuple"); + g_autoptr(GVariant) inhibitors_array = + g_variant_get_child_value (inhibitors_array_tuple, 0); + + char *what = NULL; + char *who = NULL; + char *why = NULL; + char *mode = NULL; + int num_block_inhibitors = 0; + g_autoptr(GString) error_msg = g_string_new(NULL); + GVariantIter viter; + g_variant_iter_init (&viter, inhibitors_array); + while (g_variant_iter_loop (&viter, "(ssssuu)", &what, &who, &why, &mode, NULL, NULL)) + { + // Only consider shutdown inhibitors in `block` mode. + if (strstr (what, "shutdown") && g_str_equal (mode, "block")) + { + num_block_inhibitors++; + if (num_block_inhibitors == 1) + { + g_string_append_printf (error_msg, + "Reboot blocked by a systemd inhibitor lock in `block` mode\n" + "Held by: %s\nReason: %s", + who, why); + } + } + } + + if (num_block_inhibitors == 0) + return TRUE; + else if (num_block_inhibitors > 1) + { + g_string_append_printf (error_msg, + "\nand %d other blocking inhibitor lock(s)\n" + "Use `systemd-inhibit --list` to see details", + num_block_inhibitors - 1); + } + return glnx_throw (error, "%s", error_msg->str); +} diff --git a/src/daemon/rpmostreed-utils.h b/src/daemon/rpmostreed-utils.h index f4408cb5b4..115c7c3a3a 100644 --- a/src/daemon/rpmostreed-utils.h +++ b/src/daemon/rpmostreed-utils.h @@ -83,4 +83,7 @@ gboolean rpmostreed_parse_revision (const char *revision, char **out_version, GError **error); +gboolean check_sd_inhibitor_locks (GCancellable *cancellable, + GError **error); + G_END_DECLS \ No newline at end of file diff --git a/tests/common/libvm.sh b/tests/common/libvm.sh index 34620fa222..cc7c03fdd5 100644 --- a/tests/common/libvm.sh +++ b/tests/common/libvm.sh @@ -98,9 +98,6 @@ EOF vm_setup - # XXX: hack around https://github.com/systemd/systemd/issues/14328 - vm_cmd systemctl mask --now systemd-logind - # Some tests expect the ref to be on `vmcheck`. We should drop that # requirement, but for now let's just mangle the origin local deployment_root diff --git a/tests/vmcheck/test-misc-2.sh b/tests/vmcheck/test-misc-2.sh index 7d47431a1a..34ed060ec3 100755 --- a/tests/vmcheck/test-misc-2.sh +++ b/tests/vmcheck/test-misc-2.sh @@ -44,7 +44,7 @@ assert_streq "$(vm_get_booted_csum)" "${booted_csum}" vm_assert_journal_has_content $cursor 'Not finalizing; found /run/ostree/staged-deployment-locked' echo "ok locked rebase staging" -# This also now tests custom client IDs in the journal. +# This also tests custom client IDs in the journal and interaction with systemd inhibitor locks. cursor=$(vm_get_journal_cursor) vm_cmd env RPMOSTREE_CLIENT_ID=testing-agent-id \ rpm-ostree deploy revision="${commit}" \ @@ -58,7 +58,15 @@ fi vm_cmd journalctl --after-cursor "'$cursor'" -u rpm-ostreed -o json | jq -r '.AGENT//""' > agent.txt assert_file_has_content agent.txt testing-agent-id vm_cmd journalctl --after-cursor "'$cursor'" -u rpm-ostreed -o json | jq -r '.AGENT_SD_UNIT//""' > agent_sd_unit.txt -assert_file_has_content agent_sd_unit.txt sshd.service +assert_file_has_content agent_sd_unit.txt session-1.scope +vm_cmd "systemd-inhibit --what=shutdown --mode=block sh -c 'while ! test -f /run/wakeup; do sleep 0.1; done'" & +if vm_rpmostree finalize-deployment "${commit}" 2>err.txt; then + assert_not_reached "finalized with inhibitor lock in block mode present" +fi +assert_file_has_content err.txt 'Reboot blocked' +vm_cmd test -f /run/ostree/staged-deployment-locked # Make sure that staged deployment is still locked. +vm_cmd touch /run/wakeup +sleep 1 # Wait one second for the process holding the lock to exit. vm_reboot_cmd rpm-ostree finalize-deployment "${commit}" assert_streq "$(vm_get_booted_csum)" "${commit}" vm_assert_journal_has_content $cursor "Finalized deployment; rebooting into ${commit}" @@ -364,7 +372,7 @@ vm_cmd rpm-ostree deploy \'\' \ --register-driver=OtherTestDriver --bypass-driver # Make sure OtherTestDriver's systemd unit will be the same as the commandline's vm_cmd rpm-ostree status -v > verbose_status.txt -assert_file_has_content verbose_status.txt 'AutomaticUpdatesDriver: OtherTestDriver (sshd.service)' +assert_file_has_content verbose_status.txt 'AutomaticUpdatesDriver: OtherTestDriver (session-1.scope)' vm_rpmostree upgrade 2>err.txt assert_not_file_has_content err.txt 'Updates and deployments are driven by OtherTestDriver' vm_rpmostree cleanup -p