From 886cf982d3018f7451f0548dadbc05bd2d583bb6 Mon Sep 17 00:00:00 2001 From: HATAYAMA Daisuke Date: Sat, 24 Sep 2016 21:56:07 +0900 Subject: sysctl: configure kernel parameters in the order they occur in each sysctl configuration files (#4205) Currently, systemd-sysctl command configures kernel parameters in each sysctl configuration files in random order due to characteristics of iterator of Hashmap. However, kernel parameters need to be configured in the order they occur in each sysctl configuration files. - For example, consider fs.suid_coredump and kernel.core_pattern. If fs.suid_coredump=2 is configured before kernel.core_pattern= whose default value is "core", then kernel outputs the following message: Unsafe core_pattern used with suid_dumpable=2. Pipe handler or fully qualified core dump path required. Note that the security issue mentioned in this message has already been fixed on recent kernels, so this is just a warning message on such kernels. But it's still confusing to users that this message is output on some boot and not output on another boot. - I don't know but there could be other kernel parameters that are significant in the order they are configured. - The legacy sysctl command configures kernel parameters in the order they occur in each sysctl configuration files. Although I didn't find any official specification explaining this behavior of sysctl command, I don't think there is any meaningful reason to change this behavior, in particular, to the random one. This commit does the change by simply using OrderedHashmap instead of Hashmap. --- src/sysctl/sysctl.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'src/sysctl') diff --git a/src/sysctl/sysctl.c b/src/sysctl/sysctl.c index ce7c26e7d3..fbc1e0eb1a 100644 --- a/src/sysctl/sysctl.c +++ b/src/sysctl/sysctl.c @@ -41,12 +41,12 @@ static char **arg_prefixes = NULL; static const char conf_file_dirs[] = CONF_PATHS_NULSTR("sysctl.d"); -static int apply_all(Hashmap *sysctl_options) { +static int apply_all(OrderedHashmap *sysctl_options) { char *property, *value; Iterator i; int r = 0; - HASHMAP_FOREACH_KEY(value, property, sysctl_options, i) { + ORDERED_HASHMAP_FOREACH_KEY(value, property, sysctl_options, i) { int k; k = sysctl_write(property, value); @@ -62,7 +62,7 @@ static int apply_all(Hashmap *sysctl_options) { return r; } -static int parse_file(Hashmap *sysctl_options, const char *path, bool ignore_enoent) { +static int parse_file(OrderedHashmap *sysctl_options, const char *path, bool ignore_enoent) { _cleanup_fclose_ FILE *f = NULL; int r; @@ -125,13 +125,13 @@ static int parse_file(Hashmap *sysctl_options, const char *path, bool ignore_eno } found: - existing = hashmap_get2(sysctl_options, p, &v); + existing = ordered_hashmap_get2(sysctl_options, p, &v); if (existing) { if (streq(value, existing)) continue; log_debug("Overwriting earlier assignment of %s in file '%s'.", p, path); - free(hashmap_remove(sysctl_options, p)); + free(ordered_hashmap_remove(sysctl_options, p)); free(v); } @@ -145,7 +145,7 @@ found: return log_oom(); } - k = hashmap_put(sysctl_options, property, new_value); + k = ordered_hashmap_put(sysctl_options, property, new_value); if (k < 0) { log_error_errno(k, "Failed to add sysctl variable %s to hashmap: %m", property); free(property); @@ -230,7 +230,7 @@ static int parse_argv(int argc, char *argv[]) { int main(int argc, char *argv[]) { int r = 0, k; - Hashmap *sysctl_options; + OrderedHashmap *sysctl_options; r = parse_argv(argc, argv); if (r <= 0) @@ -242,7 +242,7 @@ int main(int argc, char *argv[]) { umask(0022); - sysctl_options = hashmap_new(&string_hash_ops); + sysctl_options = ordered_hashmap_new(&string_hash_ops); if (!sysctl_options) { r = log_oom(); goto finish; @@ -280,7 +280,7 @@ int main(int argc, char *argv[]) { r = k; finish: - hashmap_free_free_free(sysctl_options); + ordered_hashmap_free_free_free(sysctl_options); strv_free(arg_prefixes); return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS; -- cgit v1.2.3 From 39540de8abe24886693ca29a9caeea85c88089aa Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 25 Oct 2016 09:22:22 +0200 Subject: sysctl: do not fail systemd-sysctl.service if /proc/sys is mounted read-only Let's make missing write access to /proc/sys non-fatal to the sysctl service. This is a follow-up to 411e869f497c7c7bd0688f1e3500f9043bc56e48 which altered the condition for running the sysctl service to check for /proc/sys/net being writable, accepting that /proc/sys might be read-only. In order to ensure the boot-up stays clean in containers lower the log level for the EROFS errors generated due to this. --- src/sysctl/sysctl.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) (limited to 'src/sysctl') diff --git a/src/sysctl/sysctl.c b/src/sysctl/sysctl.c index fbc1e0eb1a..1363a93830 100644 --- a/src/sysctl/sysctl.c +++ b/src/sysctl/sysctl.c @@ -51,11 +51,18 @@ static int apply_all(OrderedHashmap *sysctl_options) { k = sysctl_write(property, value); if (k < 0) { - log_full_errno(k == -ENOENT ? LOG_INFO : LOG_WARNING, k, - "Couldn't write '%s' to '%s', ignoring: %m", value, property); - - if (r == 0 && k != -ENOENT) - r = k; + /* If the sysctl is not available in the kernel or we are running with reduced privileges and + * cannot write it, then log about the issue at LOG_NOTICE level, and proceed without + * failing. (EROFS is treated as a permission problem here, since that's how container managers + * usually protected their sysctls.) In all other cases log an error and make the tool fail. */ + + if (IN_SET(k, -EPERM, -EACCES, -EROFS, -ENOENT)) + log_notice_errno(k, "Couldn't write '%s' to '%s', ignoring: %m", value, property); + else { + log_error_errno(k, "Couldn't write '%s' to '%s': %m", value, property); + if (r == 0) + r = k; + } } } -- cgit v1.2.3 From 9c37b41c6166ca511b317255cfad271c906e597a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 25 Oct 2016 09:25:21 +0200 Subject: sysctl: split out condition check into its own function This way, we can get rid of a label/goto. --- src/sysctl/sysctl.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) (limited to 'src/sysctl') diff --git a/src/sysctl/sysctl.c b/src/sysctl/sysctl.c index 1363a93830..a2a7a10f69 100644 --- a/src/sysctl/sysctl.c +++ b/src/sysctl/sysctl.c @@ -69,6 +69,25 @@ static int apply_all(OrderedHashmap *sysctl_options) { return r; } +static bool test_prefix(const char *p) { + char **i; + + if (strv_isempty(arg_prefixes)) + return true; + + STRV_FOREACH(i, arg_prefixes) { + const char *t; + + t = path_startswith(*i, "/proc/sys/"); + if (!t) + t = *i; + if (path_startswith(p, t)) + return true; + } + + return false; +} + static int parse_file(OrderedHashmap *sysctl_options, const char *path, bool ignore_enoent) { _cleanup_fclose_ FILE *f = NULL; int r; @@ -118,20 +137,9 @@ static int parse_file(OrderedHashmap *sysctl_options, const char *path, bool ign p = sysctl_normalize(strstrip(p)); value = strstrip(value); - if (!strv_isempty(arg_prefixes)) { - char **i, *t; - STRV_FOREACH(i, arg_prefixes) { - t = path_startswith(*i, "/proc/sys/"); - if (t == NULL) - t = *i; - if (path_startswith(p, t)) - goto found; - } - /* not found */ + if (!test_prefix(p)) continue; - } -found: existing = ordered_hashmap_get2(sysctl_options, p, &v); if (existing) { if (streq(value, existing)) -- cgit v1.2.3 From 98bf5011fe670ea18b7b35d6c8ca3e84d4efbccf Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 25 Oct 2016 09:26:10 +0200 Subject: sysctl: when failing to process a config line, show line nr --- src/sysctl/sysctl.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'src/sysctl') diff --git a/src/sysctl/sysctl.c b/src/sysctl/sysctl.c index a2a7a10f69..7117955568 100644 --- a/src/sysctl/sysctl.c +++ b/src/sysctl/sysctl.c @@ -90,6 +90,7 @@ static bool test_prefix(const char *p) { static int parse_file(OrderedHashmap *sysctl_options, const char *path, bool ignore_enoent) { _cleanup_fclose_ FILE *f = NULL; + unsigned c = 0; int r; assert(path); @@ -115,6 +116,8 @@ static int parse_file(OrderedHashmap *sysctl_options, const char *path, bool ign return log_error_errno(errno, "Failed to read file '%s', ignoring: %m", path); } + c++; + p = strstrip(l); if (!*p) continue; @@ -124,7 +127,7 @@ static int parse_file(OrderedHashmap *sysctl_options, const char *path, bool ign value = strchr(p, '='); if (!value) { - log_error("Line is not an assignment in file '%s': %s", path, value); + log_error("Line is not an assignment at '%s:%u': %s", path, c, value); if (r == 0) r = -EINVAL; @@ -145,7 +148,7 @@ static int parse_file(OrderedHashmap *sysctl_options, const char *path, bool ign if (streq(value, existing)) continue; - log_debug("Overwriting earlier assignment of %s in file '%s'.", p, path); + log_debug("Overwriting earlier assignment of %s at '%s:%u'.", p, path, c); free(ordered_hashmap_remove(sysctl_options, p)); free(v); } -- cgit v1.2.3 From 4f14f2bb6f7402302fd4fcad6754e5ee218f4487 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 25 Oct 2016 09:26:31 +0200 Subject: sysctl: no need to check for eof twice Let's only check for eof once after the fgets(). There's no point in checking EOF before the first read, and twice in each loop. --- src/sysctl/sysctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/sysctl') diff --git a/src/sysctl/sysctl.c b/src/sysctl/sysctl.c index 7117955568..cce91b3d67 100644 --- a/src/sysctl/sysctl.c +++ b/src/sysctl/sysctl.c @@ -104,7 +104,7 @@ static int parse_file(OrderedHashmap *sysctl_options, const char *path, bool ign } log_debug("Parsing %s", path); - while (!feof(f)) { + for (;;) { char l[LINE_MAX], *p, *value, *new_value, *property, *existing; void *v; int k; -- cgit v1.2.3 From e5105081152c3eb4558d9e5d9c8aaa33f9d802ff Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 25 Oct 2016 09:27:39 +0200 Subject: sysctl: minor simplification Let's place only one ternary operator. --- src/sysctl/sysctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/sysctl') diff --git a/src/sysctl/sysctl.c b/src/sysctl/sysctl.c index cce91b3d67..b3587e249d 100644 --- a/src/sysctl/sysctl.c +++ b/src/sysctl/sysctl.c @@ -247,12 +247,12 @@ static int parse_argv(int argc, char *argv[]) { } int main(int argc, char *argv[]) { + OrderedHashmap *sysctl_options = NULL; int r = 0, k; - OrderedHashmap *sysctl_options; r = parse_argv(argc, argv); if (r <= 0) - return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS; + goto finish; log_set_target(LOG_TARGET_AUTO); log_parse_environment(); -- cgit v1.2.3