From 5beac75e44331e1a99103058bb86f958d5f69d54 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 22 Sep 2017 19:57:07 +0200 Subject: [PATCH 1/4] cgroup-util: downgrade log messages from library code to LOG_DEBUG These errors don't really matter, that's why we log and proceed in the current code. However, we currently log at LOG_WARNING, but we really shouldn't given that this is library code. Hence downgrade this to LOG_DEBUG. --- src/basic/cgroup-util.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 5dea078978..2c94350cd7 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -902,7 +902,7 @@ int cg_set_group_access( if (r > 0 && streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { r = cg_set_group_access(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, mode, uid, gid); if (r < 0) - log_warning_errno(r, "Failed to set group access on compat systemd cgroup %s: %m", path); + log_debug_errno(r, "Failed to set group access on compatibility systemd cgroup %s, ignoring: %m", path); } return 0; @@ -948,9 +948,11 @@ int cg_set_task_access( if (r < 0) return r; if (r > 0 && streq(controller, SYSTEMD_CGROUP_CONTROLLER)) { + /* Always propagate access mode from unified to legacy controller */ + r = cg_set_task_access(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, mode, uid, gid); if (r < 0) - log_warning_errno(r, "Failed to set task access on compat systemd cgroup %s: %m", path); + log_debug_errno(r, "Failed to set task access on compatibility systemd cgroup %s, ignoring: %m", path); } return 0; From 40853aa53fae07bf891206fb1adf3b06f36ab888 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 22 Sep 2017 19:58:24 +0200 Subject: [PATCH 2/4] cgroup: rework which files we chown() on delegation On cgroupsv2 we should also chown()/chmod() the subtree_control file, so that children can use controllers the way they like. On cgroupsv1 we should also chown()/chmod() cgroups.clone_children, as not setting this for new cgroups makes little sense, and hence delegated clients should be able to write to it. Note that error handling for both cases is different. subtree_control matters so we check for errors, but the clone_children/tasks stuff doesn't really, as it's legacy stuff. Hence we only log errors and proceed. Fixes: #6216 --- src/basic/cgroup-util.c | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 2c94350cd7..e008d007d8 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -915,7 +915,7 @@ int cg_set_task_access( uid_t uid, gid_t gid) { - _cleanup_free_ char *fs = NULL, *procs = NULL; + _cleanup_free_ char *fs = NULL; int r; assert(path); @@ -926,6 +926,7 @@ int cg_set_task_access( if (mode != MODE_INVALID) mode &= 0666; + /* For both the legacy and unified hierarchies, "cgroup.procs" is the main entry point for PIDs */ r = cg_get_path(controller, path, "cgroup.procs", &fs); if (r < 0) return r; @@ -938,10 +939,37 @@ int cg_set_task_access( if (r < 0) return r; if (r == 0) { - /* Compatibility, Always keep values for "tasks" in sync with - * "cgroup.procs" */ - if (cg_get_path(controller, path, "tasks", &procs) >= 0) - (void) chmod_and_chown(procs, mode, uid, gid); + const char *fn; + + /* Compatibility: on cgroupsv1 always keep values for the legacy files "tasks" and + * "cgroup.clone_children" in sync with "cgroup.procs". Since this is legacy stuff, we don't care if + * this fails. */ + + FOREACH_STRING(fn, + "tasks", + "cgroup.clone_children") { + + fs = mfree(fs); + + r = cg_get_path(controller, path, fn, &fs); + if (r < 0) + log_debug_errno(r, "Failed to get path for %s of %s, ignoring: %m", fn, path); + + r = chmod_and_chown(fs, mode, uid, gid); + if (r < 0) + log_debug_errno(r, "Failed to to change ownership/access mode for %s of %s, ignoring: %m", fn, path); + } + } else { + /* On the unified controller, we want to permit subtree controllers too. */ + + fs = mfree(fs); + r = cg_get_path(controller, path, "cgroup.subtree_control", &fs); + if (r < 0) + return r; + + r = chmod_and_chown(fs, mode, uid, gid); + if (r < 0) + return r; } r = cg_hybrid_unified(); From 7960b0c704f2b41705219ba36f0cd6f9a29cef99 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 22 Sep 2017 20:02:23 +0200 Subject: [PATCH 3/4] cgroup: make use of unit_cgroup_delegate() where useful It's an easy-to-use wrapper, so let's take benefit of it. --- src/core/unit.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index df89f3d01f..f1936bdf0b 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4429,14 +4429,10 @@ int unit_acquire_invocation_id(Unit *u) { return 0; } -void unit_set_exec_params(Unit *s, ExecParameters *p) { - CGroupContext *c; +void unit_set_exec_params(Unit *u, ExecParameters *p) { + assert(u); + assert(p); - assert(s); - assert(s); - - p->cgroup_path = s->cgroup_path; - - c = unit_get_cgroup_context(s); - SET_FLAG(p->flags, EXEC_CGROUP_DELEGATE, c && c->delegate); + p->cgroup_path = u->cgroup_path; + SET_FLAG(p->flags, EXEC_CGROUP_DELEGATE, unit_cgroup_delegate(u)); } From 88af31f92221aac652fe5584093abb0e68bc544a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 22 Sep 2017 20:09:21 +0200 Subject: [PATCH 4/4] socket: assign socket units to a default slice unconditionally Due to the chown() logic socket units might end up with processes even if no explicit command is defined for them, hence let's make sure these processes are in the right cgroup, and that means within a slice. Mount, swap and service units unconditionally are assigned to a slice already, let's do the same here, too. (This becomes more important as soon as the ebpf/firewall stuff is merged, as there'll be another reason to fork off processes then) --- src/core/socket.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/socket.c b/src/core/socket.c index 9d8367e90b..5993ce0d00 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -397,12 +397,12 @@ static int socket_add_extras(Socket *s) { r = unit_add_exec_dependencies(u, &s->exec_context); if (r < 0) return r; - - r = unit_set_default_slice(u); - if (r < 0) - return r; } + r = unit_set_default_slice(u); + if (r < 0) + return r; + r = socket_add_default_dependencies(s); if (r < 0) return r;