cgroup-util: merge cg_set_tasks_access() and cg-set_group_access() into one

We never use these functions seperately, hence don't bother splitting
them into to.

Also, simplify things a bit, and maintain tables for the attribute files
to chown. Let's also update those tables a bit, and include thenew
"cgroup.threads" file in it, that needs to be delegated too, according
to the documentation.

diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c
index 7e4670ea8c5606d53ec515d01a152e706e45d1ce..5774d4dea160f85bf40a5d69b0cbbc2e85aaadf3 100644 (file)
--- a/src/basic/cgroup-util.c
+++ b/src/basic/cgroup-util.c
@@ -876,115 +876,87 @@ int cg_attach_fallback(const char *controller, const char *path, pid_t pid) {
         return r;
 }
 
-int cg_set_group_access(
+int cg_set_access(
                 const char *controller,
                 const char *path,
-                mode_t mode,
                 uid_t uid,
                 gid_t gid) {
 
-        _cleanup_free_ char *fs = NULL;
-        int r;
-
-        if (mode == MODE_INVALID && uid == UID_INVALID && gid == GID_INVALID)
-                return 0;
-
-        if (mode != MODE_INVALID)
-                mode &= 0777;
-
-        r = cg_get_path(controller, path, NULL, &fs);
-        if (r < 0)
-                return r;
-
-        r = chmod_and_chown(fs, mode, uid, gid);
-        if (r < 0)
-                return r;
-
-        r = cg_hybrid_unified();
-        if (r < 0)
-                return r;
-        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_debug_errno(r, "Failed to set group access on compatibility systemd cgroup %s, ignoring: %m", path);
-        }
-
-        return 0;
-}
-
-int cg_set_task_access(
-                const char *controller,
-                const char *path,
-                mode_t mode,
-                uid_t uid,
-                gid_t gid) {
+        struct Attribute {
+                const char *name;
+                bool fatal;
+        };
+
+        /* cgroupsv1, aka legacy/non-unified */
+        static const struct Attribute legacy_attributes[] = {
+                { "cgroup.procs",           true  },
+                { "tasks",                  false },
+                { "cgroup.clone_children",  false },
+                {},
+        };
+
+        /* cgroupsv2, aka unified */
+        static const struct Attribute unified_attributes[] = {
+                { "cgroup.procs",           true  },
+                { "cgroup.subtree_control", true  },
+                { "cgroup.threads",         false },
+                {},
+        };
+
+        static const struct Attribute* const attributes[] = {
+                [false] = legacy_attributes,
+                [true]  = unified_attributes,
+        };
 
         _cleanup_free_ char *fs = NULL;
-        int r;
+        const struct Attribute *i;
+        int r, unified;
 
         assert(path);
 
-        if (mode == MODE_INVALID && uid == UID_INVALID && gid == GID_INVALID)
+        if (uid == UID_INVALID && gid == GID_INVALID)
                 return 0;
 
-        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;
+        unified = cg_unified_controller(controller);
+        if (unified < 0)
+                return unified;
 
-        r = chmod_and_chown(fs, mode, uid, gid);
+        /* Configure access to the cgroup itself */
+        r = cg_get_path(controller, path, NULL, &fs);
         if (r < 0)
                 return r;
 
-        r = cg_unified_controller(controller);
+        r = chmod_and_chown(fs, 0755, uid, gid);
         if (r < 0)
                 return r;
-        if (r == 0) {
-                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. */
 
+        /* Configure access to the cgroup's attributes */
+        for (i = attributes[unified]; i->name; i++) {
                 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);
+                r = cg_get_path(controller, path, i->name, &fs);
                 if (r < 0)
                         return r;
-        }
 
-        r = cg_hybrid_unified();
-        if (r < 0)
-                return r;
-        if (r > 0 && streq(controller, SYSTEMD_CGROUP_CONTROLLER)) {
-                /* Always propagate access mode from unified to legacy controller */
+                r = chmod_and_chown(fs, 0644, uid, gid);
+                if (r < 0) {
+                        if (i->fatal)
+                                return r;
 
-                r = cg_set_task_access(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, mode, uid, gid);
+                        log_debug_errno(r, "Failed to set access on cgroup %s, ignoring: %m", fs);
+                }
+        }
+
+        if (streq(controller, SYSTEMD_CGROUP_CONTROLLER)) {
+                r = cg_hybrid_unified();
                 if (r < 0)
-                        log_debug_errno(r, "Failed to set task access on compatibility systemd cgroup %s, ignoring: %m", path);
+                        return r;
+                if (r > 0) {
+                        /* Always propagate access mode from unified to legacy controller */
+                        r = cg_set_access(SYSTEMD_CGROUP_CONTROLLER_LEGACY, path, uid, gid);
+                        if (r < 0)
+                                log_debug_errno(r, "Failed to set access on compatibility systemd cgroup %s, ignoring: %m", path);
+                }
         }
 
         return 0;

diff --git a/src/basic/cgroup-util.h b/src/basic/cgroup-util.h
index e286e4e9c21bcc8a7776520c46176125f2d95a91..05c9f84505ea942d57bc23e48cacf5401538ed7f 100644 (file)
--- a/src/basic/cgroup-util.h
+++ b/src/basic/cgroup-util.h
@@ -188,8 +188,7 @@ int cg_set_attribute(const char *controller, const char *path, const char *attri
 int cg_get_attribute(const char *controller, const char *path, const char *attribute, char **ret);
 int cg_get_keyed_attribute(const char *controller, const char *path, const char *attribute, const char **keys, char **values);
 
-int cg_set_group_access(const char *controller, const char *path, mode_t mode, uid_t uid, gid_t gid);
-int cg_set_task_access(const char *controller, const char *path, mode_t mode, uid_t uid, gid_t gid);
+int cg_set_access(const char *controller, const char *path, uid_t uid, gid_t gid);
 
 int cg_set_xattr(const char *controller, const char *path, const char *name, const void *value, size_t size, int flags);
 int cg_get_xattr(const char *controller, const char *path, const char *name, void *value, size_t size);

diff --git a/src/core/execute.c b/src/core/execute.c
index 9bdcb1abbfedd8684af6e214c4edfe93ba439a62..2b936bcf4af3102d401e5b496e89d75281fda780 100644 (file)
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -3009,17 +3009,12 @@ static int exec_child(
                 }
         }
 
-        /* If delegation is enabled we'll pass ownership of the cgroup
-         * (but only in systemd's own controller hierarchy!) to the
-         * user of the new process. */
+        /* If delegation is enabled we'll pass ownership of the cgroup to the user of the new process. On cgroupsv1
+         * this is only about systemd's own hierarchy, i.e. not the controller hierarchies, simply because that's not
+         * safe. On cgroupsv2 there's only one hierarchy anyway, and delegation is safe there, hence in that case only
+         * touch a single hierarchy too. */
         if (params->cgroup_path && context->user && (params->flags & EXEC_CGROUP_DELEGATE)) {
-                r = cg_set_task_access(SYSTEMD_CGROUP_CONTROLLER, params->cgroup_path, 0644, uid, gid);
-                if (r < 0) {
-                        *exit_status = EXIT_CGROUP;
-                        return log_unit_error_errno(unit, r, "Failed to adjust control group access: %m");
-                }
-
-                r = cg_set_group_access(SYSTEMD_CGROUP_CONTROLLER, params->cgroup_path, 0755, uid, gid);
+                r = cg_set_access(SYSTEMD_CGROUP_CONTROLLER, params->cgroup_path, uid, gid);
                 if (r < 0) {
                         *exit_status = EXIT_CGROUP;
                         return log_unit_error_errno(unit, r, "Failed to adjust control group access: %m");