diff --git a/pkg/sentry/fsimpl/cgroupfs/base.go b/pkg/sentry/fsimpl/cgroupfs/base.go index ee19e5e44..862c82a33 100644 --- a/pkg/sentry/fsimpl/cgroupfs/base.go +++ b/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 diff --git a/pkg/sentry/fsimpl/kernfs/kernfs.go b/pkg/sentry/fsimpl/kernfs/kernfs.go index 544698694..9a349b188 100644 --- a/pkg/sentry/fsimpl/kernfs/kernfs.go +++ b/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. // diff --git a/test/syscalls/linux/cgroup.cc b/test/syscalls/linux/cgroup.cc index a950f0989..224c4d6ad 100644 --- a/test/syscalls/linux/cgroup.cc +++ b/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 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 diff --git a/test/util/cgroup_util.cc b/test/util/cgroup_util.cc index e0e423f97..4e687481f 100644 --- a/test/util/cgroup_util.cc +++ b/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::RecursivelyCreate(absl::string_view path) { - RETURN_IF_ERRNO(RecursivelyCreateDir(path)); - return Cgroup(path); -} - -PosixErrorOr Cgroup::Create(absl::string_view path) { - RETURN_IF_ERRNO(Mkdir(path)); - return Cgroup(path); -} - PosixError Cgroup::Delete() { return Rmdir(cgroup_path_); } PosixErrorOr 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 Cgroup::ReadControlFile( @@ -183,7 +176,7 @@ PosixErrorOr 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; diff --git a/test/util/cgroup_util.h b/test/util/cgroup_util.h index 998410c3c..f45945b3e 100644 --- a/test/util/cgroup_util.h +++ b/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 RecursivelyCreate(std::string_view path); - - // Creates a new cgroup at path. The parent directory must exist and be a - // cgroupfs directory. - static PosixErrorOr 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 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> 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