cgroupfs: Return the correct cgroup path for tasks from procfs.

Previously we were returning the path to the control file used to
migrate a task, rather than the path to the cgroup directory.

PiperOrigin-RevId: 438676422

pkg/sentry/fsimpl/cgroupfs/base.go

@@ -283,9 +283,15 @@ func (c *cgroupInode) AbortMigrate(t *kernel.Task, src *kernel.Cgroup) {
 	}
 }
 
-func (c *cgroupInode) Cgroup(fd *vfs.FileDescription) kernel.Cgroup {
+func (c *cgroupInode) CgroupFromControlFileFD(fd *vfs.FileDescription) kernel.Cgroup {
+	controlFileDentry := fd.Dentry().Impl().(*kernfs.Dentry)
+	// The returned parent dentry remains valid without holding locks because in
+	// cgroupfs, the parent directory relationship of a control file is
+	// effectively immutable. Control files cannot be unlinked, renamed or
+	// destroyed independently from their parent directory.
+	parentD := controlFileDentry.Parent()
 	return kernel.Cgroup{
-		Dentry:     fd.Dentry().Impl().(*kernfs.Dentry),
+		Dentry:     parentD,
 		CgroupImpl: c,
 	}
 }
@@ -340,7 +346,7 @@ func (d *cgroupProcsData) Write(ctx context.Context, fd *vfs.FileDescription, sr
 	if targetTG == nil {
 		return 0, linuxerr.EINVAL
 	}
-	return n, targetTG.MigrateCgroup(d.Cgroup(fd))
+	return n, targetTG.MigrateCgroup(d.CgroupFromControlFileFD(fd))
 }
 
 // +stateify savable
@@ -382,7 +388,7 @@ func (d *tasksData) Write(ctx context.Context, fd *vfs.FileDescription, src user
 	if targetTask == nil {
 		return 0, linuxerr.EINVAL
 	}
-	return n, targetTask.MigrateCgroup(d.Cgroup(fd))
+	return n, targetTask.MigrateCgroup(d.CgroupFromControlFileFD(fd))
 }
 
 // parseInt64FromString interprets src as string encoding a int64 value, and

pkg/sentry/fsimpl/kernfs/kernfs.go

@@ -600,6 +600,13 @@ func (d *Dentry) WalkDentryTree(ctx context.Context, vfsObj *vfs.VirtualFilesyst
 	return target, nil
 }
 
+// Parent returns the parent of this Dentry. This is not safe in general, the
+// filesystem may concurrently move d elsewhere. The caller is responsible for
+// ensuring the returned result remains valid while it is used.
+func (d *Dentry) Parent() *Dentry {
+	return d.parent
+}
+
 // The Inode interface maps filesystem-level operations that operate on paths to
 // equivalent operations on specific filesystem nodes.
 //

test/syscalls/linux/cgroup.cc

@@ -258,7 +258,7 @@ TEST(Cgroup, MountRace) {
     }
   });
 
-  Cgroup c = Cgroup(mountpoint.path());
+  Cgroup c = Cgroup::RootCgroup(mountpoint.path());
   // c should be a valid cgroup.
   EXPECT_NO_ERRNO(c.ContainsCallingProcess());
 }
@@ -960,6 +960,67 @@ TEST(ProcCgroup, ProcfsReportsCgroupfsMountOptions) {
   }
 }
 
+TEST(ProcCgroups, ProcfsRreportsHierarchyID) {
+  SKIP_IF(!CgroupsAvailable());
+
+  Mounter m(ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir()));
+  Cgroup h1 = ASSERT_NO_ERRNO_AND_VALUE(m.MountCgroupfs("memory,cpuacct"));
+  Cgroup h2 = ASSERT_NO_ERRNO_AND_VALUE(m.MountCgroupfs("cpu"));
+
+  absl::flat_hash_map<std::string, CgroupsEntry> entries =
+      ASSERT_NO_ERRNO_AND_VALUE(ProcCgroupsEntries());
+
+  EXPECT_EQ(entries["memory"].hierarchy, entries["cpuacct"].hierarchy);
+
+  // Hierarhcy IDs are allocated sequentially, starting at 1.
+  EXPECT_EQ(entries["memory"].hierarchy, 1);
+  EXPECT_EQ(entries["cpu"].hierarchy, 2);
+}
+
+TEST(ProcCgroups, ProcfsReportsTasksCgroup) {
+  SKIP_IF(!CgroupsAvailable());
+
+  Mounter m(ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir()));
+  Cgroup h1 = ASSERT_NO_ERRNO_AND_VALUE(m.MountCgroupfs("memory"));
+  Cgroup h2 = ASSERT_NO_ERRNO_AND_VALUE(m.MountCgroupfs("cpu,cpuacct"));
+
+  Cgroup h1c1 = ASSERT_NO_ERRNO_AND_VALUE(h1.CreateChild("memory_child1"));
+  Cgroup h1c2 = ASSERT_NO_ERRNO_AND_VALUE(h1.CreateChild("memory_child2"));
+
+  Cgroup h2c1 = ASSERT_NO_ERRNO_AND_VALUE(h2.CreateChild("cpu_child1"));
+  Cgroup h2c2 = ASSERT_NO_ERRNO_AND_VALUE(h2.CreateChild("cpu_child2"));
+
+  // Test should initially be in the hierarchy roots.
+  auto entries = ASSERT_NO_ERRNO_AND_VALUE(ProcPIDCgroupEntries(getpid()));
+  EXPECT_EQ(h1.CanonicalPath(), entries["memory"].path);
+  EXPECT_EQ(h2.CanonicalPath(), entries["cpu,cpuacct"].path);
+
+  // Move to child for hierarchy #1 and check paths. Note that we haven't moved
+  // in hierarchy #2.
+  ASSERT_NO_ERRNO(h1c1.Enter(getpid()));
+  entries = ASSERT_NO_ERRNO_AND_VALUE(ProcPIDCgroupEntries(getpid()));
+  EXPECT_EQ(h1c1.CanonicalPath(), entries["memory"].path);
+  EXPECT_EQ(h2.CanonicalPath(), entries["cpu,cpuacct"].path);
+
+  // Move h2; h1 should remain unchanged.
+  ASSERT_NO_ERRNO(h2c1.Enter(getpid()));
+  entries = ASSERT_NO_ERRNO_AND_VALUE(ProcPIDCgroupEntries(getpid()));
+  EXPECT_EQ(h1c1.CanonicalPath(), entries["memory"].path);
+  EXPECT_EQ(h2c1.CanonicalPath(), entries["cpu,cpuacct"].path);
+
+  // Move the thread rather than process group.
+  const pid_t tid = syscall(SYS_gettid);
+  ASSERT_NO_ERRNO(h1c2.EnterThread(tid));
+  entries = ASSERT_NO_ERRNO_AND_VALUE(ProcPIDCgroupEntries(tid));
+  EXPECT_EQ(h1c2.CanonicalPath(), entries["memory"].path);
+  EXPECT_EQ(h2c1.CanonicalPath(), entries["cpu,cpuacct"].path);
+
+  ASSERT_NO_ERRNO(h2c2.EnterThread(tid));
+  entries = ASSERT_NO_ERRNO_AND_VALUE(ProcPIDCgroupEntries(tid));
+  EXPECT_EQ(h1c2.CanonicalPath(), entries["memory"].path);
+  EXPECT_EQ(h2c2.CanonicalPath(), entries["cpu,cpuacct"].path);
+}
+
 }  // namespace
 }  // namespace testing
 }  // namespace gvisor

test/util/cgroup_util.cc

@@ -25,26 +25,19 @@
 namespace gvisor {
 namespace testing {
 
-Cgroup::Cgroup(absl::string_view path) : cgroup_path_(path) {
+Cgroup::Cgroup(absl::string_view path, absl::string_view mountpoint)
+    : cgroup_path_(path), mountpoint_(mountpoint) {
   id_ = ++Cgroup::next_id_;
   std::cerr << absl::StreamFormat("[cg#%d] <= %s", id_, cgroup_path_)
             << std::endl;
 }
 
-PosixErrorOr<Cgroup> Cgroup::RecursivelyCreate(absl::string_view path) {
-  RETURN_IF_ERRNO(RecursivelyCreateDir(path));
-  return Cgroup(path);
-}
-
-PosixErrorOr<Cgroup> Cgroup::Create(absl::string_view path) {
-  RETURN_IF_ERRNO(Mkdir(path));
-  return Cgroup(path);
-}
-
 PosixError Cgroup::Delete() { return Rmdir(cgroup_path_); }
 
 PosixErrorOr<Cgroup> Cgroup::CreateChild(absl::string_view name) const {
-  return Cgroup::Create(JoinPath(Path(), name));
+  std::string path = JoinPath(Path(), name);
+  RETURN_IF_ERRNO(Mkdir(path));
+  return Cgroup(path, mountpoint_);
 }
 
 PosixErrorOr<std::string> Cgroup::ReadControlFile(
@@ -183,7 +176,7 @@ PosixErrorOr<Cgroup> Mounter::MountCgroupfs(std::string mopts) {
                    "Mount(\"none\", \"%s\", \"cgroup\", 0, \"%s\", 0) => OK",
                    mountpath, mopts)
             << std::endl;
-  Cgroup cg = Cgroup(mountpath);
+  Cgroup cg = Cgroup::RootCgroup(mountpath);
   mountpoints_[cg.id()] = std::move(mountpoint);
   mounts_[cg.id()] = std::move(mount);
   return cg;

test/util/cgroup_util.h

@@ -30,24 +30,28 @@ namespace testing {
 // Cgroup represents a cgroup directory on a mounted cgroupfs.
 class Cgroup {
  public:
-  Cgroup(std::string_view path);
+  static Cgroup RootCgroup(absl::string_view path) {
+    return Cgroup(path, path);
+  }
 
   uint64_t id() const { return id_; }
 
-  // RecursivelyCreate creates cgroup specified by path, including all
-  // components leading up to path. Path should end inside a cgroupfs mount. If
-  // path already exists, RecursivelyCreate does nothing and silently succeeds.
-  static PosixErrorOr<Cgroup> RecursivelyCreate(std::string_view path);
-
-  // Creates a new cgroup at path. The parent directory must exist and be a
-  // cgroupfs directory.
-  static PosixErrorOr<Cgroup> Create(std::string_view path);
-
   // Deletes the current cgroup represented by this object.
   PosixError Delete();
 
   const std::string& Path() const { return cgroup_path_; }
 
+  // Returns the canonical path for this cgroup, which is the absolute path
+  // starting at the hierarchy root.
+  const std::string CanonicalPath() const {
+    std::string relpath =
+        GetRelativePath(mountpoint_, cgroup_path_).ValueOrDie();
+    if (relpath == ".") {
+      return "/";
+    }
+    return "/" + relpath;
+  }
+
   // Creates a child cgroup under this cgroup with the given name.
   PosixErrorOr<Cgroup> CreateChild(std::string_view name) const;
 
@@ -95,12 +99,15 @@ class Cgroup {
   PosixError EnterThread(pid_t pid) const;
 
  private:
+  Cgroup(std::string_view path, std::string_view mountpoint);
+
   PosixErrorOr<absl::flat_hash_set<pid_t>> ParsePIDList(
       absl::string_view data) const;
 
   static int64_t next_id_;
   int64_t id_;
   const std::string cgroup_path_;
+  const std::string mountpoint_;
 };
 
 // Mounter is a utility for creating cgroupfs mounts. It automatically manages