From 5409c6fcc55e6700360546c42edd4a021ee5014e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 27 Jan 2022 14:14:24 +0100 Subject: [PATCH 1/4] manager: do not ignore the return value from the main loop If manager_loop() fails, we would print an error message, but then actually ignore the error in main(), and potentially execute the shutdown binary. I'm not sure how likely this is to happen in practice, but it seems sloppy. So let's do the cleanup, but actually freeze() if manager_loop() returned an error. invoke_main_loop() is refactored to return the manager objective. This way we don't need to pass a separate parameter to specify whether we are reexecuting. Subsequent patch will make further use of the returned objective. --- src/core/main.c | 75 +++++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 36 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 57aedb9b93..ea2b9b7a52 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1955,7 +1955,6 @@ static int invoke_main_loop( Manager *m, const struct rlimit *saved_rlimit_nofile, const struct rlimit *saved_rlimit_memlock, - bool *ret_reexecute, int *ret_retval, /* Return parameters relevant for shutting down */ const char **ret_shutdown_verb, /* … */ FDSet **ret_fds, /* Return parameters for reexecuting */ @@ -1968,7 +1967,6 @@ static int invoke_main_loop( assert(m); assert(saved_rlimit_nofile); assert(saved_rlimit_memlock); - assert(ret_reexecute); assert(ret_retval); assert(ret_shutdown_verb); assert(ret_fds); @@ -1977,13 +1975,13 @@ static int invoke_main_loop( assert(ret_error_message); for (;;) { - r = manager_loop(m); - if (r < 0) { + int objective = manager_loop(m); + if (objective < 0) { *ret_error_message = "Failed to run main loop"; - return log_emergency_errno(r, "Failed to run main loop: %m"); + return log_emergency_errno(objective, "Failed to run main loop: %m"); } - switch ((ManagerObjective) r) { + switch (objective) { case MANAGER_RELOAD: { LogTarget saved_log_target; @@ -2010,17 +2008,15 @@ static int invoke_main_loop( if (saved_log_target >= 0) manager_override_log_target(m, saved_log_target); - r = manager_reload(m); - if (r < 0) + if (manager_reload(m) < 0) /* Reloading failed before the point of no return. * Let's continue running as if nothing happened. */ m->objective = MANAGER_OK; - break; + continue; } case MANAGER_REEXECUTE: - r = prepare_reexecute(m, &arg_serialization, ret_fds, false); if (r < 0) { *ret_error_message = "Failed to prepare for reexecution"; @@ -2029,12 +2025,11 @@ static int invoke_main_loop( log_notice("Reexecuting."); - *ret_reexecute = true; *ret_retval = EXIT_SUCCESS; *ret_shutdown_verb = NULL; *ret_switch_root_dir = *ret_switch_root_init = NULL; - return 0; + return objective; case MANAGER_SWITCH_ROOT: if (!m->switch_root_init) { @@ -2048,7 +2043,6 @@ static int invoke_main_loop( log_notice("Switching root."); - *ret_reexecute = true; *ret_retval = EXIT_SUCCESS; *ret_shutdown_verb = NULL; @@ -2056,20 +2050,18 @@ static int invoke_main_loop( *ret_switch_root_dir = TAKE_PTR(m->switch_root); *ret_switch_root_init = TAKE_PTR(m->switch_root_init); - return 0; + return objective; case MANAGER_EXIT: - if (MANAGER_IS_USER(m)) { log_debug("Exit."); - *ret_reexecute = false; *ret_retval = m->return_value; *ret_shutdown_verb = NULL; *ret_fds = NULL; *ret_switch_root_dir = *ret_switch_root_init = NULL; - return 0; + return objective; } _fallthrough_; @@ -2077,7 +2069,7 @@ static int invoke_main_loop( case MANAGER_POWEROFF: case MANAGER_HALT: case MANAGER_KEXEC: { - static const char * const table[_MANAGER_OBJECTIVE_MAX] = { + static const char* const table[_MANAGER_OBJECTIVE_MAX] = { [MANAGER_EXIT] = "exit", [MANAGER_REBOOT] = "reboot", [MANAGER_POWEROFF] = "poweroff", @@ -2087,13 +2079,12 @@ static int invoke_main_loop( log_notice("Shutting down."); - *ret_reexecute = false; *ret_retval = m->return_value; assert_se(*ret_shutdown_verb = table[m->objective]); *ret_fds = NULL; *ret_switch_root_dir = *ret_switch_root_init = NULL; - return 0; + return objective; } default: @@ -2709,15 +2700,18 @@ static int save_env(void) { } int main(int argc, char *argv[]) { - - dual_timestamp initrd_timestamp = DUAL_TIMESTAMP_NULL, userspace_timestamp = DUAL_TIMESTAMP_NULL, kernel_timestamp = DUAL_TIMESTAMP_NULL, - security_start_timestamp = DUAL_TIMESTAMP_NULL, security_finish_timestamp = DUAL_TIMESTAMP_NULL; + dual_timestamp + initrd_timestamp = DUAL_TIMESTAMP_NULL, + userspace_timestamp = DUAL_TIMESTAMP_NULL, + kernel_timestamp = DUAL_TIMESTAMP_NULL, + security_start_timestamp = DUAL_TIMESTAMP_NULL, + security_finish_timestamp = DUAL_TIMESTAMP_NULL; struct rlimit saved_rlimit_nofile = RLIMIT_MAKE_CONST(0), saved_rlimit_memlock = RLIMIT_MAKE_CONST(RLIM_INFINITY); /* The original rlimits we passed * in. Note we use different values * for the two that indicate whether * these fields are initialized! */ - bool skip_setup, loaded_policy = false, queue_default_job = false, first_boot = false, reexecute = false; + bool skip_setup, loaded_policy = false, queue_default_job = false, first_boot = false; char *switch_root_dir = NULL, *switch_root_init = NULL; usec_t before_startup, after_startup; static char systemd[] = "systemd"; @@ -3021,16 +3015,23 @@ int main(int argc, char *argv[]) { goto finish; } - (void) invoke_main_loop(m, - &saved_rlimit_nofile, - &saved_rlimit_memlock, - &reexecute, - &retval, - &shutdown_verb, - &fds, - &switch_root_dir, - &switch_root_init, - &error_message); + r = invoke_main_loop(m, + &saved_rlimit_nofile, + &saved_rlimit_memlock, + &retval, + &shutdown_verb, + &fds, + &switch_root_dir, + &switch_root_init, + &error_message); + assert(r < 0 || IN_SET(r, MANAGER_EXIT, /* MANAGER_OK is not expected here. */ + MANAGER_RELOAD, + MANAGER_REEXECUTE, + MANAGER_REBOOT, + MANAGER_POWEROFF, + MANAGER_HALT, + MANAGER_KEXEC, + MANAGER_SWITCH_ROOT)); finish: pager_close(); @@ -3043,7 +3044,7 @@ finish: mac_selinux_finish(); - if (reexecute) + if (IN_SET(r, MANAGER_REEXECUTE, MANAGER_SWITCH_ROOT)) do_reexecute(argc, argv, &saved_rlimit_nofile, &saved_rlimit_memlock, @@ -3076,7 +3077,9 @@ finish: __lsan_do_leak_check(); #endif - if (shutdown_verb) { + /* Try to invoke the shutdown binary unless we already failed. + * If we failed above, we want to freeze after finishing cleanup. */ + if (r >= 0 && shutdown_verb) { r = become_shutdown(shutdown_verb, retval); log_error_errno(r, "Failed to execute shutdown binary, %s: %m", getpid_cached() == 1 ? "freezing" : "quitting"); error_message = "Failed to execute shutdown binary"; From 1e3eee8cf0e654e101a4208b4cee7ecec65671a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 27 Jan 2022 15:03:27 +0100 Subject: [PATCH 2/4] manager: if we are reexecuting, do not invoke any fallbacks For https://bugzilla.redhat.com/show_bug.cgi?id=1986176: if we are trying to reexecute, and this fails for any reason, we shouldn't try to execute /sbin/init or /bin/sh. It is better to just freeze. If we freeze it is easier to diagnose what happened, but if we execute one of the fallbacks, we don't really know what will happen. In particular the new init might just return, causing the machine to shut down. Or we may successfully spawn /bin/sh, which could leave the machine open. --- src/core/main.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index ea2b9b7a52..93cbf62769 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1824,7 +1824,8 @@ static void filter_args( } } -static void do_reexecute( +static int do_reexecute( + ManagerObjective objective, int argc, char* argv[], const struct rlimit *saved_rlimit_nofile, @@ -1838,6 +1839,7 @@ static void do_reexecute( const char **args; int r; + assert(IN_SET(objective, MANAGER_REEXECUTE, MANAGER_SWITCH_ROOT)); assert(argc >= 0); assert(saved_rlimit_nofile); assert(saved_rlimit_memlock); @@ -1901,6 +1903,12 @@ static void do_reexecute( args[0] = SYSTEMD_BINARY_PATH; (void) execv(args[0], (char* const*) args); + + if (objective == MANAGER_REEXECUTE) { + *ret_error_message = "Failed to execute our own binary"; + return log_error_errno(errno, "Failed to execute our own binary %s: %m", args[0]); + } + log_debug_errno(errno, "Failed to execute our own binary %s, trying fallback: %m", args[0]); } @@ -1938,17 +1946,16 @@ static void do_reexecute( ANSI_HIGHLIGHT_RED " !! " ANSI_NORMAL, "Failed to execute /sbin/init"); + *ret_error_message = "Failed to execute fallback shell"; if (r == -ENOENT) { log_warning("No /sbin/init, trying fallback"); args[0] = "/bin/sh"; args[1] = NULL; (void) execve(args[0], (char* const*) args, saved_env); - log_error_errno(errno, "Failed to execute /bin/sh, giving up: %m"); + return log_error_errno(errno, "Failed to execute /bin/sh, giving up: %m"); } else - log_warning_errno(r, "Failed to execute /sbin/init, giving up: %m"); - - *ret_error_message = "Failed to execute fallback shell"; + return log_warning_errno(r, "Failed to execute /sbin/init, giving up: %m"); } static int invoke_main_loop( @@ -3045,13 +3052,14 @@ finish: mac_selinux_finish(); if (IN_SET(r, MANAGER_REEXECUTE, MANAGER_SWITCH_ROOT)) - do_reexecute(argc, argv, - &saved_rlimit_nofile, - &saved_rlimit_memlock, - fds, - switch_root_dir, - switch_root_init, - &error_message); /* This only returns if reexecution failed */ + r = do_reexecute(r, + argc, argv, + &saved_rlimit_nofile, + &saved_rlimit_memlock, + fds, + switch_root_dir, + switch_root_init, + &error_message); /* This only returns if reexecution failed */ arg_serialization = safe_fclose(arg_serialization); fds = fdset_free(fds); From 6b1fa539979e56c558fecaec044b307d22e7d391 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 15 Feb 2022 16:23:04 +0100 Subject: [PATCH 3/4] manager: add few ", ignoring" and adjust level in one message --- src/core/main.c | 8 ++++---- test/units/testsuite-36.sh | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 93cbf62769..754a847541 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1755,7 +1755,7 @@ static void update_cpu_affinity(bool skip_setup) { log_debug("Setting CPU affinity to %s.", strnull(mask)); if (sched_setaffinity(0, arg_cpu_affinity.allocated, arg_cpu_affinity.set) < 0) - log_warning_errno(errno, "Failed to set CPU affinity: %m"); + log_warning_errno(errno, "Failed to set CPU affinity, ignoring: %m"); } static void update_numa_policy(bool skip_setup) { @@ -1776,7 +1776,7 @@ static void update_numa_policy(bool skip_setup) { if (r == -EOPNOTSUPP) log_debug_errno(r, "NUMA support not available, ignoring."); else if (r < 0) - log_warning_errno(r, "Failed to set NUMA memory policy: %m"); + log_warning_errno(r, "Failed to set NUMA memory policy, ignoring: %m"); } static void filter_args( @@ -1955,7 +1955,7 @@ static int do_reexecute( (void) execve(args[0], (char* const*) args, saved_env); return log_error_errno(errno, "Failed to execute /bin/sh, giving up: %m"); } else - return log_warning_errno(r, "Failed to execute /sbin/init, giving up: %m"); + return log_error_errno(r, "Failed to execute /sbin/init, giving up: %m"); } static int invoke_main_loop( @@ -2257,7 +2257,7 @@ static int initialize_runtime( if (!arg_system) /* Become reaper of our children */ if (prctl(PR_SET_CHILD_SUBREAPER, 1) < 0) - log_warning_errno(errno, "Failed to make us a subreaper: %m"); + log_warning_errno(errno, "Failed to make us a subreaper, ignoring: %m"); /* Bump up RLIMIT_NOFILE for systemd itself */ (void) bump_rlimit_nofile(saved_rlimit_nofile); diff --git a/test/units/testsuite-36.sh b/test/units/testsuite-36.sh index b6c00c4845..f9dfd0810c 100755 --- a/test/units/testsuite-36.sh +++ b/test/units/testsuite-36.sh @@ -181,7 +181,7 @@ else echo "PID1 NUMAPolicy support - Bind policy w/o mask" writePID1NUMAPolicy "bind" pid1ReloadWithJournal - grep "Failed to set NUMA memory policy: Invalid argument" "$journalLog" + grep "Failed to set NUMA memory policy, ignoring: Invalid argument" "$journalLog" echo "PID1 NUMAPolicy support - Bind policy w/ mask" writePID1NUMAPolicy "bind" "0" @@ -191,7 +191,7 @@ else echo "PID1 NUMAPolicy support - Interleave policy w/o mask" writePID1NUMAPolicy "interleave" pid1ReloadWithJournal - grep "Failed to set NUMA memory policy: Invalid argument" "$journalLog" + grep "Failed to set NUMA memory policy, ignoring: Invalid argument" "$journalLog" echo "PID1 NUMAPolicy support - Interleave policy w/ mask" writePID1NUMAPolicy "interleave" "0" @@ -202,7 +202,7 @@ else writePID1NUMAPolicy "preferred" pid1ReloadWithJournal # Preferred policy with empty node mask is actually allowed and should reset allocation policy to default - grep "Failed to set NUMA memory policy: Invalid argument" "$journalLog" && { echo >&2 "unexpected pass"; exit 1; } + grep "Failed to set NUMA memory policy, ignoring: Invalid argument" "$journalLog" && { echo >&2 "unexpected pass"; exit 1; } echo "PID1 NUMAPolicy support - Preferred policy w/ mask" writePID1NUMAPolicy "preferred" "0" From 667030bff6493768dc16f3b057bdd1cac3c2b1cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 16 Feb 2022 08:07:02 +0100 Subject: [PATCH 4/4] manager: add {} around cpu sets, use range formatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We would print "Setting NUMA policy to bind, with nodes .". This is not very clear, change it to "… with nodes {}.". Also use range formatting for masks to make output shorter. --- src/core/main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 754a847541..69960c1ada 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1751,8 +1751,8 @@ static void update_cpu_affinity(bool skip_setup) { assert(arg_cpu_affinity.allocated > 0); - mask = cpu_set_to_string(&arg_cpu_affinity); - log_debug("Setting CPU affinity to %s.", strnull(mask)); + mask = cpu_set_to_range_string(&arg_cpu_affinity); + log_debug("Setting CPU affinity to {%s}.", strnull(mask)); if (sched_setaffinity(0, arg_cpu_affinity.allocated, arg_cpu_affinity.set) < 0) log_warning_errno(errno, "Failed to set CPU affinity, ignoring: %m"); @@ -1769,7 +1769,7 @@ static void update_numa_policy(bool skip_setup) { if (DEBUG_LOGGING) { policy = mpol_to_string(numa_policy_get_type(&arg_numa_policy)); nodes = cpu_set_to_range_string(&arg_numa_policy.nodes); - log_debug("Setting NUMA policy to %s, with nodes %s.", strnull(policy), strnull(nodes)); + log_debug("Setting NUMA policy to %s, with nodes {%s}.", strnull(policy), strnull(nodes)); } r = apply_numa_policy(&arg_numa_policy);