From 4e0c20de9781a9b46e11657b1aff535dd385fa51 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 8 Nov 2017 18:35:16 +0100 Subject: [PATCH 01/13] namespace: set up OS hierarchy only after mounting the new root, not before Otherwise it's a pointless excercise, as we'll set up an empty directory tree that's never going to be used. Hence, let's move this around a bit, so that we do the basesystem initialization exactly when RootImage= or RootDirectory= are used, but not otherwise. --- src/core/namespace.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/namespace.c b/src/core/namespace.c index 33349f288e..f1ab6f9736 100644 --- a/src/core/namespace.c +++ b/src/core/namespace.c @@ -1152,10 +1152,6 @@ int setup_namespace( } } - /* Try to set up the new root directory before mounting anything there */ - if (root) - (void) base_filesystem_create(root, UID_INVALID, GID_INVALID); - if (root_image) { /* A root image is specified, mount it to the right place */ r = dissected_image_mount(dissected_image, root, dissect_image_flags); @@ -1192,6 +1188,10 @@ int setup_namespace( } } + /* Try to set up the new root directory before mounting anything else there. */ + if (root_image || root_directory) + (void) base_filesystem_create(root, UID_INVALID, GID_INVALID); + if (n_mounts > 0) { _cleanup_fclose_ FILE *proc_self_mountinfo = NULL; char **blacklist; From e7e4a2584f8359e75c027312969d07add683e94d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 8 Nov 2017 18:36:59 +0100 Subject: [PATCH 02/13] update TODO --- TODO | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/TODO b/TODO index 63b8635a01..d0807a4386 100644 --- a/TODO +++ b/TODO @@ -66,10 +66,16 @@ Features: * In journalctl add a way how "-o verbose" and suchlike can be tweaked to show only a specific set of properties +* beef up pam_systemd to take unit file settings such as cgroups properties as + parameters + * export UID ranges nspawns's --private-user and DynamicUser= uses in the systemd.pc pkg-config file, the same way we already expose the system user boundary there +* a new "systemd-analyze security" tool outputting a checklist of security + features a service does and does not implement + * Whenever we check a UID against the system UID range, also check for the dynamic UID range From 00b4a24743133bbb742ecdd2156ee4f7c9f32599 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 8 Nov 2017 18:39:28 +0100 Subject: [PATCH 03/13] cgroup-util: add brief comments clarifying which controllers are v2-only and which v1-only --- src/basic/cgroup-util.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h index c16a33723c..c549aa4024 100644 --- a/src/basic/cgroup-util.h +++ b/src/basic/cgroup-util.h @@ -34,11 +34,11 @@ /* An enum of well known cgroup controllers */ typedef enum CGroupController { CGROUP_CONTROLLER_CPU, - CGROUP_CONTROLLER_CPUACCT, - CGROUP_CONTROLLER_IO, - CGROUP_CONTROLLER_BLKIO, + CGROUP_CONTROLLER_CPUACCT, /* v1 only */ + CGROUP_CONTROLLER_IO, /* v2 only */ + CGROUP_CONTROLLER_BLKIO, /* v1 only */ CGROUP_CONTROLLER_MEMORY, - CGROUP_CONTROLLER_DEVICES, + CGROUP_CONTROLLER_DEVICES, /* v1 only */ CGROUP_CONTROLLER_PIDS, _CGROUP_CONTROLLER_MAX, _CGROUP_CONTROLLER_INVALID = -1, From ec635a2d21a5492969de7be2de9dfb0af3833b02 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 8 Nov 2017 19:01:18 +0100 Subject: [PATCH 04/13] cgroup: improve cg_mask_to_string a bit, and add tests for it --- src/basic/cgroup-util.c | 29 +++++++++++++++++++++-------- src/test/test-cgroup-mask.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index f5fed2a927..0100fc6dbf 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -2244,10 +2244,10 @@ int cg_trim_everywhere(CGroupMask supported, const char *path, bool delete_root) } int cg_mask_to_string(CGroupMask mask, char **ret) { - const char *controllers[_CGROUP_CONTROLLER_MAX + 1]; + _cleanup_free_ char *s = NULL; + size_t n = 0, allocated = 0; + bool space = false; CGroupController c; - int i = 0; - char *s; assert(ret); @@ -2257,19 +2257,32 @@ int cg_mask_to_string(CGroupMask mask, char **ret) { } for (c = 0; c < _CGROUP_CONTROLLER_MAX; c++) { + const char *k; + size_t l; if (!(mask & CGROUP_CONTROLLER_TO_MASK(c))) continue; - controllers[i++] = cgroup_controller_to_string(c); - controllers[i] = NULL; + k = cgroup_controller_to_string(c); + l = strlen(k); + + if (!GREEDY_REALLOC(s, allocated, n + space + l + 1)) + return -ENOMEM; + + if (space) + s[n] = ' '; + memcpy(s + n + space, k, l); + n += space + l; + + space = true; } - s = strv_join((char **)controllers, NULL); - if (!s) - return -ENOMEM; + assert(s); + s[n] = 0; *ret = s; + s = NULL; + return 0; } diff --git a/src/test/test-cgroup-mask.c b/src/test/test-cgroup-mask.c index 6fd35c81dc..4415bc7b9d 100644 --- a/src/test/test-cgroup-mask.c +++ b/src/test/test-cgroup-mask.c @@ -22,6 +22,7 @@ #include "macro.h" #include "manager.h" #include "rm-rf.h" +#include "string-util.h" #include "test-helper.h" #include "tests.h" #include "unit.h" @@ -117,10 +118,38 @@ static int test_cgroup_mask(void) { return 0; } +static void test_cg_mask_to_string_one(CGroupMask mask, const char *t) { + _cleanup_free_ char *b = NULL; + + assert_se(cg_mask_to_string(mask, &b) >= 0); + assert_se(streq_ptr(b, t)); +} + +static void test_cg_mask_to_string(void) { + test_cg_mask_to_string_one(0, NULL); + test_cg_mask_to_string_one(_CGROUP_MASK_ALL, "cpu cpuacct io blkio memory devices pids"); + test_cg_mask_to_string_one(CGROUP_MASK_CPU, "cpu"); + test_cg_mask_to_string_one(CGROUP_MASK_CPUACCT, "cpuacct"); + test_cg_mask_to_string_one(CGROUP_MASK_IO, "io"); + test_cg_mask_to_string_one(CGROUP_MASK_BLKIO, "blkio"); + test_cg_mask_to_string_one(CGROUP_MASK_MEMORY, "memory"); + test_cg_mask_to_string_one(CGROUP_MASK_DEVICES, "devices"); + test_cg_mask_to_string_one(CGROUP_MASK_PIDS, "pids"); + test_cg_mask_to_string_one(CGROUP_MASK_CPU|CGROUP_MASK_CPUACCT, "cpu cpuacct"); + test_cg_mask_to_string_one(CGROUP_MASK_CPU|CGROUP_MASK_PIDS, "cpu pids"); + test_cg_mask_to_string_one(CGROUP_MASK_CPUACCT|CGROUP_MASK_PIDS, "cpuacct pids"); + test_cg_mask_to_string_one(CGROUP_MASK_DEVICES|CGROUP_MASK_PIDS, "devices pids"); + test_cg_mask_to_string_one(CGROUP_MASK_IO|CGROUP_MASK_BLKIO, "io blkio"); +} + int main(int argc, char* argv[]) { int rc = 0; + log_parse_environment(); + log_open(); + TEST_REQ_RUNNING_SYSTEMD(rc = test_cgroup_mask()); + test_cg_mask_to_string(); return rc; } From 316049701787b6964163acdc56ad3c7035224825 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 8 Nov 2017 19:16:03 +0100 Subject: [PATCH 05/13] cgroup: make use of unit_get_subtree_mask() where appropriate subtree_mask is own_mask | members_mask, let's make use of that to shorten a few things --- src/core/cgroup.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index dff7d1dddc..b717837c72 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1107,9 +1107,7 @@ CGroupMask unit_get_members_mask(Unit *u) { if (UNIT_DEREF(member->slice) != u) continue; - u->cgroup_members_mask |= - unit_get_own_mask(member) | - unit_get_members_mask(member); + u->cgroup_members_mask |= unit_get_subtree_mask(member); /* note that this calls ourselves again, for the children */ } } @@ -1127,7 +1125,7 @@ CGroupMask unit_get_siblings_mask(Unit *u) { if (UNIT_ISSET(u->slice)) return unit_get_members_mask(UNIT_DEREF(u->slice)); - return unit_get_own_mask(u) | unit_get_members_mask(u); + return unit_get_subtree_mask(u); } CGroupMask unit_get_subtree_mask(Unit *u) { From 57d6f7001966d2f10c4cf03b72f751be2fa09cf1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 8 Nov 2017 21:33:19 +0100 Subject: [PATCH 06/13] fileio: make use of DEFINE_TRIVIAL_CLEANUP_FUNC where it makes sense --- src/basic/fileio.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 0ae330541d..9e4c5af975 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -1527,9 +1527,7 @@ int mkdtemp_malloc(const char *template, char **ret) { return 0; } -static inline void funlockfilep(FILE **f) { - funlockfile(*f); -} +DEFINE_TRIVIAL_CLEANUP_FUNC(FILE*, funlockfile); int read_line(FILE *f, size_t limit, char **ret) { _cleanup_free_ char *buffer = NULL; From 92b5e6054254b1d6cf7fcb342720fe58a58d0d7e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 8 Nov 2017 21:38:51 +0100 Subject: [PATCH 07/13] conf-parser: simplify things a bit by using strextend() --- src/shared/conf-parser.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index c304ae3334..d1c73b6f2d 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -186,7 +186,6 @@ static int parse_line(const char* unit, assert(l); l = strstrip(l); - if (!*l) return 0; @@ -319,8 +318,8 @@ int config_parse(const char *unit, for (;;) { _cleanup_free_ char *buf = NULL; - char *l, *p, *c = NULL, *e; bool escaped = false; + char *l, *p, *e; r = read_line(f, LONG_LINE_MAX, &buf); if (r == 0) @@ -356,15 +355,13 @@ int config_parse(const char *unit, return -ENOBUFS; } - c = strappend(continuation, l); - if (!c) { + if (!strextend(&continuation, l, NULL)) { if (warn) log_oom(); return -ENOMEM; } - continuation = mfree(continuation); - p = c; + p = continuation; } else p = l; @@ -378,9 +375,7 @@ int config_parse(const char *unit, if (escaped) { *(e-1) = ' '; - if (c) - continuation = c; - else { + if (!continuation) { continuation = strdup(l); if (!continuation) { if (warn) @@ -405,13 +400,14 @@ int config_parse(const char *unit, §ion_ignored, p, userdata); - free(c); - if (r < 0) { if (warn) log_warning_errno(r, "%s:%u: Failed to parse file: %m", filename, line); return r; + } + + continuation = mfree(continuation); } return 0; From bcde742e78ac3b8e8ea348cfb022c820c11800e2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 9 Nov 2017 00:26:11 +0100 Subject: [PATCH 08/13] conf-parser: turn three bool function params into a flags fields This makes things more readable and fixes some issues with incorrect flag propagation between the various flavours of config_parse(). --- src/core/load-dropin.c | 11 +- src/core/load-fragment.c | 2 +- src/core/main.c | 2 +- src/coredump/coredump.c | 2 +- src/journal-remote/journal-remote.c | 2 +- src/journal-remote/journal-upload.c | 2 +- src/journal/journald-server.c | 2 +- src/login/logind.c | 2 +- src/network/netdev/netdev.c | 4 +- src/network/networkd-conf.c | 2 +- src/network/networkd-network.c | 2 +- src/nspawn/nspawn-settings.c | 4 +- src/resolve/resolved-conf.c | 2 +- src/shared/conf-parser.c | 104 +++++++++--------- src/shared/conf-parser.h | 18 +-- src/shared/install.c | 2 +- src/shared/sleep-config.c | 8 +- src/test/test-conf-parser.c | 2 +- src/timesync/timesyncd-conf.c | 2 +- .../tty-ask-password-agent.c | 2 +- src/udev/net/link-config.c | 2 +- 21 files changed, 88 insertions(+), 91 deletions(-) diff --git a/src/core/load-dropin.c b/src/core/load-dropin.c index 948d1bc248..c98ab4164f 100644 --- a/src/core/load-dropin.c +++ b/src/core/load-dropin.c @@ -154,12 +154,11 @@ int unit_load_dropin(Unit *u) { return log_oom(); } - STRV_FOREACH(f, u->dropin_paths) { - config_parse(u->id, *f, NULL, - UNIT_VTABLE(u)->sections, - config_item_perf_lookup, load_fragment_gperf_lookup, - false, false, false, u); - } + STRV_FOREACH(f, u->dropin_paths) + (void) config_parse(u->id, *f, NULL, + UNIT_VTABLE(u)->sections, + config_item_perf_lookup, load_fragment_gperf_lookup, + 0, u); u->dropin_mtime = now(CLOCK_REALTIME); diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 84f2931b63..53a95caeaf 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -4450,7 +4450,7 @@ static int load_from_path(Unit *u, const char *path) { r = config_parse(u->id, filename, f, UNIT_VTABLE(u)->sections, config_item_perf_lookup, load_fragment_gperf_lookup, - false, true, false, u); + CONFIG_PARSE_ALLOW_INCLUDE, u); if (r < 0) return r; } diff --git a/src/core/main.c b/src/core/main.c index 3e766f0645..96cac1cd9e 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -768,7 +768,7 @@ static int parse_config_file(void) { CONF_PATHS_NULSTR("systemd/system.conf.d") : CONF_PATHS_NULSTR("systemd/user.conf.d"); - config_parse_many_nulstr(fn, conf_dirs_nulstr, "Manager\0", config_item_table_lookup, items, false, NULL); + (void) config_parse_many_nulstr(fn, conf_dirs_nulstr, "Manager\0", config_item_table_lookup, items, CONFIG_PARSE_WARN, NULL); /* Traditionally "0" was used to turn off the default unit timeouts. Fix this up so that we used USEC_INFINITY * like everywhere else. */ diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 300d647903..ee258a1219 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -147,7 +147,7 @@ static int parse_config(void) { CONF_PATHS_NULSTR("systemd/coredump.conf.d"), "Coredump\0", config_item_table_lookup, items, - false, NULL); + CONFIG_PARSE_WARN, NULL); } static inline uint64_t storage_size_max(void) { diff --git a/src/journal-remote/journal-remote.c b/src/journal-remote/journal-remote.c index e045e9a842..90cd4447d7 100644 --- a/src/journal-remote/journal-remote.c +++ b/src/journal-remote/journal-remote.c @@ -1254,7 +1254,7 @@ static int parse_config(void) { return config_parse_many_nulstr(PKGSYSCONFDIR "/journal-remote.conf", CONF_PATHS_NULSTR("systemd/journal-remote.conf.d"), "Remote\0", config_item_table_lookup, items, - false, NULL); + CONFIG_PARSE_WARN, NULL); } static void help(void) { diff --git a/src/journal-remote/journal-upload.c b/src/journal-remote/journal-upload.c index ea264989ab..1e3e541998 100644 --- a/src/journal-remote/journal-upload.c +++ b/src/journal-remote/journal-upload.c @@ -543,7 +543,7 @@ static int parse_config(void) { return config_parse_many_nulstr(PKGSYSCONFDIR "/journal-upload.conf", CONF_PATHS_NULSTR("systemd/journal-upload.conf.d"), "Upload\0", config_item_table_lookup, items, - false, NULL); + CONFIG_PARSE_WARN, NULL); } static void help(void) { diff --git a/src/journal/journald-server.c b/src/journal/journald-server.c index 00ad168286..e719fae007 100644 --- a/src/journal/journald-server.c +++ b/src/journal/journald-server.c @@ -1398,7 +1398,7 @@ static int server_parse_config_file(Server *s) { CONF_PATHS_NULSTR("systemd/journald.conf.d"), "Journal\0", config_item_perf_lookup, journald_gperf_lookup, - false, s); + CONFIG_PARSE_WARN, s); } static int server_dispatch_sync(sd_event_source *es, usec_t t, void *userdata) { diff --git a/src/login/logind.c b/src/login/logind.c index 6046596684..cdd1710fd3 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -1072,7 +1072,7 @@ static int manager_parse_config_file(Manager *m) { CONF_PATHS_NULSTR("systemd/logind.conf.d"), "Login\0", config_item_perf_lookup, logind_gperf_lookup, - false, m); + CONFIG_PARSE_WARN, m); } static int manager_dispatch_reload_signal(sd_event_source *s, const struct signalfd_siginfo *si, void *userdata) { diff --git a/src/network/netdev/netdev.c b/src/network/netdev/netdev.c index 0e1a7d1335..2ec526e636 100644 --- a/src/network/netdev/netdev.c +++ b/src/network/netdev/netdev.c @@ -630,7 +630,7 @@ static int netdev_load_one(Manager *manager, const char *filename) { r = config_parse_many(filename, network_dirs, dropin_dirname, "Match\0NetDev\0", config_item_perf_lookup, network_netdev_gperf_lookup, - true, netdev_raw); + CONFIG_PARSE_WARN, netdev_raw); if (r < 0) return r; @@ -671,7 +671,7 @@ static int netdev_load_one(Manager *manager, const char *filename) { r = config_parse(NULL, filename, file, NETDEV_VTABLE(netdev)->sections, config_item_perf_lookup, network_netdev_gperf_lookup, - false, false, false, netdev); + CONFIG_PARSE_WARN, netdev); if (r < 0) return r; diff --git a/src/network/networkd-conf.c b/src/network/networkd-conf.c index 025662437b..25ec160df8 100644 --- a/src/network/networkd-conf.c +++ b/src/network/networkd-conf.c @@ -35,7 +35,7 @@ int manager_parse_config_file(Manager *m) { CONF_PATHS_NULSTR("systemd/networkd.conf.d"), "DHCP\0", config_item_perf_lookup, networkd_gperf_lookup, - false, m); + CONFIG_PARSE_WARN, m); } static const char* const duid_type_table[_DUID_TYPE_MAX] = { diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index 29f1586d13..3ed82ca3f9 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -280,7 +280,7 @@ static int network_load_one(Manager *manager, const char *filename) { "IPv6PrefixDelegation\0" "IPv6Prefix\0", config_item_perf_lookup, network_network_gperf_lookup, - false, network); + CONFIG_PARSE_WARN, network); if (r < 0) return r; diff --git a/src/nspawn/nspawn-settings.c b/src/nspawn/nspawn-settings.c index 285e22820f..5f9c1f5ea4 100644 --- a/src/nspawn/nspawn-settings.c +++ b/src/nspawn/nspawn-settings.c @@ -59,9 +59,7 @@ int settings_load(FILE *f, const char *path, Settings **ret) { "Network\0" "Files\0", config_item_perf_lookup, nspawn_gperf_lookup, - false, - false, - true, + CONFIG_PARSE_WARN, s); if (r < 0) return r; diff --git a/src/resolve/resolved-conf.c b/src/resolve/resolved-conf.c index 3cf4261ff0..39dc358a48 100644 --- a/src/resolve/resolved-conf.c +++ b/src/resolve/resolved-conf.c @@ -236,7 +236,7 @@ int manager_parse_config_file(Manager *m) { CONF_PATHS_NULSTR("systemd/resolved.conf.d"), "Resolve\0", config_item_perf_lookup, resolved_gperf_lookup, - false, m); + CONFIG_PARSE_WARN, m); if (r < 0) return r; diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index d1c73b6f2d..86cf71afac 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -121,17 +121,18 @@ int config_item_perf_lookup( } /* Run the user supplied parser for an assignment */ -static int next_assignment(const char *unit, - const char *filename, - unsigned line, - ConfigItemLookup lookup, - const void *table, - const char *section, - unsigned section_line, - const char *lvalue, - const char *rvalue, - bool relaxed, - void *userdata) { +static int next_assignment( + const char *unit, + const char *filename, + unsigned line, + ConfigItemLookup lookup, + const void *table, + const char *section, + unsigned section_line, + const char *lvalue, + const char *rvalue, + ConfigParseFlags flags, + void *userdata) { ConfigParserCallback func = NULL; int ltype = 0; @@ -157,26 +158,26 @@ static int next_assignment(const char *unit, } /* Warn about unknown non-extension fields. */ - if (!relaxed && !startswith(lvalue, "X-")) + if (!(flags & CONFIG_PARSE_RELAXED) && !startswith(lvalue, "X-")) log_syntax(unit, LOG_WARNING, filename, line, 0, "Unknown lvalue '%s' in section '%s'", lvalue, section); return 0; } /* Parse a variable assignment line */ -static int parse_line(const char* unit, - const char *filename, - unsigned line, - const char *sections, - ConfigItemLookup lookup, - const void *table, - bool relaxed, - bool allow_include, - char **section, - unsigned *section_line, - bool *section_ignored, - char *l, - void *userdata) { +static int parse_line( + const char* unit, + const char *filename, + unsigned line, + const char *sections, + ConfigItemLookup lookup, + const void *table, + ConfigParseFlags flags, + char **section, + unsigned *section_line, + bool *section_ignored, + char *l, + void *userdata) { char *e; @@ -204,7 +205,7 @@ static int parse_line(const char* unit, * * Support for them should be eventually removed. */ - if (!allow_include) { + if (!(flags & CONFIG_PARSE_ALLOW_INCLUDE)) { log_syntax(unit, LOG_ERR, filename, line, 0, ".include not allowed here. Ignoring."); return 0; } @@ -213,7 +214,7 @@ static int parse_line(const char* unit, if (!fn) return -ENOMEM; - return config_parse(unit, fn, NULL, sections, lookup, table, relaxed, false, false, userdata); + return config_parse(unit, fn, NULL, sections, lookup, table, flags, userdata); } if (*l == '[') { @@ -234,7 +235,7 @@ static int parse_line(const char* unit, if (sections && !nulstr_contains(sections, n)) { - if (!relaxed && !startswith(n, "X-")) + if (!(flags & CONFIG_PARSE_RELAXED) && !startswith(n, "X-")) log_syntax(unit, LOG_WARNING, filename, line, 0, "Unknown section '%s'. Ignoring.", n); free(n); @@ -253,7 +254,7 @@ static int parse_line(const char* unit, if (sections && !*section) { - if (!relaxed && !*section_ignored) + if (!(flags & CONFIG_PARSE_RELAXED) && !*section_ignored) log_syntax(unit, LOG_WARNING, filename, line, 0, "Assignment outside of section. Ignoring."); return 0; @@ -277,7 +278,7 @@ static int parse_line(const char* unit, *section_line, strstrip(l), strstrip(e), - relaxed, + flags, userdata); } @@ -288,15 +289,13 @@ int config_parse(const char *unit, const char *sections, ConfigItemLookup lookup, const void *table, - bool relaxed, - bool allow_include, - bool warn, + ConfigParseFlags flags, void *userdata) { _cleanup_free_ char *section = NULL, *continuation = NULL; _cleanup_fclose_ FILE *ours = NULL; unsigned line = 0, section_line = 0; - bool section_ignored = false, allow_bom = true; + bool section_ignored = false; int r; assert(filename); @@ -307,7 +306,7 @@ int config_parse(const char *unit, if (!f) { /* Only log on request, except for ENOENT, * since we return 0 to the caller. */ - if (warn || errno == ENOENT) + if ((flags & CONFIG_PARSE_WARN) || errno == ENOENT) log_full(errno == ENOENT ? LOG_DEBUG : LOG_ERR, "Failed to open configuration file '%s': %m", filename); return errno == ENOENT ? 0 : -errno; @@ -325,38 +324,38 @@ int config_parse(const char *unit, if (r == 0) break; if (r == -ENOBUFS) { - if (warn) + if (flags & CONFIG_PARSE_WARN) log_error_errno(r, "%s:%u: Line too long", filename, line); return r; } if (r < 0) { - if (warn) + if (CONFIG_PARSE_WARN) log_error_errno(r, "%s:%u: Error while reading configuration file: %m", filename, line); return r; } l = buf; - if (allow_bom) { + if (!(flags & CONFIG_PARSE_REFUSE_BOM)) { char *q; q = startswith(buf, UTF8_BYTE_ORDER_MARK); if (q) { l = q; - allow_bom = false; + flags |= CONFIG_PARSE_REFUSE_BOM; } } if (continuation) { if (strlen(continuation) + strlen(l) > LONG_LINE_MAX) { - if (warn) + if (flags & CONFIG_PARSE_WARN) log_error("%s:%u: Continuation line too long", filename, line); return -ENOBUFS; } if (!strextend(&continuation, l, NULL)) { - if (warn) + if (flags & CONFIG_PARSE_WARN) log_oom(); return -ENOMEM; } @@ -378,7 +377,7 @@ int config_parse(const char *unit, if (!continuation) { continuation = strdup(l); if (!continuation) { - if (warn) + if (flags & CONFIG_PARSE_WARN) log_oom(); return -ENOMEM; } @@ -393,15 +392,14 @@ int config_parse(const char *unit, sections, lookup, table, - relaxed, - allow_include, + flags, §ion, §ion_line, §ion_ignored, p, userdata); if (r < 0) { - if (warn) + if (flags & CONFIG_PARSE_WARN) log_warning_errno(r, "%s:%u: Failed to parse file: %m", filename, line); return r; @@ -419,20 +417,20 @@ static int config_parse_many_files( const char *sections, ConfigItemLookup lookup, const void *table, - bool relaxed, + ConfigParseFlags flags, void *userdata) { char **fn; int r; if (conf_file) { - r = config_parse(NULL, conf_file, NULL, sections, lookup, table, relaxed, false, true, userdata); + r = config_parse(NULL, conf_file, NULL, sections, lookup, table, flags, userdata); if (r < 0) return r; } STRV_FOREACH(fn, files) { - r = config_parse(NULL, *fn, NULL, sections, lookup, table, relaxed, false, true, userdata); + r = config_parse(NULL, *fn, NULL, sections, lookup, table, flags, userdata); if (r < 0) return r; } @@ -447,7 +445,7 @@ int config_parse_many_nulstr( const char *sections, ConfigItemLookup lookup, const void *table, - bool relaxed, + ConfigParseFlags flags, void *userdata) { _cleanup_strv_free_ char **files = NULL; @@ -457,8 +455,7 @@ int config_parse_many_nulstr( if (r < 0) return r; - return config_parse_many_files(conf_file, files, - sections, lookup, table, relaxed, userdata); + return config_parse_many_files(conf_file, files, sections, lookup, table, flags, userdata); } /* Parse each config file in the directories specified as strv. */ @@ -469,7 +466,7 @@ int config_parse_many( const char *sections, ConfigItemLookup lookup, const void *table, - bool relaxed, + ConfigParseFlags flags, void *userdata) { _cleanup_strv_free_ char **dropin_dirs = NULL; @@ -486,8 +483,7 @@ int config_parse_many( if (r < 0) return r; - return config_parse_many_files(conf_file, files, - sections, lookup, table, relaxed, userdata); + return config_parse_many_files(conf_file, files, sections, lookup, table, flags, userdata); } #define DEFINE_PARSER(type, vartype, conv_func) \ diff --git a/src/shared/conf-parser.h b/src/shared/conf-parser.h index ce1113485d..a270488c23 100644 --- a/src/shared/conf-parser.h +++ b/src/shared/conf-parser.h @@ -29,8 +29,14 @@ #include "log.h" #include "macro.h" -/* An abstract parser for simple, line based, shallow configuration - * files consisting of variable assignments only. */ +/* An abstract parser for simple, line based, shallow configuration files consisting of variable assignments only. */ + +typedef enum ConfigParseFlags { + CONFIG_PARSE_RELAXED = 1U << 0, + CONFIG_PARSE_ALLOW_INCLUDE = 1U << 1, + CONFIG_PARSE_WARN = 1U << 2, + CONFIG_PARSE_REFUSE_BOM = 1U << 3, +} ConfigParseFlags; /* Prototype for a parser for a specific configuration setting */ typedef int (*ConfigParserCallback)(const char *unit, @@ -91,9 +97,7 @@ int config_parse( const char *sections, /* nulstr */ ConfigItemLookup lookup, const void *table, - bool relaxed, - bool allow_include, - bool warn, + ConfigParseFlags flags, void *userdata); int config_parse_many_nulstr( @@ -102,7 +106,7 @@ int config_parse_many_nulstr( const char *sections, /* nulstr */ ConfigItemLookup lookup, const void *table, - bool relaxed, + ConfigParseFlags flags, void *userdata); int config_parse_many( @@ -112,7 +116,7 @@ int config_parse_many( const char *sections, /* nulstr */ ConfigItemLookup lookup, const void *table, - bool relaxed, + ConfigParseFlags flags, void *userdata); /* Generic parsers */ diff --git a/src/shared/install.c b/src/shared/install.c index ed0c4a5a11..d122aebc09 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1308,7 +1308,7 @@ static int unit_file_load( r = config_parse(info->name, path, f, NULL, config_item_table_lookup, items, - true, true, false, info); + CONFIG_PARSE_RELAXED|CONFIG_PARSE_ALLOW_INCLUDE, info); if (r < 0) return log_debug_errno(r, "Failed to parse %s: %m", info->name); diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index 8c1624ff46..b4c3037f0c 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -58,10 +58,10 @@ int parse_sleep_config(const char *verb, char ***_modes, char ***_states) { {} }; - config_parse_many_nulstr(PKGSYSCONFDIR "/sleep.conf", - CONF_PATHS_NULSTR("systemd/sleep.conf.d"), - "Sleep\0", config_item_table_lookup, items, - false, NULL); + (void) config_parse_many_nulstr(PKGSYSCONFDIR "/sleep.conf", + CONF_PATHS_NULSTR("systemd/sleep.conf.d"), + "Sleep\0", config_item_table_lookup, items, + CONFIG_PARSE_WARN, NULL); if (streq(verb, "suspend")) { /* empty by default */ diff --git a/src/test/test-conf-parser.c b/src/test/test-conf-parser.c index 7a7de98bec..2974d533c3 100644 --- a/src/test/test-conf-parser.c +++ b/src/test/test-conf-parser.c @@ -311,7 +311,7 @@ static void test_config_parse(unsigned i, const char *s) { r = config_parse(NULL, name, f, "Section\0", config_item_table_lookup, items, - false, false, true, NULL); + CONFIG_PARSE_WARN, NULL); switch (i) { case 0 ... 3: diff --git a/src/timesync/timesyncd-conf.c b/src/timesync/timesyncd-conf.c index b62e20c287..333f81a948 100644 --- a/src/timesync/timesyncd-conf.c +++ b/src/timesync/timesyncd-conf.c @@ -114,7 +114,7 @@ int manager_parse_config_file(Manager *m) { CONF_PATHS_NULSTR("systemd/timesyncd.conf.d"), "Time\0", config_item_perf_lookup, timesyncd_gperf_lookup, - false, m); + CONFIG_PARSE_WARN, m); if (r < 0) return r; diff --git a/src/tty-ask-password-agent/tty-ask-password-agent.c b/src/tty-ask-password-agent/tty-ask-password-agent.c index 495ae464b4..9dd7ea1811 100644 --- a/src/tty-ask-password-agent/tty-ask-password-agent.c +++ b/src/tty-ask-password-agent/tty-ask-password-agent.c @@ -310,7 +310,7 @@ static int parse_password(const char *filename, char **wall) { r = config_parse(NULL, filename, NULL, NULL, config_item_table_lookup, items, - true, false, true, NULL); + CONFIG_PARSE_RELAXED|CONFIG_PARSE_WARN, NULL); if (r < 0) return r; diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c index a5f3b1a1b0..fcbadf3e79 100644 --- a/src/udev/net/link-config.c +++ b/src/udev/net/link-config.c @@ -175,7 +175,7 @@ static int load_link(link_config_ctx *ctx, const char *filename) { r = config_parse(NULL, filename, file, "Match\0Link\0Ethernet\0", config_item_perf_lookup, link_config_gperf_lookup, - false, false, true, link); + CONFIG_PARSE_WARN, link); if (r < 0) return r; else From 78f3c4bca5e1c454248fb915653bcefa5b59ac49 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 9 Nov 2017 09:26:13 +0100 Subject: [PATCH 09/13] conf-parser: reindent some strangely indented function headers --- src/shared/conf-parser.c | 42 +++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c index 86cf71afac..10a26d45aa 100644 --- a/src/shared/conf-parser.c +++ b/src/shared/conf-parser.c @@ -559,16 +559,17 @@ int config_parse_iec_size(const char* unit, return 0; } -int config_parse_si_size(const char* unit, - const char *filename, - unsigned line, - const char *section, - unsigned section_line, - const char *lvalue, - int ltype, - const char *rvalue, - void *data, - void *userdata) { +int config_parse_si_size( + const char* unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *lvalue, + int ltype, + const char *rvalue, + void *data, + void *userdata) { size_t *sz = data; uint64_t v; @@ -589,16 +590,17 @@ int config_parse_si_size(const char* unit, return 0; } -int config_parse_iec_uint64(const char* unit, - const char *filename, - unsigned line, - const char *section, - unsigned section_line, - const char *lvalue, - int ltype, - const char *rvalue, - void *data, - void *userdata) { +int config_parse_iec_uint64( + const char* unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *lvalue, + int ltype, + const char *rvalue, + void *data, + void *userdata) { uint64_t *bytes = data; int r; From 7546145e26e4feecf0994d84e888d7da9c47424b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 9 Nov 2017 11:12:47 +0100 Subject: [PATCH 10/13] string-util: add delete_trailing_chars() and skip_leading_chars() helpers And let's port over a couple of users to the new APIs. --- src/basic/string-util.c | 31 +++++++++++++++++++++- src/basic/string-util.h | 12 +++++++++ src/basic/unit-name.c | 9 ++----- src/core/cgroup.c | 8 +++--- src/libudev/libudev-private.h | 1 - src/libudev/libudev-util.c | 11 -------- src/test/test-string-util.c | 46 ++++++++++++++++++++++++++++++--- src/udev/udev-rules.c | 2 +- src/udev/udevadm-test-builtin.c | 2 +- src/udev/udevadm-test.c | 2 +- 10 files changed, 92 insertions(+), 32 deletions(-) diff --git a/src/basic/string-util.c b/src/basic/string-util.c index 6fb4134ae9..be2613ca9e 100644 --- a/src/basic/string-util.c +++ b/src/basic/string-util.c @@ -278,6 +278,9 @@ char *strjoin_real(const char *x, ...) { char *strstrip(char *s) { char *e; + if (!s) + return NULL; + /* Drops trailing whitespace. Modifies the string in * place. Returns pointer to first non-space character */ @@ -295,7 +298,13 @@ char *strstrip(char *s) { char *delete_chars(char *s, const char *bad) { char *f, *t; - /* Drops all whitespace, regardless where in the string */ + /* Drops all specified bad characters, regardless where in the string */ + + if (!s) + return NULL; + + if (!bad) + bad = WHITESPACE; for (f = s, t = s; *f; f++) { if (strchr(bad, *f)) @@ -309,6 +318,26 @@ char *delete_chars(char *s, const char *bad) { return s; } +char *delete_trailing_chars(char *s, const char *bad) { + char *p, *c = s; + + /* Drops all specified bad characters, at the end of the string */ + + if (!s) + return NULL; + + if (!bad) + bad = WHITESPACE; + + for (p = s; *p; p++) + if (!strchr(bad, *p)) + c = p + 1; + + *c = 0; + + return s; +} + char *truncate_nl(char *s) { assert(s); diff --git a/src/basic/string-util.h b/src/basic/string-util.h index 4c94b182c1..d2040ebd12 100644 --- a/src/basic/string-util.h +++ b/src/basic/string-util.h @@ -133,8 +133,20 @@ char *strjoin_real(const char *x, ...) _sentinel_; char *strstrip(char *s); char *delete_chars(char *s, const char *bad); +char *delete_trailing_chars(char *s, const char *bad); char *truncate_nl(char *s); +static inline char *skip_leading_chars(const char *s, const char *bad) { + + if (!s) + return NULL; + + if (!bad) + bad = WHITESPACE; + + return (char*) s + strspn(s, bad); +} + char ascii_tolower(char x); char *ascii_strlower(char *s); char *ascii_strlower_n(char *s, size_t n); diff --git a/src/basic/unit-name.c b/src/basic/unit-name.c index ba9928375e..27ce432197 100644 --- a/src/basic/unit-name.c +++ b/src/basic/unit-name.c @@ -382,19 +382,14 @@ int unit_name_path_escape(const char *f, char **ret) { if (STR_IN_SET(p, "/", "")) s = strdup("-"); else { - char *e; - if (!path_is_safe(p)) return -EINVAL; /* Truncate trailing slashes */ - e = endswith(p, "/"); - if (e) - *e = 0; + delete_trailing_chars(p, "/"); /* Truncate leading slashes */ - if (p[0] == '/') - p++; + p = skip_leading_chars(p, "/"); s = unit_name_escape(p); } diff --git a/src/core/cgroup.c b/src/core/cgroup.c index b717837c72..d81d10a2df 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1944,11 +1944,9 @@ int manager_setup_cgroup(Manager *m) { if (e) *e = 0; - /* And make sure to store away the root value without trailing - * slash, even for the root dir, so that we can easily prepend - * it everywhere. */ - while ((e = endswith(m->cgroup_root, "/"))) - *e = 0; + /* And make sure to store away the root value without trailing slash, even for the root dir, so that we can + * easily prepend it everywhere. */ + delete_trailing_chars(m->cgroup_root, "/"); /* 2. Show data */ r = cg_get_path(SYSTEMD_CGROUP_CONTROLLER, m->cgroup_root, NULL, &path); diff --git a/src/libudev/libudev-private.h b/src/libudev/libudev-private.h index 52c5075110..818da8465e 100644 --- a/src/libudev/libudev-private.h +++ b/src/libudev/libudev-private.h @@ -138,7 +138,6 @@ int udev_queue_export_device_finished(struct udev_queue_export *udev_queue_expor #define UDEV_ALLOWED_CHARS_INPUT "/ $%?," int util_log_priority(const char *priority); size_t util_path_encode(const char *src, char *dest, size_t size); -void util_remove_trailing_chars(char *path, char c); int util_replace_whitespace(const char *str, char *to, size_t len); int util_replace_chars(char *str, const char *white); unsigned int util_string_hash32(const char *key); diff --git a/src/libudev/libudev-util.c b/src/libudev/libudev-util.c index 1d73d8f090..ae809d85e1 100644 --- a/src/libudev/libudev-util.c +++ b/src/libudev/libudev-util.c @@ -150,17 +150,6 @@ size_t util_path_encode(const char *src, char *dest, size_t size) return j; } -void util_remove_trailing_chars(char *path, char c) -{ - size_t len; - - if (path == NULL) - return; - len = strlen(path); - while (len > 0 && path[len-1] == c) - path[--len] = '\0'; -} - /* * Copy from 'str' to 'to', while removing all leading and trailing whitespace, * and replacing each run of consecutive whitespace with a single underscore. diff --git a/src/test/test-string-util.c b/src/test/test-string-util.c index 604701ff7a..5d97080d2c 100644 --- a/src/test/test-string-util.c +++ b/src/test/test-string-util.c @@ -288,11 +288,47 @@ static void test_endswith_no_case(void) { } static void test_delete_chars(void) { - char *r; - char input[] = " hello, waldo. abc"; + char *s, input[] = " hello, waldo. abc"; - r = delete_chars(input, WHITESPACE); - assert_se(streq(r, "hello,waldo.abc")); + s = delete_chars(input, WHITESPACE); + assert_se(streq(s, "hello,waldo.abc")); + assert_se(s == input); +} + +static void test_delete_trailing_chars(void) { + + char *s, + input1[] = " \n \r k \n \r ", + input2[] = "kkkkthiskkkiskkkaktestkkk", + input3[] = "abcdef"; + + s = delete_trailing_chars(input1, WHITESPACE); + assert_se(streq(s, " \n \r k")); + assert_se(s == input1); + + s = delete_trailing_chars(input2, "kt"); + assert_se(streq(s, "kkkkthiskkkiskkkaktes")); + assert_se(s == input2); + + s = delete_trailing_chars(input3, WHITESPACE); + assert_se(streq(s, "abcdef")); + assert_se(s == input3); + + s = delete_trailing_chars(input3, "fe"); + assert_se(streq(s, "abcd")); + assert_se(s == input3); +} + +static void test_skip_leading_chars(void) { + char input1[] = " \n \r k \n \r ", + input2[] = "kkkkthiskkkiskkkaktestkkk", + input3[] = "abcdef"; + + assert_se(streq(skip_leading_chars(input1, WHITESPACE), "k \n \r ")); + assert_se(streq(skip_leading_chars(input2, "k"), "thiskkkiskkkaktestkkk")); + assert_se(streq(skip_leading_chars(input2, "tk"), "hiskkkiskkkaktestkkk")); + assert_se(streq(skip_leading_chars(input3, WHITESPACE), "abcdef")); + assert_se(streq(skip_leading_chars(input3, "bcaef"), "def")); } static void test_in_charset(void) { @@ -361,6 +397,8 @@ int main(int argc, char *argv[]) { test_endswith(); test_endswith_no_case(); test_delete_chars(); + test_delete_trailing_chars(); + test_skip_leading_chars(); test_in_charset(); test_split_pair(); test_first_word(); diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 9aaec72baf..b061210c7c 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -1967,7 +1967,7 @@ void udev_rules_apply_to_event(struct udev_rules *rules, } else { int count; - util_remove_trailing_chars(result, '\n'); + delete_trailing_chars(result, "\n"); if (IN_SET(esc, ESCAPE_UNSET, ESCAPE_REPLACE)) { count = util_replace_chars(result, UDEV_ALLOWED_CHARS_INPUT); if (count > 0) diff --git a/src/udev/udevadm-test-builtin.c b/src/udev/udevadm-test-builtin.c index b5662be5c2..3bcd09061a 100644 --- a/src/udev/udevadm-test-builtin.c +++ b/src/udev/udevadm-test-builtin.c @@ -85,7 +85,7 @@ static int adm_builtin(struct udev *udev, int argc, char *argv[]) { strscpyl(filename, sizeof(filename), "/sys", syspath, NULL); else strscpy(filename, sizeof(filename), syspath); - util_remove_trailing_chars(filename, '/'); + delete_trailing_chars(filename, "/"); dev = udev_device_new_from_syspath(udev, filename); if (dev == NULL) { diff --git a/src/udev/udevadm-test.c b/src/udev/udevadm-test.c index e8ffe2f309..b180e24369 100644 --- a/src/udev/udevadm-test.c +++ b/src/udev/udevadm-test.c @@ -116,7 +116,7 @@ static int adm_test(struct udev *udev, int argc, char *argv[]) { strscpyl(filename, sizeof(filename), "/sys", syspath, NULL); else strscpy(filename, sizeof(filename), syspath); - util_remove_trailing_chars(filename, '/'); + delete_trailing_chars(filename, "/"); dev = udev_device_new_from_synthetic_event(udev, filename, action); if (dev == NULL) { From 800f478c2fa569a97b38fae6e2c7ae5d077085fd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 9 Nov 2017 11:13:36 +0100 Subject: [PATCH 11/13] core: downgrade a log message from error to warning Messages that do not indicate a failing operation, but where we continue operation should be at LOG_WARN, not at LOG_ERR. --- src/core/load-dropin.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/load-dropin.c b/src/core/load-dropin.c index c98ab4164f..062060a3b3 100644 --- a/src/core/load-dropin.c +++ b/src/core/load-dropin.c @@ -117,8 +117,8 @@ static int process_deps(Unit *u, UnitDependency dependency, const char *dir_suff r = unit_add_dependency_by_name(u, dependency, entry, *p, true, UNIT_DEPENDENCY_FILE); if (r < 0) - log_unit_error_errno(u, r, "cannot add %s dependency on %s, ignoring: %m", - unit_dependency_to_string(dependency), entry); + log_unit_warning_errno(u, r, "Cannot add %s dependency on %s, ignoring: %m", + unit_dependency_to_string(dependency), entry); } return 0; From 02638280390d75f25efca683c5abd57a65c5a16f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 9 Nov 2017 15:29:34 +0100 Subject: [PATCH 12/13] core: rework the Delegate= unit file setting to take a list of controller names Previously it was not possible to select which controllers to enable for a unit where Delegate=yes was set, as all controllers were enabled. With this change, this is made configurable, and thus delegation units can pick specifically what they want to manage themselves, and what they don't care about. --- src/core/cgroup.c | 52 +++++++++++------ src/core/cgroup.h | 4 +- src/core/dbus-cgroup.c | 82 +++++++++++++++++++++++++++ src/core/load-fragment-gperf.gperf.m4 | 2 +- src/core/load-fragment.c | 61 ++++++++++++++++++++ src/core/load-fragment.h | 1 + src/core/unit.c | 21 +++++-- src/shared/bus-unit-util.c | 47 ++++++++++++++- 8 files changed, 247 insertions(+), 23 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index d81d10a2df..6872da1f89 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -209,6 +209,16 @@ void cgroup_context_dump(CGroupContext *c, FILE* f, const char *prefix) { prefix, cgroup_device_policy_to_string(c->device_policy), prefix, yes_no(c->delegate)); + if (c->delegate) { + _cleanup_free_ char *t = NULL; + + (void) cg_mask_to_string(c->delegate_controllers, &t); + + fprintf(f, "%sDelegateController=%s\n", + prefix, + strempty(t)); + } + LIST_FOREACH(device_allow, a, c->device_allow) fprintf(f, "%sDeviceAllow=%s %s%s%s\n", @@ -1062,37 +1072,47 @@ CGroupMask unit_get_own_mask(Unit *u) { if (!c) return 0; - /* If delegation is turned on, then turn on all cgroups, - * unless we are on the legacy hierarchy and the process we - * fork into it is known to drop privileges, and hence - * shouldn't get access to the controllers. - * - * Note that on the unified hierarchy it is safe to delegate - * controllers to unprivileged services. */ + return cgroup_context_get_mask(c); +} - if (c->delegate) { +CGroupMask unit_get_delegate_mask(Unit *u) { + CGroupContext *c; + + /* If delegation is turned on, then turn on selected controllers, unless we are on the legacy hierarchy and the + * process we fork into is known to drop privileges, and hence shouldn't get access to the controllers. + * + * Note that on the unified hierarchy it is safe to delegate controllers to unprivileged services. */ + + if (u->type == UNIT_SLICE) + return 0; + + c = unit_get_cgroup_context(u); + if (!c) + return 0; + + if (!c->delegate) + return 0; + + if (cg_all_unified() <= 0) { ExecContext *e; e = unit_get_exec_context(u); - if (!e || - exec_context_maintains_privileges(e) || - cg_all_unified() > 0) - return _CGROUP_MASK_ALL; + if (e && !exec_context_maintains_privileges(e)) + return 0; } - return cgroup_context_get_mask(c); + return c->delegate_controllers; } CGroupMask unit_get_members_mask(Unit *u) { assert(u); - /* Returns the mask of controllers all of the unit's children - * require, merged */ + /* Returns the mask of controllers all of the unit's children require, merged */ if (u->cgroup_members_mask_valid) return u->cgroup_members_mask; - u->cgroup_members_mask = 0; + u->cgroup_members_mask = unit_get_delegate_mask(u); if (u->type == UNIT_SLICE) { void *v; diff --git a/src/core/cgroup.h b/src/core/cgroup.h index 65245fbc43..a75be38044 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -126,6 +126,7 @@ struct CGroupContext { uint64_t tasks_max; bool delegate; + CGroupMask delegate_controllers; }; /* Used when querying IP accounting data */ @@ -153,8 +154,9 @@ void cgroup_context_free_blockio_device_weight(CGroupContext *c, CGroupBlockIODe void cgroup_context_free_blockio_device_bandwidth(CGroupContext *c, CGroupBlockIODeviceBandwidth *b); CGroupMask unit_get_own_mask(Unit *u); -CGroupMask unit_get_siblings_mask(Unit *u); +CGroupMask unit_get_delegate_mask(Unit *u); CGroupMask unit_get_members_mask(Unit *u); +CGroupMask unit_get_siblings_mask(Unit *u); CGroupMask unit_get_subtree_mask(Unit *u); CGroupMask unit_get_target_mask(Unit *u); diff --git a/src/core/dbus-cgroup.c b/src/core/dbus-cgroup.c index a99d727f4d..bef70b6f84 100644 --- a/src/core/dbus-cgroup.c +++ b/src/core/dbus-cgroup.c @@ -32,6 +32,42 @@ static BUS_DEFINE_PROPERTY_GET_ENUM(property_get_cgroup_device_policy, cgroup_device_policy, CGroupDevicePolicy); +static int property_get_delegate_controllers( + sd_bus *bus, + const char *path, + const char *interface, + const char *property, + sd_bus_message *reply, + void *userdata, + sd_bus_error *error) { + + CGroupContext *c = userdata; + CGroupController cc; + int r; + + assert(bus); + assert(reply); + assert(c); + + if (!c->delegate) + return sd_bus_message_append(reply, "as", 0); + + r = sd_bus_message_open_container(reply, 'a', "s"); + if (r < 0) + return r; + + for (cc = 0; cc < _CGROUP_CONTROLLER_MAX; cc++) { + if ((c->delegate_controllers & CGROUP_CONTROLLER_TO_MASK(cc)) == 0) + continue; + + r = sd_bus_message_append(reply, "s", cgroup_controller_to_string(cc)); + if (r < 0) + return r; + } + + return sd_bus_message_close_container(reply); +} + static int property_get_io_device_weight( sd_bus *bus, const char *path, @@ -255,6 +291,7 @@ static int property_get_ip_address_access( const sd_bus_vtable bus_cgroup_vtable[] = { SD_BUS_VTABLE_START(0), SD_BUS_PROPERTY("Delegate", "b", bus_property_get_bool, offsetof(CGroupContext, delegate), 0), + SD_BUS_PROPERTY("DelegateControllers", "as", property_get_delegate_controllers, 0, 0), SD_BUS_PROPERTY("CPUAccounting", "b", bus_property_get_bool, offsetof(CGroupContext, cpu_accounting), 0), SD_BUS_PROPERTY("CPUWeight", "t", NULL, offsetof(CGroupContext, cpu_weight), 0), SD_BUS_PROPERTY("StartupCPUWeight", "t", NULL, offsetof(CGroupContext, startup_cpu_weight), 0), @@ -315,9 +352,54 @@ static int bus_cgroup_set_transient_property( if (mode != UNIT_CHECK) { c->delegate = b; + c->delegate_controllers = b ? _CGROUP_MASK_ALL : 0; + unit_write_drop_in_private(u, mode, name, b ? "Delegate=yes" : "Delegate=no"); } + return 1; + + } else if (streq(name, "DelegateControllers")) { + CGroupMask mask = 0; + + r = sd_bus_message_enter_container(message, 'a', "s"); + if (r < 0) + return r; + + for (;;) { + CGroupController cc; + const char *t; + + r = sd_bus_message_read(message, "s", &t); + if (r < 0) + return r; + if (r == 0) + break; + + cc = cgroup_controller_from_string(t); + if (cc < 0) + return sd_bus_error_set_errnof(error, EINVAL, "Unknown cgroup contoller '%s'", t); + + mask |= CGROUP_CONTROLLER_TO_MASK(cc); + } + + r = sd_bus_message_exit_container(message); + if (r < 0) + return r; + + if (mode != UNIT_CHECK) { + _cleanup_free_ char *t = NULL; + + r = cg_mask_to_string(mask, &t); + if (r < 0) + return r; + + c->delegate = true; + c->delegate_controllers |= mask; + + unit_write_drop_in_private_format(u, mode, name, "Delegate=%s", t); + } + return 1; } diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4 index 0f6372e8a3..42c2dbb9e4 100644 --- a/src/core/load-fragment-gperf.gperf.m4 +++ b/src/core/load-fragment-gperf.gperf.m4 @@ -174,7 +174,7 @@ $1.BlockIOReadBandwidth, config_parse_blockio_bandwidth, 0, $1.BlockIOWriteBandwidth, config_parse_blockio_bandwidth, 0, offsetof($1, cgroup_context) $1.TasksAccounting, config_parse_bool, 0, offsetof($1, cgroup_context.tasks_accounting) $1.TasksMax, config_parse_tasks_max, 0, offsetof($1, cgroup_context.tasks_max) -$1.Delegate, config_parse_bool, 0, offsetof($1, cgroup_context.delegate) +$1.Delegate, config_parse_delegate, 0, offsetof($1, cgroup_context) $1.IPAccounting, config_parse_bool, 0, offsetof($1, cgroup_context.ip_accounting) $1.IPAddressAllow, config_parse_ip_address_access, 0, offsetof($1, cgroup_context.ip_address_allow) $1.IPAddressDeny, config_parse_ip_address_access, 0, offsetof($1, cgroup_context.ip_address_deny) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 53a95caeaf..02d507f7b2 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -3202,6 +3202,67 @@ int config_parse_tasks_max( return 0; } +int config_parse_delegate( + const char *unit, + const char *filename, + unsigned line, + const char *section, + unsigned section_line, + const char *lvalue, + int ltype, + const char *rvalue, + void *data, + void *userdata) { + + CGroupContext *c = data; + int r; + + /* We either accept a boolean value, which may be used to turn on delegation for all controllers, or turn it + * off for all. Or it takes a list of controller names, in which case we add the specified controllers to the + * mask to delegate. */ + + r = parse_boolean(rvalue); + if (r < 0) { + const char *p = rvalue; + CGroupMask mask = 0; + + for (;;) { + _cleanup_free_ char *word = NULL; + CGroupController cc; + + r = extract_first_word(&p, &word, NULL, EXTRACT_QUOTES); + if (r == 0) + break; + if (r == -ENOMEM) + return log_oom(); + if (r < 0) { + log_syntax(unit, LOG_ERR, filename, line, r, "Invalid syntax, ignoring: %s", rvalue); + return r; + } + + cc = cgroup_controller_from_string(word); + if (cc < 0) { + log_syntax(unit, LOG_ERR, filename, line, r, "Invalid controller name '%s', ignoring", rvalue); + continue; + } + + mask |= CGROUP_CONTROLLER_TO_MASK(cc); + } + + c->delegate = true; + c->delegate_controllers |= mask; + + } else if (r > 0) { + c->delegate = true; + c->delegate_controllers = _CGROUP_MASK_ALL; + } else { + c->delegate = false; + c->delegate_controllers = 0; + } + + return 0; +} + int config_parse_device_allow( const char *unit, const char *filename, diff --git a/src/core/load-fragment.h b/src/core/load-fragment.h index 1353b0a9d0..0bd6ec15d6 100644 --- a/src/core/load-fragment.h +++ b/src/core/load-fragment.h @@ -85,6 +85,7 @@ int config_parse_cpu_weight(const char *unit, const char *filename, unsigned lin int config_parse_cpu_shares(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata); int config_parse_memory_limit(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata); int config_parse_tasks_max(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata); +int config_parse_delegate(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata); int config_parse_device_policy(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata); int config_parse_device_allow(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata); int config_parse_io_weight(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata); diff --git a/src/core/unit.c b/src/core/unit.c index 7d95f9db0b..e2446804e7 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1057,8 +1057,9 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) { timespan[FORMAT_TIMESPAN_MAX]; Unit *following; _cleanup_set_free_ Set *following_set = NULL; - int r; const char *n; + CGroupMask m; + int r; assert(u); assert(u->type >= 0); @@ -1105,11 +1106,23 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) { if (u->cgroup_realized_mask != 0) { _cleanup_free_ char *s = NULL; (void) cg_mask_to_string(u->cgroup_realized_mask, &s); - fprintf(f, "%s\tCGroup mask: %s\n", prefix, strnull(s)); + fprintf(f, "%s\tCGroup realized mask: %s\n", prefix, strnull(s)); } - if (u->cgroup_members_mask != 0) { + if (u->cgroup_enabled_mask != 0) { _cleanup_free_ char *s = NULL; - (void) cg_mask_to_string(u->cgroup_members_mask, &s); + (void) cg_mask_to_string(u->cgroup_enabled_mask, &s); + fprintf(f, "%s\tCGroup enabled mask: %s\n", prefix, strnull(s)); + } + m = unit_get_own_mask(u); + if (m != 0) { + _cleanup_free_ char *s = NULL; + (void) cg_mask_to_string(m, &s); + fprintf(f, "%s\tCGroup own mask: %s\n", prefix, strnull(s)); + } + m = unit_get_members_mask(u); + if (m != 0) { + _cleanup_free_ char *s = NULL; + (void) cg_mask_to_string(m, &s); fprintf(f, "%s\tCGroup members mask: %s\n", prefix, strnull(s)); } diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index 57de9975c7..529bc62886 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -183,6 +183,51 @@ int bus_append_unit_property_assignment(sd_bus_message *m, const char *assignmen r = sd_bus_message_append(m, "sv", field, "t", bytes); goto finish; + + } else if (streq(field, "Delegate")) { + + r = parse_boolean(eq); + if (r < 0) { + const char *p = eq; + + r = sd_bus_message_append(m, "s", "DelegateControllers"); + if (r < 0) + goto finish; + + r = sd_bus_message_open_container(m, 'v', "as"); + if (r < 0) + goto finish; + + r = sd_bus_message_open_container(m, 'a', "s"); + if (r < 0) + goto finish; + + for (;;) { + _cleanup_free_ char *word = NULL; + + r = extract_first_word(&p, &word, NULL, EXTRACT_QUOTES); + if (r == 0) + break; + if (r == -ENOMEM) + return log_oom(); + if (r < 0) + return log_error_errno(r, "Invalid syntax: %s", eq); + + r = sd_bus_message_append(m, "s", word); + if (r < 0) + goto finish; + } + + r = sd_bus_message_close_container(m); + if (r < 0) + goto finish; + + r = sd_bus_message_close_container(m); + } else + r = sd_bus_message_append(m, "sv", "Delegate", "b", r); + + goto finish; + } else if (streq(field, "TasksMax")) { uint64_t t; @@ -238,7 +283,7 @@ int bus_append_unit_property_assignment(sd_bus_message *m, const char *assignmen "TasksAccounting", "IPAccounting", "SendSIGHUP", "SendSIGKILL", "WakeSystem", "DefaultDependencies", "IgnoreSIGPIPE", "TTYVHangup", "TTYReset", "TTYVTDisallocate", "RemainAfterExit", "PrivateTmp", "PrivateDevices", "PrivateNetwork", "PrivateUsers", - "NoNewPrivileges", "SyslogLevelPrefix", "Delegate", "RemainAfterElapse", + "NoNewPrivileges", "SyslogLevelPrefix", "RemainAfterElapse", "MemoryDenyWriteExecute", "RestrictRealtime", "DynamicUser", "RemoveIPC", "ProtectKernelTunables", "ProtectKernelModules", "ProtectControlGroups", "MountAPIVFS", "CPUSchedulingResetOnFork", "LockPersonality")) { From a9f01ad1bf5bac3980d34324e42dab2b5e4f86ef Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 9 Nov 2017 15:31:37 +0100 Subject: [PATCH 13/13] man: document the new Delegate= syntax --- man/systemd.resource-control.xml | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/man/systemd.resource-control.xml b/man/systemd.resource-control.xml index 62dad57748..761a6056de 100644 --- a/man/systemd.resource-control.xml +++ b/man/systemd.resource-control.xml @@ -711,13 +711,30 @@ Delegate= - Turns on delegation of further resource control - partitioning to processes of the unit. For unprivileged - services (i.e. those using the User= - setting), this allows processes to create a subhierarchy - beneath its control group path. For privileged services and - scopes, this ensures the processes will have all control - group controllers enabled. + Turns on delegation of further resource control partitioning to processes of the unit. Units where this + is enabled may create and manage their own private subhierarchy of control groups below the control group of + the unit itself. For unprivileged services (i.e. those using the User= setting) the unit's + control group will be made accessible to the relevant user. When enabled the service manager will refrain + from manipulating control groups or moving processes below the unit's control group, so that a clear concept + of ownership is established: the control group tree above the unit's control group (i.e. towards the root + control group) is owned and managed by the service manager of the host, while the control group tree below + the unit's control group is owned and managed by the unit itself. Takes either a boolean argument or a list + of control group controller names. If true, delegation is turned on, and all supported controllers are + enabled for the unit, making them available to the unit's processes for management. If false, delegation is + turned off entirely (and no additional controllers are enabled). If set to a list of controllers, delegation + is turned on, and the specified controllers are enabled for the unit. Note that assigning the empty string + will enable delegation, but not enable any additional controllers. Defaults to false. + + Note that controller delegation to less privileged code is only safe on the unified control group + hierarchy. Accordingly, access to the specified controllers will not be granted to unprivileged services on + the legacy hierarchy, even when requested. + + The following controller names may be specified: , , + , , , , + . Not all of these controllers are available on all kernels however, and some are + specific to the unified hierarchy while others are specific to the legacy hierarchy. Also note that the + kernel might support further controllers, which aren't covered here yet as delegation is either not supported + at all for them or not defined cleanly.