From 2ba23ca96c2c1d0e561490da8ae36ba8f1bc59d9 Mon Sep 17 00:00:00 2001 From: Rahat Mahmood Date: Wed, 27 Apr 2022 14:17:35 -0700 Subject: [PATCH] kernfs/cgroupfs: Fix rename. Rename was broken for serveral reasons: - The underlying kernfs.OrderedChildren.Rename had a bug so it could never successfully extract OrderedChildren from the dst Inode. - kernfs.OrderedChildren.Rename didn't check for name collisions on the dst. cgroupfs is the first kernfs-based filesystem to implement rename, so these code paths weren't exercised. PiperOrigin-RevId: 444964501 --- pkg/sentry/fsimpl/kernfs/inode_impl_util.go | 58 +++++++++++--------- test/syscalls/linux/cgroup.cc | 59 +++++++++++++++++++++ 2 files changed, 93 insertions(+), 24 deletions(-) diff --git a/pkg/sentry/fsimpl/kernfs/inode_impl_util.go b/pkg/sentry/fsimpl/kernfs/inode_impl_util.go index 8530464eb..d002f58db 100644 --- a/pkg/sentry/fsimpl/kernfs/inode_impl_util.go +++ b/pkg/sentry/fsimpl/kernfs/inode_impl_util.go @@ -400,6 +400,16 @@ type OrderedChildrenOptions struct { Writable bool } +// inodeWithOrderedChildren allows extraction of an OrderedChildren from an +// Inode implementation. A concrete type that both implements the Inode +// interface and embeds OrderedChildren will be castable to this interface, and +// we can get to the embedded OrderedChildren through the orderedChildren +// method. +type inodeWithOrderedChildren interface { + Inode + orderedChildren() *OrderedChildren +} + // OrderedChildren partially implements the Inode interface. OrderedChildren can // be embedded in directory inodes to keep track of children in the // directory, and can then be used to implement a generic directory FD -- see @@ -427,6 +437,11 @@ type OrderedChildren struct { set map[string]*slot } +// orderedChildren implements inodeWithOrderedChildren.orderedChildren. +func (o *OrderedChildren) orderedChildren() *OrderedChildren { + return o +} + // Init initializes an OrderedChildren. func (o *OrderedChildren) Init(opts OrderedChildrenOptions) { o.writable = opts.Writable @@ -572,27 +587,6 @@ func (o *OrderedChildren) removeLocked(name string) { } } -// Precondition: caller must hold o.mu for writing. -func (o *OrderedChildren) replaceChildLocked(ctx context.Context, name string, newI Inode) { - if s, ok := o.set[name]; ok { - if s.static { - panic(fmt.Sprintf("replacing a static inode: %v", s.inode)) - } - - // Existing slot with given name, simply replace the dentry. - s.inode = newI - } - - // No existing slot with given name, create and hash new slot. - s := &slot{ - name: name, - inode: newI, - static: false, - } - o.order.PushBack(s) - o.set[name] = s -} - // Precondition: caller must hold o.mu for reading or writing. func (o *OrderedChildren) checkExistingLocked(name string, child Inode) error { s, ok := o.set[name] @@ -640,11 +634,11 @@ func (o *OrderedChildren) Rename(ctx context.Context, oldname, newname string, c if !o.writable { return linuxerr.EPERM } - - dst, ok := dstDir.(interface{}).(*OrderedChildren) + dstIOC, ok := dstDir.(inodeWithOrderedChildren) if !ok { return linuxerr.EXDEV } + dst := dstIOC.orderedChildren() if !dst.writable { return linuxerr.EPERM } @@ -659,12 +653,28 @@ func (o *OrderedChildren) Rename(ctx context.Context, oldname, newname string, c dst.mu.Lock() defer dst.mu.Unlock() } + + // Ensure target inode exists in src. if err := o.checkExistingLocked(oldname, child); err != nil { return err } + + // Ensure no name collision in dst. + if _, ok := dst.set[newname]; ok { + return linuxerr.EEXIST + } + + // Remove from src. o.removeLocked(oldname) - dst.replaceChildLocked(ctx, newname, child) + // Add to dst. + s := &slot{ + name: newname, + inode: child, + } + dst.order.PushBack(s) + dst.set[newname] = s + return nil } diff --git a/test/syscalls/linux/cgroup.cc b/test/syscalls/linux/cgroup.cc index 421df540e..812c10c94 100644 --- a/test/syscalls/linux/cgroup.cc +++ b/test/syscalls/linux/cgroup.cc @@ -490,6 +490,65 @@ TEST(Cgroup, MkdirWithPermissions) { EXPECT_TRUE(S_ISDIR(s2.st_mode)); } +TEST(Cgroup, CantRenameControlFile) { + SKIP_IF(!CgroupsAvailable()); + Mounter m(ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir())); + Cgroup c = ASSERT_NO_ERRNO_AND_VALUE(m.MountCgroupfs("")); + + const std::string control_file_path = c.Relpath("cgroup.procs"); + EXPECT_THAT( + rename(c.Relpath("cgroup.procs").c_str(), c.Relpath("foo").c_str()), + SyscallFailsWithErrno(ENOTDIR)); +} + +TEST(Cgroup, CrossDirRenameNotAllowed) { + SKIP_IF(!CgroupsAvailable()); + Mounter m(ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir())); + Cgroup c = ASSERT_NO_ERRNO_AND_VALUE(m.MountCgroupfs("")); + + Cgroup dir1 = ASSERT_NO_ERRNO_AND_VALUE(c.CreateChild("dir1")); + Cgroup dir2 = ASSERT_NO_ERRNO_AND_VALUE(c.CreateChild("dir2")); + + Cgroup target = ASSERT_NO_ERRNO_AND_VALUE(dir1.CreateChild("target")); + // Move to sibling directory. + EXPECT_THAT(rename(target.Path().c_str(), dir2.Relpath("target").c_str()), + SyscallFailsWithErrno(EIO)); + // Move to parent directory. + EXPECT_THAT(rename(target.Path().c_str(), c.Relpath("target").c_str()), + SyscallFailsWithErrno(EIO)); + + // Original directory unaffected. + EXPECT_THAT(Exists(target.Path()), IsPosixErrorOkAndHolds(true)); +} + +TEST(Cgroup, RenameNameCollision) { + SKIP_IF(!CgroupsAvailable()); + Mounter m(ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir())); + Cgroup c = ASSERT_NO_ERRNO_AND_VALUE(m.MountCgroupfs("")); + + Cgroup dir1 = ASSERT_NO_ERRNO_AND_VALUE(c.CreateChild("dir1")); + Cgroup dir2 = ASSERT_NO_ERRNO_AND_VALUE(c.CreateChild("dir2")); + + // Collision with dir. + EXPECT_THAT(rename(dir1.Path().c_str(), dir2.Path().c_str()), + SyscallFailsWithErrno(EEXIST)); + // Collision with control file. + EXPECT_THAT(rename(dir1.Path().c_str(), c.Relpath("cgroup.procs").c_str()), + SyscallFailsWithErrno(EEXIST)); +} + +TEST(Cgroup, Rename) { + SKIP_IF(!CgroupsAvailable()); + Mounter m(ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir())); + Cgroup c = ASSERT_NO_ERRNO_AND_VALUE(m.MountCgroupfs("")); + Cgroup child = ASSERT_NO_ERRNO_AND_VALUE(c.CreateChild("child")); + Cgroup target = ASSERT_NO_ERRNO_AND_VALUE(child.CreateChild("oldname")); + ASSERT_THAT(rename(target.Path().c_str(), child.Relpath("newname").c_str()), + SyscallSucceeds()); + EXPECT_THAT(Exists(child.Relpath("newname")), IsPosixErrorOkAndHolds(true)); + EXPECT_THAT(Exists(child.Relpath("oldname")), IsPosixErrorOkAndHolds(false)); +} + TEST(MemoryCgroup, MemoryUsageInBytes) { SKIP_IF(!CgroupsAvailable());