From 49ae54c01067e77f1b1cd80e65358bf12a526456 Mon Sep 17 00:00:00 2001 From: Shambhavi Srivastava Date: Fri, 22 Apr 2022 13:17:12 -0700 Subject: [PATCH] Fixing memory allocations for tmpfs size. Fixing partial write memory allocation. Accounting for memory allocation in hard links. Adding test cases to check for hard link memory allocation for the same. PiperOrigin-RevId: 443742013 --- pkg/sentry/fsimpl/tmpfs/filesystem.go | 19 --------------- pkg/sentry/fsimpl/tmpfs/regular_file.go | 16 ++++++++----- pkg/sentry/fsimpl/tmpfs/tmpfs.go | 18 ++++++++++++-- test/syscalls/linux/mount.cc | 32 +++++++++++++++++++++++++ 4 files changed, 58 insertions(+), 27 deletions(-) diff --git a/pkg/sentry/fsimpl/tmpfs/filesystem.go b/pkg/sentry/fsimpl/tmpfs/filesystem.go index 03a086e57..16ba75647 100644 --- a/pkg/sentry/fsimpl/tmpfs/filesystem.go +++ b/pkg/sentry/fsimpl/tmpfs/filesystem.go @@ -800,25 +800,6 @@ func (fs *filesystem) UnlinkAt(ctx context.Context, rp *vfs.ResolvingPath) error if err := vfsObj.PrepareDeleteDentry(mntns, &child.vfsd); err != nil { return err } - // Remove pages used if child being removed is a SymLink or Regular File. - switch impl := child.inode.impl.(type) { - case *symlink: - if len(impl.target) >= shortSymlinkLen { - if err := fs.updatePagesUsed(uint64(len(impl.target)), 0); err != nil { - vfsObj.AbortDeleteDentry(&child.vfsd) - return err - } - } - case *regularFile: - impl.inode.mu.Lock() - if err := fs.updatePagesUsed(impl.size.Load(), 0); err != nil { - impl.inode.mu.Unlock() - vfsObj.AbortDeleteDentry(&child.vfsd) - return err - } - impl.inode.mu.Unlock() - } - // Generate inotify events. Note that this must take place before the link // count of the child is decremented, or else the watches may be dropped // before these events are added. diff --git a/pkg/sentry/fsimpl/tmpfs/regular_file.go b/pkg/sentry/fsimpl/tmpfs/regular_file.go index e6c7d8762..b59a4c806 100644 --- a/pkg/sentry/fsimpl/tmpfs/regular_file.go +++ b/pkg/sentry/fsimpl/tmpfs/regular_file.go @@ -443,7 +443,8 @@ func (fd *regularFileFD) pwrite(ctx context.Context, src usermem.IOSequence, off // Locking f.inode.mu is sufficient for reading f.size. offset = int64(f.size.RacyLoad()) } - if end := offset + srclen; end < offset { + end := offset + srclen + if end < offset { // Overflow. return 0, offset, linuxerr.EINVAL } @@ -452,15 +453,18 @@ func (fd *regularFileFD) pwrite(ctx context.Context, src usermem.IOSequence, off if err != nil { return 0, offset, err } + maybeSizeInc := false src = src.TakeFirst64(srclen) - reservedSize := f.size.Load() + uint64(srclen) - if err = f.inode.fs.updatePagesUsed(f.size.Load(), reservedSize); err != nil { - return 0, 0, err + if uint64(end) > f.size.Load() { + maybeSizeInc = true + if err = f.inode.fs.updatePagesUsed(f.size.Load(), uint64(end)); err != nil { + return 0, 0, err + } } rw := getRegularFileReadWriter(f, offset) n, err := src.CopyInTo(ctx, rw) - if unwritten := srclen - n; unwritten != 0 { - if err := f.inode.fs.updatePagesUsed(reservedSize, f.size.Load()); err != nil { + if unwritten := srclen - n; maybeSizeInc && unwritten != 0 { + if err := f.inode.fs.updatePagesUsed(uint64(end), f.size.Load()); err != nil { return 0, 0, err } } diff --git a/pkg/sentry/fsimpl/tmpfs/tmpfs.go b/pkg/sentry/fsimpl/tmpfs/tmpfs.go index bcc0cc924..d535f0b73 100644 --- a/pkg/sentry/fsimpl/tmpfs/tmpfs.go +++ b/pkg/sentry/fsimpl/tmpfs/tmpfs.go @@ -528,12 +528,26 @@ func (i *inode) tryIncRef() bool { func (i *inode) decRef(ctx context.Context) { i.refs.DecRef(func() { i.watches.HandleDeletion(ctx) - if regFile, ok := i.impl.(*regularFile); ok { + // Remove pages used if child being removed is a SymLink or Regular File. + switch impl := i.impl.(type) { + case *symlink: + if len(impl.target) >= shortSymlinkLen { + if err := i.fs.updatePagesUsed(uint64(len(impl.target)), 0); err != nil { + panic(fmt.Sprintf("Encountered error: %v while accounting tmpfs size.", err)) + } + } + case *regularFile: + impl.inode.mu.Lock() + if err := i.fs.updatePagesUsed(impl.size.Load(), 0); err != nil { + panic(fmt.Sprintf("Encountered error: %v while accounting tmpfs size.", err)) + } + impl.inode.mu.Unlock() // Release memory used by regFile to store data. Since regFile is // no longer usable, we don't need to grab any locks or update any // metadata. - regFile.data.DropAll(regFile.memFile) + impl.data.DropAll(impl.memFile) } + }) } diff --git a/test/syscalls/linux/mount.cc b/test/syscalls/linux/mount.cc index 36a90ad93..0b127643f 100644 --- a/test/syscalls/linux/mount.cc +++ b/test/syscalls/linux/mount.cc @@ -582,6 +582,38 @@ TEST(MountTest, TmpfsSymlinkAllocCheck) { EXPECT_THAT(symlink(target.c_str(), pathname.c_str()), SyscallSucceeds()); } +// Tests memory allocation for Hard Links is not double allocated. +TEST(MountTest, TmpfsHardLinkAllocCheck) { + SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN))); + auto const dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir()); + auto tmpfs_size_opt = absl::StrCat("size=", kPageSize); + auto const mount = ASSERT_NO_ERRNO_AND_VALUE( + Mount("", dir.path(), "tmpfs", 0, tmpfs_size_opt, 0)); + const std::string fileOne = JoinPath(dir.path(), "foo1"); + const std::string fileTwo = JoinPath(dir.path(), "foo2"); + auto const fd = + ASSERT_NO_ERRNO_AND_VALUE(Open(fileOne, O_CREAT | O_RDWR, 0777)); + EXPECT_THAT(link(fileOne.c_str(), fileTwo.c_str()), SyscallSucceeds()); + + // Check that it starts at size zero. + struct stat buf; + ASSERT_THAT(fstat(fd.get(), &buf), SyscallSucceeds()); + EXPECT_EQ(buf.st_size, 0); + + // Grow to 1 Page Size. + ASSERT_THAT(fallocate(fd.get(), 0, 0, kPageSize), SyscallSucceeds()); + ASSERT_THAT(fstat(fd.get(), &buf), SyscallSucceeds()); + EXPECT_EQ(buf.st_size, kPageSize); + + // Grow to size beyond tmpfs allocated bytes. + ASSERT_THAT(fallocate(fd.get(), 0, 0, kPageSize + 1), + SyscallFailsWithErrno(ENOSPC)); + ASSERT_THAT(fstat(fd.get(), &buf), SyscallSucceeds()); + EXPECT_EQ(buf.st_size, kPageSize); + EXPECT_THAT(unlink(fileTwo.c_str()), SyscallSucceeds()); + EXPECT_THAT(unlink(fileOne.c_str()), SyscallSucceeds()); +} + } // namespace } // namespace testing