nspawn: Be more robust when deciding to create subcgroups or chown cgroups

To demonstrate the breakage in the chown part: Be using an interactive
terminal, go to spawn a shell in a container; using --register=no, and using
userns.  It will end up chown()ing the cgroup of your terminal session to the
container!  And you will be left with that after you quit the container!

Similarly. the subcgroup bit will try create subcgroups for the parent and
child even they share the cgroup with other processes (as they likely to if
--register=no); and will find only partial success, leaving the cgroup with all
controllers disabled.

What we really care about is if the child process is alone in the cgroup, so
we'll take a peek at cgroup.procs for that cgroup to find out.

3 files changed, 76 insertions, 40 deletions

diff --git a/src/nspawn/nspawn-cgroup.c b/src/nspawn/nspawn-cgroup.c
index b64475bcb4..795ad5f4ae 100644
--- a/src/nspawn/nspawn-cgroup.c
+++ b/src/nspawn/nspawn-cgroup.c
@@ -25,7 +25,9 @@
 #include "fs-util.h"
 #include "mkdir.h"
 #include "mount-util.h"
+#include "parse-util.h"
 #include "path-util.h"
+#include "process-util.h"
 #include "rm-rf.h"
 #include "string-util.h"
 #include "strv.h"
@@ -59,25 +61,6 @@ static int chown_cgroup_path(const char *path, uid_t uid_shift) {
         return 0;
 }
 
-static int chown_cgroup(pid_t pid, uid_t uid_shift) {
-        _cleanup_free_ char *path = NULL, *fs = NULL;
-        int r;
-
-        r = cg_pid_get_path(NULL, pid, &path);
-        if (r < 0)
-                return log_error_errno(r, "Failed to get host cgroup of the container: %m");
-
-        r = cg_get_path(SYSTEMD_CGROUP_CONTROLLER, path, NULL, &fs);
-        if (r < 0)
-                return log_error_errno(r, "Failed to get host file system path for container cgroup: %m");
-
-        r = chown_cgroup_path(fs, uid_shift);
-        if (r < 0)
-                return log_error_errno(r, "Failed to chown() cgroup %s: %m", fs);
-
-        return 0;
-}
-
 static int sync_cgroup(pid_t pid, CGroupUnified outer_cgver, CGroupUnified inner_cgver, uid_t uid_shift) {
         _cleanup_free_ char *cgroup = NULL;
         char mountpoint[] = "/tmp/containerXXXXXX", pid_string[DECIMAL_STR_MAX(pid) + 1];
@@ -87,13 +70,6 @@ static int sync_cgroup(pid_t pid, CGroupUnified outer_cgver, CGroupUnified inner
 
 #define LOG_PFIX "PID " PID_FMT ": sync host cgroup -> container cgroup"
 
-        unified = cg_unified(SYSTEMD_CGROUP_CONTROLLER);
-        if (unified < 0)
-                return log_error_errno(unified, LOG_PFIX ": failed to determine host cgroup version: %m", pid);
-
-        if ((outer_cgver >= CGROUP_UNIFIED_SYSTEMD) == (inner_cgver >= CGROUP_UNIFIED_SYSTEMD))
-                return 0;
-
         /* When the host uses the legacy cgroup setup, but the
          * container shall use the unified hierarchy, let's make sure
          * we copy the path from the name=systemd hierarchy into the
@@ -168,9 +144,6 @@ static int create_subcgroup(pid_t pid, CGroupUnified outer_cgver, CGroupUnified
          * did not create a scope unit for the container move us and
          * the container into two separate subcgroups. */
 
-        if (inner_cgver == CGROUP_UNIFIED_NONE || outer_cgver == CGROUP_UNIFIED_NONE)
-                return 0;
-
         r = cg_mask_supported(&supported);
         if (r < 0)
                 return log_error_errno(r, "Failed to create host subcgroup: Failed to determine supported controllers: %m");
@@ -194,22 +167,85 @@ static int create_subcgroup(pid_t pid, CGroupUnified outer_cgver, CGroupUnified
         return 0;
 }
 
-int cgroup_setup(pid_t pid, CGroupUnified outer_cgver, CGroupUnified inner_cgver, uid_t uid_shift, bool keep_unit) {
-        int r;
+static int cgpath_count_procs(const char *cgpath, Set **ret_pids) {
+        char line[LINE_MAX];
+        _cleanup_set_free_ Set *pid_set = NULL;
+        _cleanup_fclose_ FILE *procs = NULL;
 
-        r = sync_cgroup(pid, outer_cgver, inner_cgver, uid_shift);
-        if (r < 0)
-                return r;
+        pid_set = set_new(NULL);
+        if (!pid_set)
+                return -ENOMEM;
+
+        procs = fopen(prefix_roota(cgpath, "cgroup.procs"), "re");
+        if (!procs)
+                return -errno;
 
-        if (keep_unit) {
-                r = create_subcgroup(pid, outer_cgver, inner_cgver);
+        FOREACH_LINE(line, procs, return -errno) {
+                int r;
+                pid_t pid;
+
+                truncate_nl(line);
+                r = parse_pid(line, &pid);
                 if (r < 0)
                         return r;
+                set_put(pid_set, PID_TO_PTR(pid));
         }
 
-        r = chown_cgroup(pid, uid_shift);
+        if (ret_pids) {
+                *ret_pids = pid_set;
+                pid_set = NULL;
+        }
+        return 0;
+}
+
+int cgroup_setup(pid_t pid, CGroupUnified outer_cgver, CGroupUnified inner_cgver, uid_t uid_shift) {
+        _cleanup_free_ char *cgpath = NULL, *cgroup = NULL;
+        _cleanup_set_free_ Set *peers = NULL;
+        int r;
+
+        if ((outer_cgver >= CGROUP_UNIFIED_SYSTEMD) != (inner_cgver >= CGROUP_UNIFIED_SYSTEMD)) {
+                /* sync the name=systemd hierarchy with the unified hierarchy */
+                r = sync_cgroup(pid, outer_cgver, inner_cgver, uid_shift);
+                if (r < 0)
+                        return r;
+        }
+
+        r = cg_pid_get_path(SYSTEMD_CGROUP_CONTROLLER, pid, &cgroup);
         if (r < 0)
-                return r;
+                return log_error_errno(r, "Failed to get host cgroup of the container: %m");
+
+        r = cg_get_path(SYSTEMD_CGROUP_CONTROLLER, cgroup, NULL, &cgpath);
+        if (r < 0)
+                return log_error_errno(r, "Failed to get host file system path for container cgroup: %m");
+
+        r = cgpath_count_procs(cgpath, &peers);
+        if (r < 0)
+                return log_error_errno(r, "Unable to count the processes in the container's cgroup: %m");
+
+        /* This applies only to the unified hierarchy */
+        if (outer_cgver >= CGROUP_UNIFIED_SYSTEMD && inner_cgver >= CGROUP_UNIFIED_SYSTEMD) {
+                if (set_size(peers) == 2 && set_contains(peers, PID_TO_PTR(getpid()))) {
+                        char *tmp;
+
+                        r = create_subcgroup(pid, outer_cgver, inner_cgver);
+                        if (r < 0)
+                                return r;
+
+                        set_remove(peers, PID_TO_PTR(getpid()));
+                        tmp = strappend(cgpath, "/payload");
+                        if (!tmp)
+                                return log_oom();
+                        cgpath = tmp;
+                }
+        }
+
+        /* This applies only to the name=systemd/unified hierarchy, but IMO it should apply to all
+         * hierarchies. */
+        if (uid_shift != UID_INVALID && set_size(peers) == 1) {
+                r = chown_cgroup_path(cgpath, uid_shift);
+                if (r < 0)
+                        return log_error_errno(r, "Failed to chown() cgroup %s: %m", cgpath);
+        }
 
         return 0;
 }
diff --git a/src/nspawn/nspawn-cgroup.h b/src/nspawn/nspawn-cgroup.h
index b141c4e5d3..e677766726 100644
--- a/src/nspawn/nspawn-cgroup.h
+++ b/src/nspawn/nspawn-cgroup.h
@@ -24,7 +24,7 @@
 
 #include "cgroup-util.h"
 
-int cgroup_setup(pid_t pid, CGroupUnified outer_cgver, CGroupUnified inner_cgver, uid_t uid_shift, bool keep_unit);
+int cgroup_setup(pid_t pid, CGroupUnified outer_cgver, CGroupUnified inner_cgver, uid_t uid_shift);
 
 int mount_cgroups(const char *dest, CGroupUnified outer_cgver, CGroupUnified inner_cgver, bool userns, uid_t uid_shift, uid_t uid_range, const char *selinux_apifs_context, bool use_cgns);
 int mount_systemd_cgroup_writable(const char *dest, CGroupUnified inner_cgver);
diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c
index 33863b01b3..caf8ba3868 100644
--- a/src/nspawn/nspawn.c
+++ b/src/nspawn/nspawn.c
@@ -3875,7 +3875,7 @@ static int run(int master,
                         return r;
         }
 
-        r = cgroup_setup(*pid, outer_cgver, arg_unified_cgroup_hierarchy, arg_uid_shift, arg_keep_unit);
+        r = cgroup_setup(*pid, outer_cgver, arg_unified_cgroup_hierarchy, arg_uid_shift);
         if (r < 0)
                 return r;