From 68ac0d05a97f43ad2ded5667c4c499f07def6728 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 17 Nov 2017 13:02:14 +0100 Subject: [PATCH 01/13] =?UTF-8?q?cgroup:=20move=20cgroup=20controller=20na?= =?UTF-8?q?mes=20def.h=20=E2=86=92=20cgroup-util.h?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These definitions are clearly cgroup specific, hence let's move them out of def.h --- src/basic/cgroup-util.h | 4 ++++ src/basic/def.h | 4 ---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h index c9666075ec..df3d01e4ef 100644 --- a/src/basic/cgroup-util.h +++ b/src/basic/cgroup-util.h @@ -32,6 +32,10 @@ #include "macro.h" #include "set.h" +#define SYSTEMD_CGROUP_CONTROLLER_LEGACY "name=systemd" +#define SYSTEMD_CGROUP_CONTROLLER_HYBRID "name=unified" +#define SYSTEMD_CGROUP_CONTROLLER "_systemd" + /* An enum of well known cgroup controllers */ typedef enum CGroupController { CGROUP_CONTROLLER_CPU, diff --git a/src/basic/def.h b/src/basic/def.h index b0bd1dfa1c..77ab735aed 100644 --- a/src/basic/def.h +++ b/src/basic/def.h @@ -37,10 +37,6 @@ /* The default value for the net.unix.max_dgram_qlen sysctl */ #define DEFAULT_UNIX_MAX_DGRAM_QLEN 512UL -#define SYSTEMD_CGROUP_CONTROLLER_LEGACY "name=systemd" -#define SYSTEMD_CGROUP_CONTROLLER_HYBRID "name=unified" -#define SYSTEMD_CGROUP_CONTROLLER "_systemd" - #define SIGNALS_CRASH_HANDLER SIGSEGV,SIGILL,SIGFPE,SIGBUS,SIGQUIT,SIGABRT #define SIGNALS_IGNORE SIGPIPE From ea9053c5f8eaae748683848ed807e7dbf30d85a7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 17 Nov 2017 13:05:30 +0100 Subject: [PATCH 02/13] nspawn: rework mount_systemd_cgroup_writable() a bit We shouldn't call alloca() as part of function calls, that's not really defined in C. Hence, let's first do our stack allocations, and then invoke functions. Also, some coding style fixes, and minor shuffling around. No functional changes. --- src/nspawn/nspawn-mount.c | 41 +++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/src/nspawn/nspawn-mount.c b/src/nspawn/nspawn-mount.c index c8c941cdfd..69707cb7f9 100644 --- a/src/nspawn/nspawn-mount.c +++ b/src/nspawn/nspawn-mount.c @@ -1213,23 +1213,25 @@ int mount_cgroups( if (unified_requested >= CGROUP_UNIFIED_ALL) return mount_unified_cgroups(dest); - else if (use_cgns) + if (use_cgns) return mount_legacy_cgns_supported(dest, unified_requested, userns, uid_shift, uid_range, selinux_apifs_context); return mount_legacy_cgns_unsupported(dest, unified_requested, userns, uid_shift, uid_range, selinux_apifs_context); } -static int mount_systemd_cgroup_writable_one(const char *systemd_own, const char *systemd_root) -{ +static int mount_systemd_cgroup_writable_one(const char *root, const char *own) { int r; + assert(root); + assert(own); + /* Make our own cgroup a (writable) bind mount */ - r = mount_verbose(LOG_ERR, systemd_own, systemd_own, NULL, MS_BIND, NULL); + r = mount_verbose(LOG_ERR, own, own, NULL, MS_BIND, NULL); if (r < 0) return r; /* And then remount the systemd cgroup root read-only */ - return mount_verbose(LOG_ERR, NULL, systemd_root, NULL, + return mount_verbose(LOG_ERR, NULL, root, NULL, MS_BIND|MS_REMOUNT|MS_NOSUID|MS_NOEXEC|MS_NODEV|MS_RDONLY, NULL); } @@ -1238,6 +1240,7 @@ int mount_systemd_cgroup_writable( CGroupUnified unified_requested) { _cleanup_free_ char *own_cgroup_path = NULL; + const char *root, *own; int r; assert(dest); @@ -1250,19 +1253,27 @@ int mount_systemd_cgroup_writable( if (path_equal(own_cgroup_path, "/")) return 0; - if (unified_requested >= CGROUP_UNIFIED_ALL) - return mount_systemd_cgroup_writable_one(strjoina(dest, "/sys/fs/cgroup", own_cgroup_path), - prefix_roota(dest, "/sys/fs/cgroup")); + if (unified_requested >= CGROUP_UNIFIED_ALL) { - if (unified_requested >= CGROUP_UNIFIED_SYSTEMD) { - r = mount_systemd_cgroup_writable_one(strjoina(dest, "/sys/fs/cgroup/unified", own_cgroup_path), - prefix_roota(dest, "/sys/fs/cgroup/unified")); - if (r < 0) - return r; + root = prefix_roota(dest, "/sys/fs/cgroup"); + own = strjoina(root, own_cgroup_path); + + } else { + + if (unified_requested >= CGROUP_UNIFIED_SYSTEMD) { + root = prefix_roota(dest, "/sys/fs/cgroup/unified"); + own = strjoina(root, own_cgroup_path); + + r = mount_systemd_cgroup_writable_one(root, own); + if (r < 0) + return r; + } + + root = prefix_roota(dest, "/sys/fs/cgroup/systemd"); + own = strjoina(root, own_cgroup_path); } - return mount_systemd_cgroup_writable_one(strjoina(dest, "/sys/fs/cgroup/systemd", own_cgroup_path), - prefix_roota(dest, "/sys/fs/cgroup/systemd")); + return mount_systemd_cgroup_writable_one(root, own); } int setup_volatile_state( From d7c9693a3e792a43d27f78ec5ccf37e4738c7902 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 17 Nov 2017 13:35:56 +0100 Subject: [PATCH 03/13] nspawn-mount: rework get_controllers() a bit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's rename get_controllers() → get_process_controllers(), in order to underline the difference to cg_kernel_controllers(). After all, one returns the controllers available to the process, the other the controllers enabled in the kernel at all). Let's also update the code to use read_line() and set_put_strdup() to shorten the code a bit, and make it more robust. --- src/nspawn/nspawn-mount.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/src/nspawn/nspawn-mount.c b/src/nspawn/nspawn-mount.c index 69707cb7f9..3273a7cfc4 100644 --- a/src/nspawn/nspawn-mount.c +++ b/src/nspawn/nspawn-mount.c @@ -867,19 +867,30 @@ int mount_custom( /* Retrieve existing subsystems. This function is called in a new cgroup * namespace. */ -static int get_controllers(Set *subsystems) { +static int get_process_controllers(Set **ret) { + _cleanup_set_free_free_ Set *controllers = NULL; _cleanup_fclose_ FILE *f = NULL; - char line[LINE_MAX]; + int r; - assert(subsystems); + assert(ret); + + controllers = set_new(&string_hash_ops); + if (!controllers) + return -ENOMEM; f = fopen("/proc/self/cgroup", "re"); if (!f) return errno == ENOENT ? -ESRCH : -errno; - FOREACH_LINE(line, f, return -errno) { - int r; - char *e, *l, *p; + for (;;) { + _cleanup_free_ char *line = NULL; + char *e, *l; + + r = read_line(f, LONG_LINE_MAX, &line); + if (r < 0) + return r; + if (r == 0) + break; l = strchr(line, ':'); if (!l) @@ -895,15 +906,14 @@ static int get_controllers(Set *subsystems) { if (STR_IN_SET(l, "", "name=systemd", "name=unified")) continue; - p = strdup(l); - if (!p) - return -ENOMEM; - - r = set_consume(subsystems, p); + r = set_put_strdup(controllers, l); if (r < 0) return r; } + *ret = controllers; + controllers = NULL; + return 0; } @@ -999,11 +1009,7 @@ static int mount_legacy_cgns_supported( if (r > 0) goto skip_controllers; - controllers = set_new(&string_hash_ops); - if (!controllers) - return log_oom(); - - r = get_controllers(controllers); + r = get_process_controllers(&controllers); if (r < 0) return log_error_errno(r, "Failed to determine cgroup controllers: %m"); From bf516294c89d8db9769d90b4f90c177adafa038c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 17 Nov 2017 13:42:58 +0100 Subject: [PATCH 04/13] nspawn: minor optimization no need to prepare the target path if we quite the loop anyway one step later. --- src/nspawn/nspawn-mount.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/nspawn/nspawn-mount.c b/src/nspawn/nspawn-mount.c index 3273a7cfc4..bfff53054c 100644 --- a/src/nspawn/nspawn-mount.c +++ b/src/nspawn/nspawn-mount.c @@ -1038,13 +1038,13 @@ static int mount_legacy_cgns_supported( if (r == 0) break; - target = prefix_root("/sys/fs/cgroup", tok); - if (!target) - return log_oom(); - if (streq(controller, tok)) break; + target = prefix_root("/sys/fs/cgroup/", tok); + if (!target) + return log_oom(); + r = symlink_idempotent(controller, target); if (r == -EINVAL) return log_error_errno(r, "Invalid existing symlink for combined hierarchy: %m"); From 6925a0de4eaf181c08d7380249461abe5c42fe3f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 17 Nov 2017 16:27:13 +0100 Subject: [PATCH 05/13] cgroup-util: move Set* allocation into cg_kernel_controllers() Previously, callers had to do this on their own. Let's make the call do that instead, making the caller code a bit shorter. --- src/basic/cgroup-util.c | 17 ++++++++++++++--- src/basic/cgroup-util.h | 2 +- src/core/mount-setup.c | 6 +----- src/nspawn/nspawn-mount.c | 6 +----- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index cd32896b26..7a3a1b2b0e 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -2369,21 +2369,29 @@ int cg_mask_supported(CGroupMask *ret) { return 0; } -int cg_kernel_controllers(Set *controllers) { +int cg_kernel_controllers(Set **ret) { + _cleanup_set_free_free_ Set *controllers = NULL; _cleanup_fclose_ FILE *f = NULL; int r; - assert(controllers); + assert(ret); /* Determines the full list of kernel-known controllers. Might * include controllers we don't actually support, arbitrary * named hierarchies and controllers that aren't currently * accessible (because not mounted). */ + controllers = set_new(&string_hash_ops); + if (!controllers) + return -ENOMEM; + f = fopen("/proc/cgroups", "re"); if (!f) { - if (errno == ENOENT) + if (errno == ENOENT) { + *ret = NULL; return 0; + } + return -errno; } @@ -2421,6 +2429,9 @@ int cg_kernel_controllers(Set *controllers) { return r; } + *ret = controllers; + controllers = NULL; + return 0; } diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h index df3d01e4ef..e286e4e9c2 100644 --- a/src/basic/cgroup-util.h +++ b/src/basic/cgroup-util.h @@ -243,7 +243,7 @@ int cg_mask_supported(CGroupMask *ret); int cg_mask_from_string(const char *s, CGroupMask *ret); int cg_mask_to_string(CGroupMask mask, char **ret); -int cg_kernel_controllers(Set *controllers); +int cg_kernel_controllers(Set **controllers); bool cg_ns_supported(void); diff --git a/src/core/mount-setup.c b/src/core/mount-setup.c index 0054cf8ba3..adf20b68c7 100644 --- a/src/core/mount-setup.c +++ b/src/core/mount-setup.c @@ -237,11 +237,7 @@ int mount_cgroup_controllers(char ***join_controllers) { /* Mount all available cgroup controllers that are built into the kernel. */ - controllers = set_new(&string_hash_ops); - if (!controllers) - return log_oom(); - - r = cg_kernel_controllers(controllers); + r = cg_kernel_controllers(&controllers); if (r < 0) return log_error_errno(r, "Failed to enumerate cgroup controllers: %m"); diff --git a/src/nspawn/nspawn-mount.c b/src/nspawn/nspawn-mount.c index bfff53054c..9e6f573ae3 100644 --- a/src/nspawn/nspawn-mount.c +++ b/src/nspawn/nspawn-mount.c @@ -1111,11 +1111,7 @@ static int mount_legacy_cgns_unsupported( if (r > 0) goto skip_controllers; - controllers = set_new(&string_hash_ops); - if (!controllers) - return log_oom(); - - r = cg_kernel_controllers(controllers); + r = cg_kernel_controllers(&controllers); if (r < 0) return log_error_errno(r, "Failed to determine cgroup controllers: %m"); From 3c7416b6cae1006388755598415e1139727b926a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 17 Nov 2017 16:43:08 +0100 Subject: [PATCH 06/13] core: unify common code for preparing for forking off unit processes This introduces a new function unit_prepare_exec() that encapsulates a number of calls we do in preparation for spawning off some processes in all our unit types that do so. This allows us to neatly unify a bit of code between unit types and shorten our code. --- src/core/mount.c | 23 ++++++----------------- src/core/mount.h | 2 -- src/core/service.c | 34 ++++++++++------------------------ src/core/service.h | 2 -- src/core/socket.c | 23 ++++++----------------- src/core/socket.h | 2 -- src/core/swap.c | 25 +++++++------------------ src/core/swap.h | 2 -- src/core/unit.c | 28 ++++++++++++++++++++++++++++ src/core/unit.h | 5 +++++ 10 files changed, 62 insertions(+), 84 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 79a6e1626c..1500670b09 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -750,33 +750,21 @@ static void mount_dump(Unit *u, FILE *f, const char *prefix) { } static int mount_spawn(Mount *m, ExecCommand *c, pid_t *_pid) { - pid_t pid; - int r; + ExecParameters exec_params = { .flags = EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN, .stdin_fd = -1, .stdout_fd = -1, .stderr_fd = -1, }; + pid_t pid; + int r; assert(m); assert(c); assert(_pid); - (void) unit_realize_cgroup(UNIT(m)); - if (m->reset_accounting) { - (void) unit_reset_cpu_accounting(UNIT(m)); - (void) unit_reset_ip_accounting(UNIT(m)); - m->reset_accounting = false; - } - - unit_export_state_files(UNIT(m)); - - r = unit_setup_exec_runtime(UNIT(m)); - if (r < 0) - return r; - - r = unit_setup_dynamic_creds(UNIT(m)); + r = unit_prepare_exec(UNIT(m)); if (r < 0) return r; @@ -1091,7 +1079,8 @@ static int mount_start(Unit *u) { m->result = MOUNT_SUCCESS; m->reload_result = MOUNT_SUCCESS; - m->reset_accounting = true; + + u->reset_accounting = true; mount_enter_mounting(m); return 1; diff --git a/src/core/mount.h b/src/core/mount.h index 8309441a8d..44fe3b889e 100644 --- a/src/core/mount.h +++ b/src/core/mount.h @@ -68,8 +68,6 @@ struct Mount { bool just_mounted:1; bool just_changed:1; - bool reset_accounting:1; - bool sloppy_options; bool lazy_unmount; diff --git a/src/core/service.c b/src/core/service.c index 445d1becc1..cd23eb5233 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1223,24 +1223,26 @@ static int service_spawn( ExecFlags flags, pid_t *_pid) { - _cleanup_strv_free_ char **final_env = NULL, **our_env = NULL, **fd_names = NULL; - _cleanup_free_ int *fds = NULL; - unsigned n_storage_fds = 0, n_socket_fds = 0, n_env = 0; - pid_t pid; - ExecParameters exec_params = { .flags = flags, .stdin_fd = -1, .stdout_fd = -1, .stderr_fd = -1, }; - + _cleanup_strv_free_ char **final_env = NULL, **our_env = NULL, **fd_names = NULL; + unsigned n_storage_fds = 0, n_socket_fds = 0, n_env = 0; + _cleanup_free_ int *fds = NULL; + pid_t pid; int r; assert(s); assert(c); assert(_pid); + r = unit_prepare_exec(UNIT(s)); + if (r < 0) + return r; + if (flags & EXEC_IS_CONTROL) { /* If this is a control process, mask the permissions/chroot application if this is requested. */ if (s->permissions_start_only) @@ -1249,23 +1251,6 @@ static int service_spawn( exec_params.flags &= ~EXEC_APPLY_CHROOT; } - (void) unit_realize_cgroup(UNIT(s)); - if (s->reset_accounting) { - (void) unit_reset_cpu_accounting(UNIT(s)); - (void) unit_reset_ip_accounting(UNIT(s)); - s->reset_accounting = false; - } - - unit_export_state_files(UNIT(s)); - - r = unit_setup_exec_runtime(UNIT(s)); - if (r < 0) - return r; - - r = unit_setup_dynamic_creds(UNIT(s)); - if (r < 0) - return r; - if ((flags & EXEC_PASS_FDS) || s->exec_context.std_input == EXEC_INPUT_SOCKET || s->exec_context.std_output == EXEC_OUTPUT_SOCKET || @@ -2188,7 +2173,8 @@ static int service_start(Unit *u) { s->main_pid_known = false; s->main_pid_alien = false; s->forbid_restart = false; - s->reset_accounting = true; + + u->reset_accounting = true; s->status_text = mfree(s->status_text); s->status_errno = 0; diff --git a/src/core/service.h b/src/core/service.h index a529f48a63..d6be140c54 100644 --- a/src/core/service.h +++ b/src/core/service.h @@ -166,8 +166,6 @@ struct Service { bool forbid_restart:1; bool start_timeout_defined:1; - bool reset_accounting:1; - char *bus_name; char *bus_name_owner; /* unique name of the current owner */ diff --git a/src/core/socket.c b/src/core/socket.c index 6c0d799bd9..15922d919f 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -1865,33 +1865,21 @@ static int socket_coldplug(Unit *u) { } static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) { - pid_t pid; - int r; + ExecParameters exec_params = { .flags = EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN, .stdin_fd = -1, .stdout_fd = -1, .stderr_fd = -1, }; + pid_t pid; + int r; assert(s); assert(c); assert(_pid); - (void) unit_realize_cgroup(UNIT(s)); - if (s->reset_accounting) { - (void) unit_reset_cpu_accounting(UNIT(s)); - (void) unit_reset_ip_accounting(UNIT(s)); - s->reset_accounting = false; - } - - unit_export_state_files(UNIT(s)); - - r = unit_setup_exec_runtime(UNIT(s)); - if (r < 0) - return r; - - r = unit_setup_dynamic_creds(UNIT(s)); + r = unit_prepare_exec(UNIT(s)); if (r < 0) return r; @@ -2471,7 +2459,8 @@ static int socket_start(Unit *u) { return r; s->result = SOCKET_SUCCESS; - s->reset_accounting = true; + + u->reset_accounting = true; socket_enter_start_pre(s); return 1; diff --git a/src/core/socket.h b/src/core/socket.h index 81cfc97578..e9e560e57e 100644 --- a/src/core/socket.h +++ b/src/core/socket.h @@ -162,8 +162,6 @@ struct Socket { char *user, *group; - bool reset_accounting:1; - char *fdname; RateLimit trigger_limit; diff --git a/src/core/swap.c b/src/core/swap.c index f37a81f56c..115e59c5ca 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -603,35 +603,23 @@ static void swap_dump(Unit *u, FILE *f, const char *prefix) { } static int swap_spawn(Swap *s, ExecCommand *c, pid_t *_pid) { - pid_t pid; - int r; + ExecParameters exec_params = { .flags = EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN, .stdin_fd = -1, .stdout_fd = -1, .stderr_fd = -1, }; + pid_t pid; + int r; assert(s); assert(c); assert(_pid); - (void) unit_realize_cgroup(UNIT(s)); - if (s->reset_accounting) { - (void) unit_reset_cpu_accounting(UNIT(s)); - (void) unit_reset_ip_accounting(UNIT(s)); - s->reset_accounting = false; - } - - unit_export_state_files(UNIT(s)); - - r = unit_setup_exec_runtime(UNIT(s)); + r = unit_prepare_exec(UNIT(s)); if (r < 0) - goto fail; - - r = unit_setup_dynamic_creds(UNIT(s)); - if (r < 0) - goto fail; + return r; r = swap_arm_timer(s, usec_add(now(CLOCK_MONOTONIC), s->timeout_usec)); if (r < 0) @@ -868,7 +856,8 @@ static int swap_start(Unit *u) { return r; s->result = SWAP_SUCCESS; - s->reset_accounting = true; + + u->reset_accounting = true; swap_enter_activating(s); return 1; diff --git a/src/core/swap.h b/src/core/swap.h index d1ee73de8e..fa9d45ac0c 100644 --- a/src/core/swap.h +++ b/src/core/swap.h @@ -71,8 +71,6 @@ struct Swap { bool is_active:1; bool just_activated:1; - bool reset_accounting:1; - SwapResult result; usec_t timeout_usec; diff --git a/src/core/unit.c b/src/core/unit.c index d5e6b3891b..10b416a65b 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -5145,6 +5145,34 @@ void unit_unlink_state_files(Unit *u) { } } +int unit_prepare_exec(Unit *u) { + int r; + + assert(u); + + /* Prepares everything so that we can fork of a process for this unit */ + + (void) unit_realize_cgroup(u); + + if (u->reset_accounting) { + (void) unit_reset_cpu_accounting(u); + (void) unit_reset_ip_accounting(u); + u->reset_accounting = false; + } + + unit_export_state_files(u); + + r = unit_setup_exec_runtime(u); + if (r < 0) + return r; + + r = unit_setup_dynamic_creds(u); + if (r < 0) + return r; + + return 0; +} + static const char* const collect_mode_table[_COLLECT_MODE_MAX] = { [COLLECT_INACTIVE] = "inactive", [COLLECT_INACTIVE_OR_FAILED] = "inactive-or-failed", diff --git a/src/core/unit.h b/src/core/unit.h index 2b11a285d4..0f756dcda4 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -342,6 +342,9 @@ struct Unit { UnitCGroupBPFState cgroup_bpf_state:2; + /* Reset cgroup accounting next time we fork something off */ + bool reset_accounting:1; + bool start_limit_hit:1; /* Did we already invoke unit_coldplug() for this unit? */ @@ -760,6 +763,8 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask); void unit_export_state_files(Unit *u); void unit_unlink_state_files(Unit *u); +int unit_prepare_exec(Unit *u); + /* Macros which append UNIT= or USER_UNIT= to the message */ #define log_unit_full(unit, level, error, ...) \ From 5962e9db5ebf01739208c7f1a33b2c5fb05af9cc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 17 Nov 2017 18:03:00 +0100 Subject: [PATCH 07/13] update TODO --- TODO | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/TODO b/TODO index d8de3b4a23..b18f8b5aa7 100644 --- a/TODO +++ b/TODO @@ -32,6 +32,9 @@ Features: * add async version of sd_bus_add_match and make use of that +* support projid-based quota in machinectl for containers, and then drop + implicit btrfs loopback magic in machined + * let's log the "tainted" string at boot * Add NetworkNamespacePath= to specify a path to a network namespace @@ -80,7 +83,11 @@ Features: * maybe hook of xfs/ext4 quotactl() with services? i.e. automatically manage the quota of a the user indicated in User= via unit file settings, like the - other resource management concepts. Would mix nicely with DynamicUser=1 + other resource management concepts. Would mix nicely with DynamicUser=1. Or + alternatively, do this with projids, so that we can also cover services + running as root. Quota should probably cover all the special dirs such as + StateDirectory=, LogsDirectory=, CacheDirectory=, as well as RootDirectory= if it + is set, plus the whole disk space any image configured with RootImage=. * add dissect_image_warn() as a wrapper around dissect_image() that prints friendly log messages for the returned errors, so that we don't have to From 77fa610b22f343671945501905e8079d68807ddb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 17 Nov 2017 18:03:22 +0100 Subject: [PATCH 08/13] =?UTF-8?q?cgroup-util:=20optimization=20=E2=80=94?= =?UTF-8?q?=20open=20subtree=5Fcontrol=20file=20only=20once=20for=20all=20?= =?UTF-8?q?controllers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/basic/cgroup-util.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 7a3a1b2b0e..a7964fb5a9 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -2541,6 +2541,7 @@ int cg_unified_flush(void) { } int cg_enable_everywhere(CGroupMask supported, CGroupMask mask, const char *p) { + _cleanup_fclose_ FILE *f = NULL; _cleanup_free_ char *fs = NULL; CGroupController c; int r; @@ -2574,7 +2575,15 @@ int cg_enable_everywhere(CGroupMask supported, CGroupMask mask, const char *p) { s[0] = mask & bit ? '+' : '-'; strcpy(s + 1, n); - r = write_string_file(fs, s, 0); + if (!f) { + f = fopen(fs, "we"); + if (!f) { + log_debug_errno(errno, "Failed to open cgroup.subtree_control file of %s: %m", p); + break; + } + } + + r = write_string_stream(f, s, 0); if (r < 0) log_debug_errno(r, "Failed to enable controller %s for %s (%s): %m", n, p, fs); } From 5e20b0a4522d8d2753ae3991a7aa9f11f1f6cc95 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 17 Nov 2017 19:24:06 +0100 Subject: [PATCH 09/13] cgroup: properly determine cgroups zombie processes belong to When a process becomes a zombie its cgroup might be deleted. Let's add some minimal code to detect cases like this, so that we can still attribute this back to the original cgroup. --- src/basic/cgroup-util.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index a7964fb5a9..ae8f2d1160 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -1104,6 +1104,11 @@ int cg_pid_get_path(const char *controller, pid_t pid, char **path) { if (!p) return -ENOMEM; + /* Truncate suffix indicating the process is a zombie */ + e = endswith(p, " (deleted)"); + if (e) + *e = 0; + *path = p; return 0; } From 64e844e5ca5be2f8766c68bcfb112e4cd84a8bb2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 17 Nov 2017 19:25:18 +0100 Subject: [PATCH 10/13] cgroup: fix delegation on the unified hierarchy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make sure to add the delegation mask to the mask of controllers we have to enable on our own unit. Do not claim it was a members mask, as such a logic would mean we'd collide with cgroupv2's "no processes on inner nodes policy". This change does the right thing: it means any controller enabled through Controllers= will be made available to subcrgoups of our unit, but the unit itself has to still enable it through cgroup.subtree_control (which it can since that file is delegated too) to be inherited further down. Or to say this differently: we only should manipulate cgroup.subtree_control ourselves for inner nodes (i.e. slices), and for leaves we need to provide a way to enable controllers in the slices above, but stay away from the cgroup's own cgroup.subtree_control — which is what this patch ensures. Fixes: #7355 --- src/core/cgroup.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index bd0910ed69..7f86beb4ea 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1073,7 +1073,7 @@ CGroupMask unit_get_own_mask(Unit *u) { if (!c) return 0; - return cgroup_context_get_mask(c); + return cgroup_context_get_mask(c) | unit_get_delegate_mask(u); } CGroupMask unit_get_delegate_mask(Unit *u) { @@ -1113,7 +1113,7 @@ CGroupMask unit_get_members_mask(Unit *u) { if (u->cgroup_members_mask_valid) return u->cgroup_members_mask; - u->cgroup_members_mask = unit_get_delegate_mask(u); + u->cgroup_members_mask = 0; if (u->type == UNIT_SLICE) { void *v; @@ -1146,7 +1146,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_subtree_mask(u); + return unit_get_subtree_mask(u); /* we are the top-level slice */ } CGroupMask unit_get_subtree_mask(Unit *u) { From b961baf1cef3bfa01488e1acbe9e67932588a96e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 17 Nov 2017 20:06:26 +0100 Subject: [PATCH 11/13] test: add a test case that validates cgroup delegation This test runs on the unified hierarchy, and ensures that cgroup delegation works properly, i.e. writ access is granted and the requested controllers are enabled. --- test/TEST-19-DELEGATE/Makefile | 4 +++ test/TEST-19-DELEGATE/test.sh | 43 ++++++++++++++++++++++++++++++ test/TEST-19-DELEGATE/testsuite.sh | 20 ++++++++++++++ 3 files changed, 67 insertions(+) create mode 100644 test/TEST-19-DELEGATE/Makefile create mode 100755 test/TEST-19-DELEGATE/test.sh create mode 100755 test/TEST-19-DELEGATE/testsuite.sh diff --git a/test/TEST-19-DELEGATE/Makefile b/test/TEST-19-DELEGATE/Makefile new file mode 100644 index 0000000000..b895de8bcb --- /dev/null +++ b/test/TEST-19-DELEGATE/Makefile @@ -0,0 +1,4 @@ +include ../Makefile.guess + +all setup clean run: + @basedir=../.. TEST_BASE_DIR=../ BUILD_DIR=$(BUILD_DIR) ./test.sh --$@ diff --git a/test/TEST-19-DELEGATE/test.sh b/test/TEST-19-DELEGATE/test.sh new file mode 100755 index 0000000000..841a29c06f --- /dev/null +++ b/test/TEST-19-DELEGATE/test.sh @@ -0,0 +1,43 @@ +#!/bin/bash +# -*- mode: shell-script; indent-tabs-mode: nil; sh-basic-offset: 4; -*- +# ex: ts=8 sw=4 sts=4 et filetype=sh +set -e +TEST_DESCRIPTION="test cgroup delegation in the unifier hierarchy" +TEST_NO_NSPAWN=1 + +. $TEST_BASE_DIR/test-functions +QEMU_TIMEOUT=180 +UNIFIED_CGROUP_HIERARCHY=yes + +test_setup() { + create_empty_image + mkdir -p $TESTDIR/root + mount ${LOOPDEV}p1 $TESTDIR/root + + ( + LOG_LEVEL=5 + eval $(udevadm info --export --query=env --name=${LOOPDEV}p2) + + setup_basic_environment + + # setup the testsuite service + cat >$initdir/etc/systemd/system/testsuite.service < /testok + +exit 0 From 99f3baa983af730815fb8fb6e0fbe3b5e40abc0b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 20 Nov 2017 12:40:08 +0100 Subject: [PATCH 12/13] man: clarify that the controllers listed on Delegate= might not be the only ones --- man/systemd.resource-control.xml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/man/systemd.resource-control.xml b/man/systemd.resource-control.xml index 1ab13ea00b..18b7bf8f6a 100644 --- a/man/systemd.resource-control.xml +++ b/man/systemd.resource-control.xml @@ -720,9 +720,10 @@ 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 reset the list of controllers, all assignments prior to this will have no effect. - Defaults to false. + is turned on, and the specified controllers are enabled for the unit. Note that additional controllers than + the ones specified might be made available as well, depending on configuration of the containing slice unit + or other units contained in it. Note that assigning the empty string will enable delegation, but reset the + list of controllers, all assignments prior to this will have no effect. 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 From e7266e98f9f90a23681a0adc583ac6e02ab243c4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 21 Nov 2017 11:08:22 +0100 Subject: [PATCH 13/13] test: fix UDEV-WANTS testcase for non-bash shells testsuite.sh uses "set -o pipefile", which is a bashism, hence use bash to invoke the script. --- test/TEST-17-UDEV-WANTS/test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/TEST-17-UDEV-WANTS/test.sh b/test/TEST-17-UDEV-WANTS/test.sh index 7beef7f398..24989ebcf6 100755 --- a/test/TEST-17-UDEV-WANTS/test.sh +++ b/test/TEST-17-UDEV-WANTS/test.sh @@ -32,7 +32,7 @@ test_setup() { Description=Testsuite service [Service] -ExecStart=/bin/sh -x /testsuite.sh +ExecStart=/bin/bash -x /testsuite.sh Type=oneshot StandardOutput=tty StandardError=tty