From 3787934b54822e6d6fcfd95dd2a5157c1d1b2212 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 9 Aug 2023 17:41:55 +0900 Subject: [PATCH 1/5] proc-cmdline: make proc_cmdline_get_bool() take flags All other command line parsers takes flags. Let's make proc_cmdline_get_bool() also take flags. Though, currently, no flag is set by the caller. --- src/basic/proc-cmdline.c | 4 +-- src/basic/proc-cmdline.h | 2 +- src/core/main.c | 2 +- src/firstboot/firstboot.c | 2 +- src/partition/repart.c | 2 +- src/shared/cgroup-setup.c | 4 +-- src/shared/condition.c | 2 +- src/shared/reboot-util.c | 2 +- src/sulogin-shell/sulogin-shell.c | 2 +- src/test/test-proc-cmdline.c | 52 +++++++++++++++---------------- src/udev/net/link-config.c | 2 +- 11 files changed, 38 insertions(+), 38 deletions(-) diff --git a/src/basic/proc-cmdline.c b/src/basic/proc-cmdline.c index 32d4f414cc..95130a36be 100644 --- a/src/basic/proc-cmdline.c +++ b/src/basic/proc-cmdline.c @@ -392,13 +392,13 @@ int proc_cmdline_get_key(const char *key, ProcCmdlineFlags flags, char **ret_val return cmdline_get_key(args, key, flags, ret_value); } -int proc_cmdline_get_bool(const char *key, bool *ret) { +int proc_cmdline_get_bool(const char *key, ProcCmdlineFlags flags, bool *ret) { _cleanup_free_ char *v = NULL; int r; assert(ret); - r = proc_cmdline_get_key(key, PROC_CMDLINE_VALUE_OPTIONAL, &v); + r = proc_cmdline_get_key(key, flags | PROC_CMDLINE_VALUE_OPTIONAL, &v); if (r < 0) return r; if (r == 0) { /* key not specified at all */ diff --git a/src/basic/proc-cmdline.h b/src/basic/proc-cmdline.h index a64d7757bc..babdcbeeaa 100644 --- a/src/basic/proc-cmdline.h +++ b/src/basic/proc-cmdline.h @@ -22,7 +22,7 @@ int proc_cmdline_strv(char ***ret); int proc_cmdline_parse(const proc_cmdline_parse_t parse, void *userdata, ProcCmdlineFlags flags); int proc_cmdline_get_key(const char *parameter, ProcCmdlineFlags flags, char **value); -int proc_cmdline_get_bool(const char *key, bool *ret); +int proc_cmdline_get_bool(const char *key, ProcCmdlineFlags flags, bool *ret); int proc_cmdline_get_key_many_internal(ProcCmdlineFlags flags, ...); #define proc_cmdline_get_key_many(flags, ...) proc_cmdline_get_key_many_internal(flags, __VA_ARGS__, NULL) diff --git a/src/core/main.c b/src/core/main.c index 822825ca6f..c929b88db6 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -2183,7 +2183,7 @@ static void log_execution_mode(bool *ret_first_boot) { /* Let's check whether we are in first boot. First, check if an override was * specified on the kernel commandline. If yes, we honour that. */ - r = proc_cmdline_get_bool("systemd.condition-first-boot", &first_boot); + r = proc_cmdline_get_bool("systemd.condition-first-boot", /* flags = */ 0, &first_boot); if (r < 0) log_debug_errno(r, "Failed to parse systemd.condition-first-boot= kernel commandline argument, ignoring: %m"); diff --git a/src/firstboot/firstboot.c b/src/firstboot/firstboot.c index 1956ab3b13..59d4dcd118 100644 --- a/src/firstboot/firstboot.c +++ b/src/firstboot/firstboot.c @@ -1650,7 +1650,7 @@ static int run(int argc, char *argv[]) { * opposed to some other file system tree/image) */ bool enabled; - r = proc_cmdline_get_bool("systemd.firstboot", &enabled); + r = proc_cmdline_get_bool("systemd.firstboot", /* flags = */ 0, &enabled); if (r < 0) return log_error_errno(r, "Failed to parse systemd.firstboot= kernel command line argument, ignoring: %m"); if (r > 0 && !enabled) { diff --git a/src/partition/repart.c b/src/partition/repart.c index c87f42346d..afe0dcc7fb 100644 --- a/src/partition/repart.c +++ b/src/partition/repart.c @@ -6690,7 +6690,7 @@ static int parse_proc_cmdline_factory_reset(void) { if (!in_initrd()) /* Never honour kernel command line factory reset request outside of the initrd */ return 0; - r = proc_cmdline_get_bool("systemd.factory_reset", &b); + r = proc_cmdline_get_bool("systemd.factory_reset", /* flags = */ 0, &b); if (r < 0) return log_error_errno(r, "Failed to parse systemd.factory_reset kernel command line argument: %m"); if (r > 0) { diff --git a/src/shared/cgroup-setup.c b/src/shared/cgroup-setup.c index 1dc56b11ad..c35fd460bd 100644 --- a/src/shared/cgroup-setup.c +++ b/src/shared/cgroup-setup.c @@ -96,7 +96,7 @@ bool cg_is_unified_wanted(void) { return (wanted = r >= CGROUP_UNIFIED_ALL); /* If we were explicitly passed systemd.unified_cgroup_hierarchy, respect that. */ - r = proc_cmdline_get_bool("systemd.unified_cgroup_hierarchy", &b); + r = proc_cmdline_get_bool("systemd.unified_cgroup_hierarchy", /* flags = */ 0, &b); if (r > 0) return (wanted = b); @@ -147,7 +147,7 @@ bool cg_is_hybrid_wanted(void) { /* Otherwise, let's see what the kernel command line has to say. Since checking is expensive, cache * a non-error result. */ - r = proc_cmdline_get_bool("systemd.legacy_systemd_cgroup_controller", &b); + r = proc_cmdline_get_bool("systemd.legacy_systemd_cgroup_controller", /* flags = */ 0, &b); /* The meaning of the kernel option is reversed wrt. to the return value of this function, hence the * negation. */ diff --git a/src/shared/condition.c b/src/shared/condition.c index 092f32a69e..06fcd71be5 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -748,7 +748,7 @@ static int condition_test_needs_update(Condition *c, char **env) { assert(c->parameter); assert(c->type == CONDITION_NEEDS_UPDATE); - r = proc_cmdline_get_bool("systemd.condition-needs-update", &b); + r = proc_cmdline_get_bool("systemd.condition-needs-update", /* flags = */ 0, &b); if (r < 0) log_debug_errno(r, "Failed to parse systemd.condition-needs-update= kernel command line argument, ignoring: %m"); if (r > 0) diff --git a/src/shared/reboot-util.c b/src/shared/reboot-util.c index a010bf6e7f..75910ede44 100644 --- a/src/shared/reboot-util.c +++ b/src/shared/reboot-util.c @@ -113,7 +113,7 @@ int shall_restore_state(void) { bool ret; int r; - r = proc_cmdline_get_bool("systemd.restore_state", &ret); + r = proc_cmdline_get_bool("systemd.restore_state", /* flags = */ 0, &ret); if (r < 0) return r; diff --git a/src/sulogin-shell/sulogin-shell.c b/src/sulogin-shell/sulogin-shell.c index 7ffafcda55..f2de30cc3f 100644 --- a/src/sulogin-shell/sulogin-shell.c +++ b/src/sulogin-shell/sulogin-shell.c @@ -109,7 +109,7 @@ static int run(int argc, char *argv[]) { /* We look the argument in the kernel cmdline under the same name as the environment variable * to express that this is not supported at the same level as the regular kernel cmdline * switches. */ - r = proc_cmdline_get_bool("SYSTEMD_SULOGIN_FORCE", &force); + r = proc_cmdline_get_bool("SYSTEMD_SULOGIN_FORCE", /* flags = */ 0, &force); if (r < 0) log_debug_errno(r, "Failed to parse SYSTEMD_SULOGIN_FORCE from kernel command line, ignoring: %m"); } diff --git a/src/test/test-proc-cmdline.c b/src/test/test-proc-cmdline.c index 4fd9125378..3614ebe6f4 100644 --- a/src/test/test-proc-cmdline.c +++ b/src/test/test-proc-cmdline.c @@ -178,19 +178,19 @@ TEST(proc_cmdline_get_bool) { assert_se(putenv((char*) "SYSTEMD_PROC_CMDLINE=foo_bar bar-waldo=1 x_y-z=0 quux=miep\nda=yes\nthe=1") == 0); assert_se(putenv((char*) "SYSTEMD_EFI_OPTIONS=") == 0); - assert_se(proc_cmdline_get_bool("", &value) == -EINVAL); - assert_se(proc_cmdline_get_bool("abc", &value) == 0 && value == false); - assert_se(proc_cmdline_get_bool("foo_bar", &value) > 0 && value == true); - assert_se(proc_cmdline_get_bool("foo-bar", &value) > 0 && value == true); - assert_se(proc_cmdline_get_bool("bar-waldo", &value) > 0 && value == true); - assert_se(proc_cmdline_get_bool("bar_waldo", &value) > 0 && value == true); - assert_se(proc_cmdline_get_bool("x_y-z", &value) > 0 && value == false); - assert_se(proc_cmdline_get_bool("x-y-z", &value) > 0 && value == false); - assert_se(proc_cmdline_get_bool("x-y_z", &value) > 0 && value == false); - assert_se(proc_cmdline_get_bool("x_y_z", &value) > 0 && value == false); - assert_se(proc_cmdline_get_bool("quux", &value) == -EINVAL && value == false); - assert_se(proc_cmdline_get_bool("da", &value) > 0 && value == true); - assert_se(proc_cmdline_get_bool("the", &value) > 0 && value == true); + assert_se(proc_cmdline_get_bool("", /* flags = */ 0, &value) == -EINVAL); + assert_se(proc_cmdline_get_bool("abc", /* flags = */ 0, &value) == 0 && value == false); + assert_se(proc_cmdline_get_bool("foo_bar", /* flags = */ 0, &value) > 0 && value == true); + assert_se(proc_cmdline_get_bool("foo-bar", /* flags = */ 0, &value) > 0 && value == true); + assert_se(proc_cmdline_get_bool("bar-waldo", /* flags = */ 0, &value) > 0 && value == true); + assert_se(proc_cmdline_get_bool("bar_waldo", /* flags = */ 0, &value) > 0 && value == true); + assert_se(proc_cmdline_get_bool("x_y-z", /* flags = */ 0, &value) > 0 && value == false); + assert_se(proc_cmdline_get_bool("x-y-z", /* flags = */ 0, &value) > 0 && value == false); + assert_se(proc_cmdline_get_bool("x-y_z", /* flags = */ 0, &value) > 0 && value == false); + assert_se(proc_cmdline_get_bool("x_y_z", /* flags = */ 0, &value) > 0 && value == false); + assert_se(proc_cmdline_get_bool("quux", /* flags = */ 0, &value) == -EINVAL && value == false); + assert_se(proc_cmdline_get_bool("da", /* flags = */ 0, &value) > 0 && value == true); + assert_se(proc_cmdline_get_bool("the", /* flags = */ 0, &value) > 0 && value == true); } #if ENABLE_EFI @@ -200,19 +200,19 @@ TEST(proc_cmdline_get_bool_efi) { assert_se(putenv((char*) "SYSTEMD_PROC_CMDLINE=") == 0); assert_se(putenv((char*) "SYSTEMD_EFI_OPTIONS=foo_bar bar-waldo=1 x_y-z=0 quux=miep\nda=yes\nthe=1") == 0); - assert_se(proc_cmdline_get_bool("", &value) == -EINVAL); - assert_se(proc_cmdline_get_bool("abc", &value) == 0 && value == false); - assert_se(proc_cmdline_get_bool("foo_bar", &value) > 0 && value == true); - assert_se(proc_cmdline_get_bool("foo-bar", &value) > 0 && value == true); - assert_se(proc_cmdline_get_bool("bar-waldo", &value) > 0 && value == true); - assert_se(proc_cmdline_get_bool("bar_waldo", &value) > 0 && value == true); - assert_se(proc_cmdline_get_bool("x_y-z", &value) > 0 && value == false); - assert_se(proc_cmdline_get_bool("x-y-z", &value) > 0 && value == false); - assert_se(proc_cmdline_get_bool("x-y_z", &value) > 0 && value == false); - assert_se(proc_cmdline_get_bool("x_y_z", &value) > 0 && value == false); - assert_se(proc_cmdline_get_bool("quux", &value) == -EINVAL && value == false); - assert_se(proc_cmdline_get_bool("da", &value) > 0 && value == true); - assert_se(proc_cmdline_get_bool("the", &value) > 0 && value == true); + assert_se(proc_cmdline_get_bool("", /* flags = */ 0, &value) == -EINVAL); + assert_se(proc_cmdline_get_bool("abc", /* flags = */ 0, &value) == 0 && value == false); + assert_se(proc_cmdline_get_bool("foo_bar", /* flags = */ 0, &value) > 0 && value == true); + assert_se(proc_cmdline_get_bool("foo-bar", /* flags = */ 0, &value) > 0 && value == true); + assert_se(proc_cmdline_get_bool("bar-waldo", /* flags = */ 0, &value) > 0 && value == true); + assert_se(proc_cmdline_get_bool("bar_waldo", /* flags = */ 0, &value) > 0 && value == true); + assert_se(proc_cmdline_get_bool("x_y-z", /* flags = */ 0, &value) > 0 && value == false); + assert_se(proc_cmdline_get_bool("x-y-z", /* flags = */ 0, &value) > 0 && value == false); + assert_se(proc_cmdline_get_bool("x-y_z", /* flags = */ 0, &value) > 0 && value == false); + assert_se(proc_cmdline_get_bool("x_y_z", /* flags = */ 0, &value) > 0 && value == false); + assert_se(proc_cmdline_get_bool("quux", /* flags = */ 0, &value) == -EINVAL && value == false); + assert_se(proc_cmdline_get_bool("da", /* flags = */ 0, &value) > 0 && value == true); + assert_se(proc_cmdline_get_bool("the", /* flags = */ 0, &value) > 0 && value == true); } #endif diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c index b554a6827d..ff8fb24f66 100644 --- a/src/udev/net/link-config.c +++ b/src/udev/net/link-config.c @@ -702,7 +702,7 @@ static bool enable_name_policy(void) { if (cached >= 0) return cached; - r = proc_cmdline_get_bool("net.ifnames", &b); + r = proc_cmdline_get_bool("net.ifnames", /* flags = */ 0, &b); if (r < 0) log_warning_errno(r, "Failed to parse net.ifnames= kernel command line option, ignoring: %m"); if (r <= 0) From 1177373b6a0aa3698c2ec300bed0ad338f8f4721 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 9 Aug 2023 02:30:33 +0900 Subject: [PATCH 2/5] fstab-util: introduce fstab_enabled() helper function And refuse to parse fstab when 'fstab=no' is specified in the kernel command line. When 'fstab=no' is specified in the kernel command line, fstab-generator does not parse fstab and will not create e.g. /boot or /efi mount entry even if fstab contains entries for the mount points. However, gpt-auto generator may parse fstab file, and adjust or ignore mounts for EFI or XBOOTLDR partitions based on the fstab file. This makes gpt-auto also ignore fstab entries if 'fstab=no' is set in the kernel command line. --- src/shared/fstab-util.c | 27 +++++++++++++++++++++++++++ src/shared/fstab-util.h | 8 ++++++++ 2 files changed, 35 insertions(+) diff --git a/src/shared/fstab-util.c b/src/shared/fstab-util.c index 28121b8e2c..fed0e1113f 100644 --- a/src/shared/fstab-util.c +++ b/src/shared/fstab-util.c @@ -13,15 +13,39 @@ #include "nulstr-util.h" #include "parse-util.h" #include "path-util.h" +#include "proc-cmdline.h" #include "string-util.h" #include "strv.h" +bool fstab_enabled_full(int enabled) { + static int cached = -1; + bool val; + int r; + + /* If 'enabled' is non-negative, then update the cache with it. */ + if (enabled >= 0) + cached = enabled; + + if (cached >= 0) + return cached; + + r = proc_cmdline_get_bool("fstab", PROC_CMDLINE_STRIP_RD_PREFIX, &val); + if (r < 0) + log_debug_errno(r, "Failed to parse kernel command line, ignoring: %m"); + + /* If nothing specified, then defaults to true. */ + return (cached = r > 0 ? val : true); +} + int fstab_has_fstype(const char *fstype) { _cleanup_endmntent_ FILE *f = NULL; struct mntent *m; assert(fstype); + if (!fstab_enabled()) + return false; + f = setmntent(fstab_path(), "re"); if (!f) return errno == ENOENT ? false : -errno; @@ -88,6 +112,9 @@ int fstab_is_mount_point_full(const char *where, const char *path) { assert(where || path); + if (!fstab_enabled()) + return false; + f = setmntent(fstab_path(), "re"); if (!f) return errno == ENOENT ? false : -errno; diff --git a/src/shared/fstab-util.h b/src/shared/fstab-util.h index a1c079fd41..9cf34f0f8b 100644 --- a/src/shared/fstab-util.h +++ b/src/shared/fstab-util.h @@ -6,6 +6,14 @@ #include "macro.h" +bool fstab_enabled_full(int enabled); +static inline bool fstab_enabled(void) { + return fstab_enabled_full(-1); +} +static inline bool fstab_set_enabled(bool enabled) { + return fstab_enabled_full(enabled); +} + bool fstab_is_extrinsic(const char *mount, const char *opts); int fstab_has_fstype(const char *fstype); From b564224b3713ef316aff272dd0fc8b79cea86292 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 9 Aug 2023 02:40:21 +0900 Subject: [PATCH 3/5] fstab-generator: update cache in fstab_enabled_full() based on the parsed result of fstab= Currently, fstab-generator does not use fstab file parsers in fstab-util.c. So, this is not necessary. Just for a possible optimization in the case the parsers used in the future. --- src/fstab-generator/fstab-generator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index c88f5d4b47..0a30f8c9c9 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -1367,7 +1367,7 @@ static int parse_proc_cmdline_item(const char *key, const char *value, void *dat if (r < 0) log_warning("Failed to parse fstab switch %s. Ignoring.", value); else - arg_fstab_enabled = r; + arg_fstab_enabled = fstab_set_enabled(r); } else if (streq(key, "root")) { From 1be6f21320de1992abf45d893bc23dc187b112df Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 9 Aug 2023 01:57:16 +0900 Subject: [PATCH 4/5] remount-fs: split-out remount_by_fstab() No functional change, just refactoring and preparation for later commits. --- src/remount-fs/remount-fs.c | 58 +++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/src/remount-fs/remount-fs.c b/src/remount-fs/remount-fs.c index 4515592387..25e661105b 100644 --- a/src/remount-fs/remount-fs.c +++ b/src/remount-fs/remount-fs.c @@ -69,13 +69,45 @@ static int do_remount(const char *path, bool force_rw, Hashmap **pids) { return track_pid(pids, path, pid); } -static int run(int argc, char *argv[]) { +static int remount_by_fstab(Hashmap **ret_pids) { _cleanup_hashmap_free_free_ Hashmap *pids = NULL; _cleanup_endmntent_ FILE *f = NULL; bool has_root = false; struct mntent* me; int r; + assert(ret_pids); + + f = setmntent(fstab_path(), "re"); + if (!f) { + if (errno != ENOENT) + return log_error_errno(errno, "Failed to open %s: %m", fstab_path()); + + return 0; + } + + while ((me = getmntent(f))) { + /* Remount the root fs, /usr, and all API VFSs */ + if (!mount_point_is_api(me->mnt_dir) && + !PATH_IN_SET(me->mnt_dir, "/", "/usr")) + continue; + + if (path_equal(me->mnt_dir, "/")) + has_root = true; + + r = do_remount(me->mnt_dir, false, &pids); + if (r < 0) + return r; + } + + *ret_pids = TAKE_PTR(pids); + return has_root; +} + +static int run(int argc, char *argv[]) { + _cleanup_hashmap_free_free_ Hashmap *pids = NULL; + int r; + log_setup(); if (argc > 1) @@ -84,26 +116,10 @@ static int run(int argc, char *argv[]) { umask(0022); - f = setmntent(fstab_path(), "re"); - if (!f) { - if (errno != ENOENT) - return log_error_errno(errno, "Failed to open %s: %m", fstab_path()); - } else - while ((me = getmntent(f))) { - /* Remount the root fs, /usr, and all API VFSs */ - if (!mount_point_is_api(me->mnt_dir) && - !PATH_IN_SET(me->mnt_dir, "/", "/usr")) - continue; - - if (path_equal(me->mnt_dir, "/")) - has_root = true; - - r = do_remount(me->mnt_dir, false, &pids); - if (r < 0) - return r; - } - - if (!has_root) { + r = remount_by_fstab(&pids); + if (r < 0) + return r; + if (r == 0) { /* The $SYSTEMD_REMOUNT_ROOT_RW environment variable is set by systemd-gpt-auto-generator to tell us * whether to remount things. We honour it only if there's no explicit line in /etc/fstab configured * which takes precedence. */ From a38de31c8b0dc5a5b1b534e766cb12988a2db5d9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 9 Aug 2023 02:02:23 +0900 Subject: [PATCH 5/5] remount-fs: refuse to remount based on fstab when fstab=no kernel command line option specified Otherwise, if for some reasons remount-fs is invoked even when fstab=no is specified, mounts may get unexpected options from fstab. For safety, let's parse the kernel command line option. --- src/remount-fs/remount-fs.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/remount-fs/remount-fs.c b/src/remount-fs/remount-fs.c index 25e661105b..2874a0390d 100644 --- a/src/remount-fs/remount-fs.c +++ b/src/remount-fs/remount-fs.c @@ -78,6 +78,9 @@ static int remount_by_fstab(Hashmap **ret_pids) { assert(ret_pids); + if (!fstab_enabled()) + return 0; + f = setmntent(fstab_path(), "re"); if (!f) { if (errno != ENOENT)