From 4dc63c4bc7850bc570fcf33800a444e09dec81d5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 16:13:44 +0100 Subject: [PATCH 01/25] main: minor optimization Let's remove one memory allocation in the common path. --- src/core/main.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 2ad5073368..1be7874428 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1413,6 +1413,7 @@ static int bump_unix_max_dgram_qlen(void) { static int fixup_environment(void) { _cleanup_free_ char *term = NULL; + const char *t; int r; /* We expect the environment to be set correctly @@ -1432,13 +1433,10 @@ static int fixup_environment(void) { r = proc_cmdline_get_key("TERM", 0, &term); if (r < 0) return r; - if (r == 0) { - term = strdup(default_term_for_tty("/dev/console")); - if (!term) - return -ENOMEM; - } - if (setenv("TERM", term, 1) < 0) + t = term ?: default_term_for_tty("/dev/console"); + + if (setenv("TERM", t, 1) < 0) return -errno; return 0; From 97d1fb94ba3b557abf9acbcd47acb11dbeb1239d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 16:14:19 +0100 Subject: [PATCH 02/25] main: split out all parsing of command line arguments/kernel arguments/configuration files Let's shorten main() a bit, and split out everything that loads our configuration and runtime parameters into a function of its own. No changes in behaviour. --- src/core/main.c | 72 ++++++++++++++++++++++++++++--------------------- 1 file changed, 42 insertions(+), 30 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 1be7874428..6196a6ce6c 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2051,6 +2051,46 @@ static void free_arguments(void) { arg_syscall_archs = set_free(arg_syscall_archs); } +static int load_configuration(int argc, char **argv, const char **ret_error_message) { + int r; + + assert(ret_error_message); + + arg_default_tasks_max = system_tasks_max_scale(DEFAULT_TASKS_MAX_PERCENTAGE, 100U); + + r = parse_config_file(); + if (r < 0) { + *ret_error_message = "Failed to parse config file"; + return r; + } + + if (arg_system) { + r = proc_cmdline_parse(parse_proc_cmdline_item, NULL, 0); + if (r < 0) + log_warning_errno(r, "Failed to parse kernel command line, ignoring: %m"); + } + + /* Note that this also parses bits from the kernel command line, including "debug". */ + log_parse_environment(); + + r = parse_argv(argc, argv); + if (r < 0) { + *ret_error_message = "Failed to parse commandline arguments"; + return r; + } + + /* Initialize default unit */ + if (!arg_default_unit) { + arg_default_unit = strdup(SPECIAL_DEFAULT_TARGET); + if (!arg_default_unit) { + *ret_error_message = "Failed to set default unit"; + return log_oom(); + } + } + + return 0; +} + int main(int argc, char *argv[]) { Manager *m = NULL; int r, retval = EXIT_FAILURE; @@ -2225,37 +2265,9 @@ int main(int argc, char *argv[]) { (void) reset_all_signal_handlers(); (void) ignore_signals(SIGNALS_IGNORE, -1); - arg_default_tasks_max = system_tasks_max_scale(DEFAULT_TASKS_MAX_PERCENTAGE, 100U); - - if (parse_config_file() < 0) { - error_message = "Failed to parse config file"; + r = load_configuration(argc, argv, &error_message); + if (r < 0) goto finish; - } - - if (arg_system) { - r = proc_cmdline_parse(parse_proc_cmdline_item, NULL, 0); - if (r < 0) - log_warning_errno(r, "Failed to parse kernel command line, ignoring: %m"); - } - - /* Note that this also parses bits from the kernel command - * line, including "debug". */ - log_parse_environment(); - - if (parse_argv(argc, argv) < 0) { - error_message = "Failed to parse commandline arguments"; - goto finish; - } - - /* Initialize default unit */ - if (!arg_default_unit) { - arg_default_unit = strdup(SPECIAL_DEFAULT_TARGET); - if (!arg_default_unit) { - r = log_oom(); - error_message = "Failed to set default unit"; - goto finish; - } - } if (arg_action == ACTION_TEST && geteuid() == 0) { From b0d7c9899391c3a4476703fd3b0f578b9706bf1e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 16:23:09 +0100 Subject: [PATCH 03/25] core: split out various startup safety checks from main() into its own function No functional changes, just some refactoring to make main() more digestable. --- src/core/main.c | 63 +++++++++++++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 6196a6ce6c..29220a7231 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2091,6 +2091,38 @@ static int load_configuration(int argc, char **argv, const char **ret_error_mess return 0; } +static int safety_checks(void) { + + if (arg_action == ACTION_TEST && + geteuid() == 0) { + log_error("Don't run test mode as root."); + return -EPERM; + } + + if (!arg_system && + arg_action == ACTION_RUN && + sd_booted() <= 0) { + log_error("Trying to run as user instance, but the system has not been booted with systemd."); + return -EOPNOTSUPP; + } + + if (!arg_system && + arg_action == ACTION_RUN && + !getenv("XDG_RUNTIME_DIR")) { + log_error("Trying to run as user instance, but $XDG_RUNTIME_DIR is not set."); + return -EUNATCH; + } + + if (arg_system && + arg_action == ACTION_RUN && + running_in_chroot() > 0) { + log_error("Cannot be run in a chroot() environment."); + return -EOPNOTSUPP; + } + + return 0; +} + int main(int argc, char *argv[]) { Manager *m = NULL; int r, retval = EXIT_FAILURE; @@ -2269,30 +2301,15 @@ int main(int argc, char *argv[]) { if (r < 0) goto finish; - if (arg_action == ACTION_TEST && - geteuid() == 0) { - log_error("Don't run test mode as root."); + r = safety_checks(); + if (r < 0) goto finish; - } - if (!arg_system && - arg_action == ACTION_RUN && - sd_booted() <= 0) { - log_error("Trying to run as user instance, but the system has not been booted with systemd."); - goto finish; - } - - if (arg_system && - arg_action == ACTION_RUN && - running_in_chroot() > 0) { - log_error("Cannot be run in a chroot() environment."); - goto finish; - } - - if (IN_SET(arg_action, ACTION_TEST, ACTION_HELP)) { + if (IN_SET(arg_action, ACTION_TEST, ACTION_HELP)) pager_open(arg_no_pager, false); + + if (arg_action != ACTION_RUN) skip_setup = true; - } if (arg_action == ACTION_HELP) { retval = help(); @@ -2307,12 +2324,6 @@ int main(int argc, char *argv[]) { goto finish; } - if (!arg_system && - !getenv("XDG_RUNTIME_DIR")) { - log_error("Trying to run as user instance, but $XDG_RUNTIME_DIR is not set."); - goto finish; - } - assert_se(IN_SET(arg_action, ACTION_RUN, ACTION_TEST)); /* Close logging fds, in order not to confuse fdset below */ From 5625c18a4726254f1169adb7748c3d888d47d2bf Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 16:25:28 +0100 Subject: [PATCH 04/25] main: invoke pager_open() only at one place Let's just move this up a few lines and unify the invocation. --- src/core/main.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 29220a7231..2903936ca5 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2305,7 +2305,7 @@ int main(int argc, char *argv[]) { if (r < 0) goto finish; - if (IN_SET(arg_action, ACTION_TEST, ACTION_HELP)) + if (IN_SET(arg_action, ACTION_TEST, ACTION_HELP, ACTION_DUMP_CONFIGURATION_ITEMS)) pager_open(arg_no_pager, false); if (arg_action != ACTION_RUN) @@ -2318,7 +2318,6 @@ int main(int argc, char *argv[]) { retval = version(); goto finish; } else if (arg_action == ACTION_DUMP_CONFIGURATION_ITEMS) { - pager_open(arg_no_pager, false); unit_dump_config_items(stdout); retval = EXIT_SUCCESS; goto finish; From 74da609f0d0f9112047dd746188469df3692ad4a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 16:32:10 +0100 Subject: [PATCH 05/25] main: split out security policy loading into its own function More refactoring to make things more digestable. --- src/core/main.c | 54 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 11 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 2903936ca5..51d50d1873 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2123,6 +2123,43 @@ static int safety_checks(void) { return 0; } +static int initialize_security( + bool *loaded_policy, + dual_timestamp *security_start_timestamp, + dual_timestamp *security_finish_timestamp, + const char **ret_error_message) { + + int r; + + assert(loaded_policy); + assert(security_start_timestamp); + assert(security_finish_timestamp); + assert(ret_error_message); + + dual_timestamp_get(security_start_timestamp); + + r = mac_selinux_setup(loaded_policy) < 0; + if (r < 0) { + *ret_error_message = "Failed to load SELinux policy"; + return r; + } + + r = mac_smack_setup(loaded_policy); + if (r < 0) { + *ret_error_message = "Failed to load SMACK policy"; + return r; + } + + r = ima_setup(); + if (r < 0) { + *ret_error_message = "Failed to load IMA policy"; + return r; + } + + dual_timestamp_get(security_finish_timestamp); + return 0; +} + int main(int argc, char *argv[]) { Manager *m = NULL; int r, retval = EXIT_FAILURE; @@ -2201,18 +2238,13 @@ int main(int argc, char *argv[]) { goto finish; } - dual_timestamp_get(&security_start_timestamp); - if (mac_selinux_setup(&loaded_policy) < 0) { - error_message = "Failed to load SELinux policy"; + r = initialize_security( + &loaded_policy, + &security_start_timestamp, + &security_finish_timestamp, + &error_message); + if (r < 0) goto finish; - } else if (mac_smack_setup(&loaded_policy) < 0) { - error_message = "Failed to load SMACK policy"; - goto finish; - } else if (ima_setup() < 0) { - error_message = "Failed to load IMA policy"; - goto finish; - } - dual_timestamp_get(&security_finish_timestamp); } if (mac_selinux_init() < 0) { From 263162da6cbf75fa5dd0eb514f9313ecea9155d7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 16:34:13 +0100 Subject: [PATCH 06/25] core: split out test summary output into its own function More refactoring to make main() shorter. --- src/core/main.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 51d50d1873..f342875f6f 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2160,6 +2160,16 @@ static int initialize_security( return 0; } +static void test_summary(Manager *m) { + assert(m); + + printf("-> By units:\n"); + manager_dump_units(m, stdout, "\t"); + + printf("-> By jobs:\n"); + manager_dump_jobs(m, stdout, "\t"); +} + int main(int argc, char *argv[]) { Manager *m = NULL; int r, retval = EXIT_FAILURE; @@ -2481,11 +2491,7 @@ int main(int argc, char *argv[]) { } if (arg_action == ACTION_TEST) { - printf("-> By units:\n"); - manager_dump_units(m, stdout, "\t"); - - printf("-> By jobs:\n"); - manager_dump_jobs(m, stdout, "\t"); + test_summary(m); retval = EXIT_SUCCESS; goto finish; } From febf46a46ca9d3454abaeb7ebda1b5582bfcbca0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 16:38:20 +0100 Subject: [PATCH 07/25] core: add more safety check Let's make sure that if we are PID 1 we are invoked in ACTION_RUN mode, and in arg_system mode, as well as the opposite. Everything else is untested and probably not worth supporting hence let's bail out early if people try anyway. --- src/core/main.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/core/main.c b/src/core/main.c index f342875f6f..c6702a9630 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2093,6 +2093,25 @@ static int load_configuration(int argc, char **argv, const char **ret_error_mess static int safety_checks(void) { + if (getpid_cached() == 1 && + arg_action != ACTION_RUN) { + log_error("Unsupported execution mode while PID 1."); + return -EPERM; + } + + if (getpid_cached() == 1 && + !arg_system) { + log_error("Can't run --user mode as PID 1."); + return -EPERM; + } + + if (arg_action == ACTION_RUN && + arg_system && + getpid_cached() != 1) { + log_error("Can't run system mode unless PID 1."); + return -EPERM; + } + if (arg_action == ACTION_TEST && geteuid() == 0) { log_error("Don't run test mode as root."); From 51e5ff02a8238df78b077d924605b94088f5fe58 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 16:38:57 +0100 Subject: [PATCH 08/25] main: split out taint string logging into its own function It's sufficiently complex now, let's add our own function for this too. --- src/core/main.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index c6702a9630..6734a15504 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2189,6 +2189,25 @@ static void test_summary(Manager *m) { manager_dump_jobs(m, stdout, "\t"); } +static void log_taint_string(Manager *m) { + _cleanup_free_ char *taint = NULL; + + assert(m); + + if (!arg_system) + return; + + taint = manager_taint_string(m); + if (isempty(taint)) + return; + + log_struct(LOG_NOTICE, + LOG_MESSAGE("System is tainted: %s", taint), + "TAINT=%s", taint, + "MESSAGE_ID=" SD_MESSAGE_TAINTED_STR, + NULL); +} + int main(int argc, char *argv[]) { Manager *m = NULL; int r, retval = EXIT_FAILURE; @@ -2497,17 +2516,7 @@ int main(int argc, char *argv[]) { "Loaded units and determined initial transaction in %s.", format_timespan(timespan, sizeof(timespan), after_startup - before_startup, 100 * USEC_PER_MSEC)); - if (arg_system) { - _cleanup_free_ char *taint; - - taint = manager_taint_string(m); - if (!isempty(taint)) - log_struct(LOG_NOTICE, - LOG_MESSAGE("System is tainted: %s", taint), - "TAINT=%s", taint, - "MESSAGE_ID=" SD_MESSAGE_TAINTED_STR, - NULL); - } + log_taint_string(m); if (arg_action == ACTION_TEST) { test_summary(m); From 6f101b566f35ae2d9c9529961d64b1ca9623989e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 16:48:07 +0100 Subject: [PATCH 09/25] main: move initialize_join_controllers() invocation into load_configuration() This just sets up some variables the loaded configuration will then modify. Let's invoke it hence right before loading the configuration. This moves the initialization just a tiny bit later, but that shouldn't matter, since we never access it in-between. --- src/core/main.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 6734a15504..c3d0c01772 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2056,6 +2056,12 @@ static int load_configuration(int argc, char **argv, const char **ret_error_mess assert(ret_error_message); + r = initialize_join_controllers(); + if (r < 0) { + *ret_error_message = "Failed to initialize cgroup controller joining table"; + return r; + } + arg_default_tasks_max = system_tasks_max_scale(DEFAULT_TASKS_MAX_PERCENTAGE, 100U); r = parse_config_file(); @@ -2352,12 +2358,6 @@ int main(int argc, char *argv[]) { log_warning_errno(r, "Failed to redirect standard streams to /dev/null: %m"); } - r = initialize_join_controllers(); - if (r < 0) { - error_message = "Failed to initialize cgroup controllers"; - goto finish; - } - /* Mount /proc, /sys and friends, so that /proc/cmdline and * /proc/$PID/fd is available. */ if (getpid_cached() == 1) { From 5a2e0c6257a49a8a1d6b60b33c766f4ca96ade54 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 16:49:43 +0100 Subject: [PATCH 10/25] main: move chdir("/") a bit earlier There's no need to do this within the block where logging is closed, hence move it earlier, so that this block can be kept as small as possible. --- 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 c3d0c01772..66270d4e0d 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2405,6 +2405,9 @@ int main(int argc, char *argv[]) { assert_se(IN_SET(arg_action, ACTION_RUN, ACTION_TEST)); + /* Move out of the way, so that we won't block unmounts */ + assert_se(chdir("/") == 0); + /* Close logging fds, in order not to confuse fdset below */ log_close(); @@ -2426,9 +2429,6 @@ int main(int argc, char *argv[]) { setsid(); } - /* Move out of the way, so that we won't block unmounts */ - assert_se(chdir("/") == 0); - /* Reset the console, but only if this is really init and we * are freshly booted */ if (arg_system && arg_action == ACTION_RUN) { From a70c72a0463a86ef952346304afe474add0302cf Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 16:53:13 +0100 Subject: [PATCH 11/25] main: slightly rearrange serialization fdset, and logging/console setup Let's merge two if blocks, and move log_close()/log_open() out of the testing codepath, as there's no reason to have it there. --- src/core/main.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 66270d4e0d..27a4ebc59e 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2408,11 +2408,12 @@ int main(int argc, char *argv[]) { /* Move out of the way, so that we won't block unmounts */ assert_se(chdir("/") == 0); - /* Close logging fds, in order not to confuse fdset below */ - log_close(); - - /* Remember open file descriptors for later deserialization */ if (arg_action == ACTION_RUN) { + + /* Close logging fds, in order not to confuse fdset below */ + log_close(); + + /* Remember open file descriptors for later deserialization */ r = fdset_new_fill(&fds); if (r < 0) { log_emergency_errno(r, "Failed to allocate fd set: %m"); @@ -2424,27 +2425,23 @@ int main(int argc, char *argv[]) { if (arg_serialization) assert_se(fdset_remove(fds, fileno(arg_serialization)) >= 0); - if (arg_system) + if (arg_system) { /* Become a session leader if we aren't one yet. */ setsid(); + + /* If we are init, we connect stdin/stdout/stderr to /dev/null and make sure we don't have a + * controlling tty. */ + release_terminal(); + + /* Reset the console, but only if this is really init and we are freshly booted */ + if (getpid_cached() == 1 && !skip_setup) + console_setup(); + } + + /* Open the logging devices, if possible and necessary */ + log_open(); } - /* Reset the console, but only if this is really init and we - * are freshly booted */ - if (arg_system && arg_action == ACTION_RUN) { - - /* If we are init, we connect stdin/stdout/stderr to - * /dev/null and make sure we don't have a controlling - * tty. */ - release_terminal(); - - if (getpid_cached() == 1 && !skip_setup) - console_setup(); - } - - /* Open the logging devices, if possible and necessary */ - log_open(); - if (arg_show_status == _SHOW_STATUS_UNSET) arg_show_status = SHOW_STATUS_YES; From fd8c85c6524ca64832dae0a187796a1ee56c253f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 17:00:35 +0100 Subject: [PATCH 12/25] main: let's move the arg_show_status check into status_welcome() It's kinda nice to hide this check inside of status_welcome() itself, so that it handles all this on its own. --- src/core/main.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 27a4ebc59e..af76f750fe 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1338,6 +1338,9 @@ static int status_welcome(void) { _cleanup_free_ char *pretty_name = NULL, *ansi_color = NULL; int r; + if (arg_show_status <= 0) + return 0; + r = parse_env_file("/etc/os-release", NEWLINE, "PRETTY_NAME", &pretty_name, "ANSI_COLOR", &ansi_color, @@ -1915,9 +1918,7 @@ static int initialize_runtime( */ if (arg_system && !skip_setup) { - if (arg_show_status > 0) - status_welcome(); - + status_welcome(); hostname_setup(); machine_id_setup(NULL, arg_machine_id, NULL); loopback_setup(); From 72c7329b6eda90bbbf83354c10c51f60c3523870 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 17:03:55 +0100 Subject: [PATCH 13/25] main: slight modernizations for status_welcome() There's no point in duplicating the complex parse_env_file() invocation, hence let's not do it. --- src/core/main.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index af76f750fe..7e3658f711 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1336,23 +1336,23 @@ static int enforce_syscall_archs(Set *archs) { static int status_welcome(void) { _cleanup_free_ char *pretty_name = NULL, *ansi_color = NULL; + const char *fn; int r; if (arg_show_status <= 0) return 0; - r = parse_env_file("/etc/os-release", NEWLINE, - "PRETTY_NAME", &pretty_name, - "ANSI_COLOR", &ansi_color, - NULL); - if (r == -ENOENT) - r = parse_env_file("/usr/lib/os-release", NEWLINE, + FOREACH_STRING(fn, "/etc/os-release", "/usr/lib/os-release") { + r = parse_env_file(fn, NEWLINE, "PRETTY_NAME", &pretty_name, "ANSI_COLOR", &ansi_color, NULL); + if (r != -ENOENT) + break; + } if (r < 0 && r != -ENOENT) - log_warning_errno(r, "Failed to read os-release file: %m"); + log_warning_errno(r, "Failed to read os-release file, ignoring: %m"); if (log_get_show_color()) return status_printf(NULL, false, false, From efeb853fbd03aaa63d657623139b30581d386096 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 17:09:18 +0100 Subject: [PATCH 14/25] main: split out code that collects passed fds More refactoring to make main() more digestable --- src/core/main.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 7e3658f711..8dd4da1203 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2215,6 +2215,26 @@ static void log_taint_string(Manager *m) { NULL); } +static int collect_fds(FDSet **ret_fds, const char **ret_error_message) { + int r; + + assert(ret_fds); + assert(ret_error_message); + + r = fdset_new_fill(ret_fds); + if (r < 0) { + *ret_error_message = "Failed to allocate fd set"; + return log_emergency_errno(r, "Failed to allocate fd set: %m"); + } + + fdset_cloexec(*ret_fds, true); + + if (arg_serialization) + assert_se(fdset_remove(*ret_fds, fileno(arg_serialization)) >= 0); + + return 0; +} + int main(int argc, char *argv[]) { Manager *m = NULL; int r, retval = EXIT_FAILURE; @@ -2411,20 +2431,13 @@ int main(int argc, char *argv[]) { if (arg_action == ACTION_RUN) { - /* Close logging fds, in order not to confuse fdset below */ + /* Close logging fds, in order not to confuse collecting passed fds and terminal logic below */ log_close(); /* Remember open file descriptors for later deserialization */ - r = fdset_new_fill(&fds); - if (r < 0) { - log_emergency_errno(r, "Failed to allocate fd set: %m"); - error_message = "Failed to allocate fd set"; + r = collect_fds(&fds, &error_message); + if (r < 0) goto finish; - } else - fdset_cloexec(fds, true); - - if (arg_serialization) - assert_se(fdset_remove(fds, fileno(arg_serialization)) >= 0); if (arg_system) { /* Become a session leader if we aren't one yet. */ From 2e51b31caf683409cfa953f5bbefcc72d9ffb016 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 17:13:36 +0100 Subject: [PATCH 15/25] main: split out code that sets up the console/terminal and stuff More refactoring to make main() more digestable. --- src/core/main.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 8dd4da1203..06fe372631 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2235,6 +2235,23 @@ static int collect_fds(FDSet **ret_fds, const char **ret_error_message) { return 0; } +static void setup_console_terminal(bool skip_setup) { + + if (!arg_system) + return; + + /* Become a session leader if we aren't one yet. */ + (void) setsid(); + + /* If we are init, we connect stdin/stdout/stderr to /dev/null and make sure we don't have a controlling + * tty. */ + (void) release_terminal(); + + /* Reset the console, but only if this is really init and we are freshly booted */ + if (getpid_cached() == 1 && !skip_setup) + (void) console_setup(); +} + int main(int argc, char *argv[]) { Manager *m = NULL; int r, retval = EXIT_FAILURE; @@ -2439,18 +2456,8 @@ int main(int argc, char *argv[]) { if (r < 0) goto finish; - if (arg_system) { - /* Become a session leader if we aren't one yet. */ - setsid(); - - /* If we are init, we connect stdin/stdout/stderr to /dev/null and make sure we don't have a - * controlling tty. */ - release_terminal(); - - /* Reset the console, but only if this is really init and we are freshly booted */ - if (getpid_cached() == 1 && !skip_setup) - console_setup(); - } + /* Give up any control of the console, but make sure its initialized. */ + setup_console_terminal(skip_setup); /* Open the logging devices, if possible and necessary */ log_open(); From db33214b528af9d04d552444dc96e8ccc7f03ddd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 17:16:24 +0100 Subject: [PATCH 16/25] core: move arg_show_status fix-up into load_configuration() It's part of finalizing our runtime parameters, hence let's move this into load_configuration() after we loaded everything else. This is safe, since we don't use it between the location where it was and where we place it now yet. --- src/core/main.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 06fe372631..6d440cc13f 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2095,6 +2095,10 @@ static int load_configuration(int argc, char **argv, const char **ret_error_mess } } + /* Initialize the show status setting if it hasn't been set explicitly yet */ + if (arg_show_status == _SHOW_STATUS_UNSET) + arg_show_status = SHOW_STATUS_YES; + return 0; } @@ -2463,9 +2467,6 @@ int main(int argc, char *argv[]) { log_open(); } - if (arg_show_status == _SHOW_STATUS_UNSET) - arg_show_status = SHOW_STATUS_YES; - /* Make sure we leave a core dump without panicing the * kernel. */ if (getpid_cached() == 1) { From 3c3c6cb95d70a248d88cc49e43329784858ba7c1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 17:34:12 +0100 Subject: [PATCH 17/25] main: move install_crash_handler() and mount_cgroup_controllers() invocations Let's place them in initialize_runtime(), where they appear to fit best. Effectively this is just a move a little bit down, swapping places with log_execution_mode(), which should require neither call to be done first. Note that changes the conditionalization a bit for these calls, from (PID == 1) to (arg_system && arg_action == ACTION_RUN). At this point this is pretty much the same however, as we don't allow PID 1 without ACTION_RUN and without arg_system set, safety_checks() ensures that. --- src/core/main.c | 54 ++++++++++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 6d440cc13f..623cbc4231 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1917,25 +1917,35 @@ static int initialize_runtime( * - Some only apply when we first start up, but not when we reexecute */ - if (arg_system && !skip_setup) { - status_welcome(); - hostname_setup(); - machine_id_setup(NULL, arg_machine_id, NULL); - loopback_setup(); - bump_unix_max_dgram_qlen(); - test_usr(); - write_container_id(); - } + if (arg_system) { + /* Make sure we leave a core dump without panicing the kernel. */ + install_crash_handler(); - if (arg_system && arg_watchdog_device) { - r = watchdog_set_device(arg_watchdog_device); - if (r < 0) - log_warning_errno(r, "Failed to set watchdog device to %s, ignoring: %m", - arg_watchdog_device); - } + if (!skip_setup) { + r = mount_cgroup_controllers(arg_join_controllers); + if (r < 0) { + *ret_error_message = "Failed to mount cgroup hierarchies"; + return r; + } - if (arg_system && arg_runtime_watchdog > 0 && arg_runtime_watchdog != USEC_INFINITY) - watchdog_set_timeout(&arg_runtime_watchdog); + status_welcome(); + hostname_setup(); + machine_id_setup(NULL, arg_machine_id, NULL); + loopback_setup(); + bump_unix_max_dgram_qlen(); + test_usr(); + write_container_id(); + } + + if (arg_watchdog_device) { + r = watchdog_set_device(arg_watchdog_device); + if (r < 0) + log_warning_errno(r, "Failed to set watchdog device to %s, ignoring: %m", arg_watchdog_device); + } + + if (arg_runtime_watchdog > 0 && arg_runtime_watchdog != USEC_INFINITY) + watchdog_set_timeout(&arg_runtime_watchdog); + } if (arg_timer_slack_nsec != NSEC_INFINITY) if (prctl(PR_SET_TIMERSLACK, arg_timer_slack_nsec) < 0) @@ -2467,16 +2477,6 @@ int main(int argc, char *argv[]) { log_open(); } - /* Make sure we leave a core dump without panicing the - * kernel. */ - if (getpid_cached() == 1) { - install_crash_handler(); - - r = mount_cgroup_controllers(arg_join_controllers); - if (r < 0) - goto finish; - } - log_execution_mode(&first_boot); if (arg_action == ACTION_RUN) { From 3a671cd130cc8588d9ac6251e84477f5367da9b8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 17:36:19 +0100 Subject: [PATCH 18/25] main: tweak timerslack message a bit Let's clarify that this is a non-issue, by downgrading it to LOG_WARN and saying "ignoring" in the message. --- src/core/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/main.c b/src/core/main.c index 623cbc4231..ccd35f19dd 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1949,7 +1949,7 @@ static int initialize_runtime( if (arg_timer_slack_nsec != NSEC_INFINITY) if (prctl(PR_SET_TIMERSLACK, arg_timer_slack_nsec) < 0) - log_error_errno(errno, "Failed to adjust timer slack: %m"); + log_warning_errno(errno, "Failed to adjust timer slack, ignoring: %m"); if (arg_system && !cap_test_all(arg_capability_bounding_set)) { r = capability_bounding_set_drop_usermode(arg_capability_bounding_set); From 713a88757aec3a90f9b4bd67565aa42986400ebf Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 17:37:16 +0100 Subject: [PATCH 19/25] mount-setup: fix MNT_CHECK_WRITABLE error handling, and log about the issue Let's correct the error handling (the error is in errno, not r), and let's add logging like the rest of the function has it. --- src/core/mount-setup.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/core/mount-setup.c b/src/core/mount-setup.c index 7171d8fda4..a0c5f5aaae 100644 --- a/src/core/mount-setup.c +++ b/src/core/mount-setup.c @@ -155,10 +155,12 @@ bool mount_point_ignore(const char *path) { } static int mount_one(const MountPoint *p, bool relabel) { - int r; + int r, priority; assert(p); + priority = (p->mode & MNT_FATAL) ? LOG_ERR : LOG_DEBUG; + if (p->condition_fn && !p->condition_fn()) return 0; @@ -168,7 +170,7 @@ static int mount_one(const MountPoint *p, bool relabel) { r = path_is_mount_point(p->where, NULL, AT_SYMLINK_FOLLOW); if (r < 0 && r != -ENOENT) { - log_full_errno((p->mode & MNT_FATAL) ? LOG_ERR : LOG_DEBUG, r, "Failed to determine whether %s is a mount point: %m", p->where); + log_full_errno(priority, r, "Failed to determine whether %s is a mount point: %m", p->where); return (p->mode & MNT_FATAL) ? r : 0; } if (r > 0) @@ -196,7 +198,7 @@ static int mount_one(const MountPoint *p, bool relabel) { p->type, p->flags, p->options) < 0) { - log_full_errno((p->mode & MNT_FATAL) ? LOG_ERR : LOG_DEBUG, errno, "Failed to mount %s at %s: %m", p->type, p->where); + log_full_errno(priority, errno, "Failed to mount %s at %s: %m", p->type, p->where); return (p->mode & MNT_FATAL) ? -errno : 0; } @@ -205,10 +207,13 @@ static int mount_one(const MountPoint *p, bool relabel) { (void) label_fix(p->where, false, false); if (p->mode & MNT_CHECK_WRITABLE) { - r = access(p->where, W_OK); - if (r < 0) { + if (access(p->where, W_OK) < 0) { + r = -errno; + (void) umount(p->where); (void) rmdir(p->where); + + log_full_errno(priority, r, "Mount point %s not writable after mounting: %m", p->where); return (p->mode & MNT_FATAL) ? r : 0; } } From 2d77603838b9dd86e6f8119c53df319a8606cf41 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 17:52:33 +0100 Subject: [PATCH 20/25] main: let's move ACTION_RUN test into initialize_runtime() Let's hide this check inside the function and make it easier to follow the general control flow of main(). --- src/core/main.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index ccd35f19dd..4fd6f4edd6 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1917,6 +1917,9 @@ static int initialize_runtime( * - Some only apply when we first start up, but not when we reexecute */ + if (arg_action != ACTION_RUN) + return 0; + if (arg_system) { /* Make sure we leave a core dump without panicing the kernel. */ install_crash_handler(); @@ -2479,14 +2482,12 @@ int main(int argc, char *argv[]) { log_execution_mode(&first_boot); - if (arg_action == ACTION_RUN) { - r = initialize_runtime(skip_setup, - &saved_rlimit_nofile, - &saved_rlimit_memlock, - &error_message); - if (r < 0) - goto finish; - } + r = initialize_runtime(skip_setup, + &saved_rlimit_nofile, + &saved_rlimit_memlock, + &error_message); + if (r < 0) + goto finish; r = manager_new(arg_system ? UNIT_FILE_SYSTEM : UNIT_FILE_USER, arg_action == ACTION_TEST ? MANAGER_TEST_FULL : 0, From 43db615ba4773ca5ef2800c2ad48d5816d535352 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 17:54:20 +0100 Subject: [PATCH 21/25] main: conditionalize fixup_environment() internally This code is executed before we parse command line/configuration parameters, hence let's not use arg_system to figure our how to clean up things, but instead PID == 1. Let's move that check inside of the function, to make things a bit more robust abstract from the outside. Also, let's add a log message about this, that was so far missing. --- src/core/main.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 4fd6f4edd6..28621ae38b 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1419,20 +1419,19 @@ static int fixup_environment(void) { const char *t; int r; - /* We expect the environment to be set correctly - * if run inside a container. */ + /* Only fix up the environment when we are started as PID 1 */ + if (getpid_cached() != 1) + return 0; + + /* We expect the environment to be set correctly if run inside a container. */ if (detect_container() > 0) return 0; - /* When started as PID1, the kernel uses /dev/console - * for our stdios and uses TERM=linux whatever the - * backend device used by the console. We try to make - * a better guess here since some consoles might not - * have support for color mode for example. + /* When started as PID1, the kernel uses /dev/console for our stdios and uses TERM=linux whatever the backend + * device used by the console. We try to make a better guess here since some consoles might not have support + * for color mode for example. * - * However if TERM was configured through the kernel - * command line then leave it alone. */ - + * However if TERM was configured through the kernel command line then leave it alone. */ r = proc_cmdline_get_key("TERM", 0, &term); if (r < 0) return r; @@ -2398,11 +2397,14 @@ int main(int argc, char *argv[]) { initialize_coredump(skip_setup); + r = fixup_environment(); + if (r < 0) { + log_emergency_errno(r, "Failed to fix up PID 1 environment: %m"); + error_message = "Failed to fix up PID1 environment"; + goto finish; + } + if (arg_system) { - if (fixup_environment() < 0) { - error_message = "Failed to fix up PID1 environment"; - goto finish; - } /* Try to figure out if we can use colors with the console. No * need to do that for user instances since they never log From b9e90f3aa6af0ee5b59bbf249ec852d56e814c83 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 18:51:54 +0100 Subject: [PATCH 22/25] main: do bother with uid_to_name() unless we do debug logging --- src/core/main.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 28621ae38b..ab9f3db276 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1889,11 +1889,13 @@ static void log_execution_mode(bool *ret_first_boot) { log_info("Running with unpopulated /etc."); } } else { - _cleanup_free_ char *t; + if (DEBUG_LOGGING) { + _cleanup_free_ char *t; - t = uid_to_name(getuid()); - log_debug(PACKAGE_STRING " running in %suser mode for user " UID_FMT "/%s. (" SYSTEMD_FEATURES ")", - arg_action == ACTION_TEST ? " test" : "", getuid(), strna(t)); + t = uid_to_name(getuid()); + log_debug(PACKAGE_STRING " running in %suser mode for user " UID_FMT "/%s. (" SYSTEMD_FEATURES ")", + arg_action == ACTION_TEST ? " test" : "", getuid(), strna(t)); + } *ret_first_boot = false; } From aa40ff070391e1a5eebafcec834a59973fbaac00 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 18:53:03 +0100 Subject: [PATCH 23/25] main: split out 'skip_setup' check into its own functions And let's optimize it a tiny bit, by only iterating through the argument list once, instead of twice. --- src/core/main.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index ab9f3db276..20d5c53bb5 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2270,6 +2270,25 @@ static void setup_console_terminal(bool skip_setup) { (void) console_setup(); } +static bool early_skip_setup_check(int argc, char *argv[]) { + bool found_deserialize = false; + int i; + + /* Determine if this is a reexecution or normal bootup. We do the full command line parsing much later, so + * let's just have a quick peek here. Note that if we have switched root, do all the special setup things + * anyway, even if in that case we also do deserialization. */ + + for (i = 1; i < argc; i++) { + + if (streq(argv[i], "--switched-root")) + return false; /* If we switched root, don't skip the setup. */ + else if (streq(argv[i], "--deserialize")) + found_deserialize = true; + } + + return found_deserialize; /* When we are deserializing, then we are reexecuting, hence avoid the extensive setup */ +} + int main(int argc, char *argv[]) { Manager *m = NULL; int r, retval = EXIT_FAILURE; @@ -2284,7 +2303,7 @@ int main(int argc, char *argv[]) { dual_timestamp security_start_timestamp = DUAL_TIMESTAMP_NULL; dual_timestamp security_finish_timestamp = DUAL_TIMESTAMP_NULL; static char systemd[] = "systemd"; - bool skip_setup = false; + bool skip_setup; bool loaded_policy = false; bool queue_default_job = false; bool first_boot = false; @@ -2297,16 +2316,7 @@ int main(int argc, char *argv[]) { dual_timestamp_from_monotonic(&kernel_timestamp, 0); dual_timestamp_get(&userspace_timestamp); - /* Determine if this is a reexecution or normal bootup. We do - * the full command line parsing much later, so let's just - * have a quick peek here. */ - if (strv_find(argv+1, "--deserialize")) - skip_setup = true; - - /* If we have switched root, do all the special setup - * things */ - if (strv_find(argv+1, "--switched-root")) - skip_setup = false; + skip_setup = early_skip_setup_check(argc, argv); /* If we get started via the /sbin/init symlink then we are called 'init'. After a subsequent reexecution we are then From 625e8690ab1545f9e88bdf4c3ec8c67aa6c8c2ff Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 19:02:35 +0100 Subject: [PATCH 24/25] main: reorder variable declarations a bit Let's remove a bit redundancy, and list variables of the same type/category in one declaration line. --- src/core/main.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 20d5c53bb5..34f651d6ac 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2290,26 +2290,19 @@ static bool early_skip_setup_check(int argc, char *argv[]) { } int main(int argc, char *argv[]) { - Manager *m = NULL; - int r, retval = EXIT_FAILURE; - usec_t before_startup, after_startup; - char timespan[FORMAT_TIMESPAN_MAX]; - FDSet *fds = NULL; - bool reexecute = false; - const char *shutdown_verb = NULL; - dual_timestamp initrd_timestamp = DUAL_TIMESTAMP_NULL; - dual_timestamp userspace_timestamp = DUAL_TIMESTAMP_NULL; - dual_timestamp kernel_timestamp = DUAL_TIMESTAMP_NULL; - dual_timestamp security_start_timestamp = DUAL_TIMESTAMP_NULL; - dual_timestamp security_finish_timestamp = DUAL_TIMESTAMP_NULL; - static char systemd[] = "systemd"; - bool skip_setup; - bool loaded_policy = false; - bool queue_default_job = false; - bool first_boot = false; - char *switch_root_dir = NULL, *switch_root_init = 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_t) -1); - const char *error_message = NULL; + bool skip_setup, loaded_policy = false, queue_default_job = false, first_boot = false, reexecute = false; + char *switch_root_dir = NULL, *switch_root_init = NULL; + usec_t before_startup, after_startup; + static char systemd[] = "systemd"; + char timespan[FORMAT_TIMESPAN_MAX]; + const char *shutdown_verb = NULL, *error_message = NULL; + int r, retval = EXIT_FAILURE; + Manager *m = NULL; + FDSet *fds = NULL; redirect_telinit(argc, argv); From d72a8f10b671f4ddca952d2af7d0ca157e8967f7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 15 Dec 2017 19:03:17 +0100 Subject: [PATCH 25/25] main: add some more comments for the early initialization phase --- src/core/main.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 34f651d6ac..c717473563 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2304,20 +2304,23 @@ int main(int argc, char *argv[]) { Manager *m = NULL; FDSet *fds = NULL; + /* SysV compatibility: redirect init → telinit */ redirect_telinit(argc, argv); + /* Take timestamps early on */ dual_timestamp_from_monotonic(&kernel_timestamp, 0); dual_timestamp_get(&userspace_timestamp); + /* Figure out whether we need to do initialize the system, or if we already did that because we are + * reexecuting */ skip_setup = early_skip_setup_check(argc, argv); - /* If we get started via the /sbin/init symlink then we are - called 'init'. After a subsequent reexecution we are then - called 'systemd'. That is confusing, hence let's call us - systemd right-away. */ + /* If we get started via the /sbin/init symlink then we are called 'init'. After a subsequent reexecution we + * are then called 'systemd'. That is confusing, hence let's call us systemd right-away. */ program_invocation_short_name = systemd; (void) prctl(PR_SET_NAME, systemd); + /* Save the original command line */ saved_argv = argv; saved_argc = argc;