Merge pull request #12771 from tanyifeng:tmpfs-otrunc-mtime

PiperOrigin-RevId: 892618235

Author: gvisor-bot

pkg/sentry/fsimpl/tmpfs/filesystem.go

@@ -468,7 +468,8 @@ func (d *dentry) open(ctx context.Context, rp *vfs.ResolvingPath, opts *vfs.Open
 				fd.vfsfd.DecRef(ctx)
 				return nil, err
 			}
-			_, err := impl.truncate(0)
+			// truncate updates mtime/ctime internally.
+			err := impl.truncate(0)
 			mnt.EndWrite()
 			if err != nil {
 				fd.vfsfd.DecRef(ctx)

pkg/sentry/fsimpl/tmpfs/regular_file.go

@@ -173,12 +173,16 @@ func NewMemfd(ctx context.Context, creds *auth.Credentials, mount *vfs.Mount, al
 	return &fd.vfsfd, nil
 }
 
-// truncate grows or shrinks the file to the given size. It returns true if the
-// file size was updated.
-func (rf *regularFile) truncate(newSize uint64) (bool, error) {
+// truncate grows or shrinks the file to the given size. It unconditionally
+// updates mtime and ctime.
+func (rf *regularFile) truncate(newSize uint64) error {
 	rf.inode.mu.Lock()
 	defer rf.inode.mu.Unlock()
-	return rf.truncateLocked(newSize)
+	if err := rf.truncateNoTimeUpdateLocked(newSize); err != nil {
+		return err
+	}
+	rf.inode.touchCMtimeLocked()
+	return nil
 }
 
 // Preconditions:
@@ -194,26 +198,29 @@ func (rf *regularFile) growLocked(newSize uint64) error {
 	return nil
 }
 
+// truncateNoTimeUpdateLocked grows or shrinks the file to the given size.
+// Callers are responsible for updating timestamps.
+//
 // Preconditions: rf.inode.mu must be held.
-func (rf *regularFile) truncateLocked(newSize uint64) (bool, error) {
+func (rf *regularFile) truncateNoTimeUpdateLocked(newSize uint64) error {
 	oldSize := rf.size.RacyLoad()
 	if newSize == oldSize {
 		// Nothing to do.
-		return false, nil
+		return nil
 	}
 
 	// Need to hold inode.mu and dataMu while modifying size.
 	rf.dataMu.Lock()
 	if newSize > oldSize {
 		err := rf.growLocked(newSize)
 		rf.dataMu.Unlock()
-		return err == nil, err
+		return err
 	}
 
 	// We are shrinking the file. First check if this is allowed.
 	if rf.seals&linux.F_SEAL_SHRINK != 0 {
 		rf.dataMu.Unlock()
-		return false, linuxerr.EPERM
+		return linuxerr.EPERM
 	}
 
 	rf.size.Store(newSize)
@@ -238,7 +245,7 @@ func (rf *regularFile) truncateLocked(newSize uint64) (bool, error) {
 	decPages := rf.data.Truncate(newSize, rf.inode.fs.mf)
 	rf.dataMu.Unlock()
 	rf.inode.fs.unaccountPages(decPages)
-	return true, nil
+	return nil
 }
 
 // AddMapping implements memmap.Mappable.AddMapping.

pkg/sentry/fsimpl/tmpfs/regular_file_test.go

@@ -324,26 +324,4 @@ func TestTruncate(t *testing.T) {
 	if got := statAfterTruncateUp.Ctime.ToNsec(); got <= statAfterTruncateDown.Ctime.ToNsec() {
 		t.Errorf("fd.Stat got Ctime %v, want > %v", got, stat.Ctime)
 	}
-
-	// Truncate to the current size.
-	newSize = statAfterTruncateUp.Size
-	if err := fd.SetStat(ctx, vfs.SetStatOptions{
-		Stat: linux.Statx{
-			Mask: linux.STATX_SIZE,
-			Size: newSize,
-		},
-	}); err != nil {
-		t.Errorf("fd.Truncate failed: %v", err)
-	}
-	statAfterTruncateNoop, err := fd.Stat(ctx, sizeStatOpts)
-	if err != nil {
-		t.Fatalf("fd.Stat failed: %v", err)
-	}
-	// Mtime and Ctime should not be bumped, since operation is a noop.
-	if got := statAfterTruncateNoop.Mtime.ToNsec(); got != statAfterTruncateUp.Mtime.ToNsec() {
-		t.Errorf("fd.Stat got Mtime %v, want %v", got, statAfterTruncateUp.Mtime)
-	}
-	if got := statAfterTruncateNoop.Ctime.ToNsec(); got != statAfterTruncateUp.Ctime.ToNsec() {
-		t.Errorf("fd.Stat got Ctime %v, want %v", got, statAfterTruncateUp.Ctime)
-	}
 }

pkg/sentry/fsimpl/tmpfs/tmpfs.go

@@ -704,14 +704,11 @@ func (i *inode) setStat(ctx context.Context, creds *auth.Credentials, opts *vfs.
 	if mask&linux.STATX_SIZE != 0 {
 		switch impl := i.impl.(type) {
 		case *regularFile:
-			updated, err := impl.truncateLocked(stat.Size)
-			if err != nil {
+			if err := impl.truncateNoTimeUpdateLocked(stat.Size); err != nil {
 				return err
 			}
-			if updated {
-				needsMtimeBump = true
-				needsCtimeBump = true
-			}
+			needsMtimeBump = true
+			needsCtimeBump = true
 		case *directory:
 			return linuxerr.EISDIR
 		default:

test/syscalls/linux/BUILD

@@ -1596,6 +1596,7 @@ cc_binary(
         "//test/util:thread_util",
         "@com_google_absl//absl/memory",
         "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/time",
     ],
 )
 

test/syscalls/linux/open.cc

@@ -20,12 +20,15 @@
 #include <unistd.h>
 
 #include <cstdlib>
+#include <ctime>
 #include <memory>
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "absl/memory/memory.h"
 #include "absl/strings/string_view.h"
+#include "absl/time/clock.h"
+#include "absl/time/time.h"
 #include "test/syscalls/linux/file_base.h"
 #include "test/util/capability_util.h"
 #include "test/util/cleanup.h"
@@ -542,6 +545,122 @@ TEST_F(OpenTest, OPathWithODirectory) {
               SyscallFailsWithErrno(ENOTDIR));
 }
 
+// Truncate operations that don't change file size should still update mtime
+// and ctime. Tests truncation via open(O_TRUNC), ftruncate(2), and truncate(2).
+TEST_F(OpenTest, TruncateNoSizeChangeUpdatesTimestamps) {
+  // Create three zero-length files and record their initial timestamps.
+  auto path1 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
+  auto path2 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
+  auto path3 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
+
+  // When running on gVisor, only test on tmpfs. We don't know how the gofer
+  // server implements open(O_TRUNC), ftruncate(2), and truncate(2). If it
+  // uses truncate(2) under the hood and the underlying filesystem is host
+  // tmpfs, then this test will fail.
+  SKIP_IF(IsRunningOnGvisor() &&
+          !ASSERT_NO_ERRNO_AND_VALUE(IsTmpfs(path1.path())));
+
+  struct stat before1 = {}, before2 = {}, before3 = {};
+  ASSERT_THAT(stat(path1.path().c_str(), &before1), SyscallSucceeds());
+  ASSERT_THAT(stat(path2.path().c_str(), &before2), SyscallSucceeds());
+  ASSERT_THAT(stat(path3.path().c_str(), &before3), SyscallSucceeds());
+  EXPECT_EQ(before1.st_size, 0);
+  EXPECT_EQ(before2.st_size, 0);
+  EXPECT_EQ(before3.st_size, 0);
+
+  const auto ts_gt = [](const struct timespec& a, const struct timespec& b) {
+    return a.tv_sec > b.tv_sec ||
+           (a.tv_sec == b.tv_sec && a.tv_nsec > b.tv_nsec);
+  };
+
+  absl::SleepFor(absl::Milliseconds(10));
+
+  // Test 1: open(O_TRUNC) on zero-length file.
+  const FileDescriptor fd1 =
+      ASSERT_NO_ERRNO_AND_VALUE(Open(path1.path(), O_WRONLY | O_TRUNC));
+  struct stat after1 = {};
+  ASSERT_THAT(fstat(fd1.get(), &after1), SyscallSucceeds());
+  EXPECT_EQ(after1.st_size, 0);
+  EXPECT_TRUE(ts_gt(after1.st_mtim, before1.st_mtim));
+  EXPECT_TRUE(ts_gt(after1.st_ctim, before1.st_ctim));
+
+  // Test 2: ftruncate(2) to same size (0).
+  const FileDescriptor fd2 =
+      ASSERT_NO_ERRNO_AND_VALUE(Open(path2.path(), O_WRONLY));
+  ASSERT_THAT(ftruncate(fd2.get(), 0), SyscallSucceeds());
+  struct stat after2 = {};
+  ASSERT_THAT(fstat(fd2.get(), &after2), SyscallSucceeds());
+  EXPECT_EQ(after2.st_size, 0);
+  EXPECT_TRUE(ts_gt(after2.st_mtim, before2.st_mtim));
+  EXPECT_TRUE(ts_gt(after2.st_ctim, before2.st_ctim));
+
+  // Test 3: truncate(2) to same size (0). Note that Linux tmpfs does NOT
+  // update mtime/ctime for truncate(2) same-size, but ext4 does. gVisor
+  // unconditionally updates for simplicity. Do not test this on native tmpfs.
+  if (IsRunningOnGvisor() ||
+      !ASSERT_NO_ERRNO_AND_VALUE(IsTmpfs(path3.path()))) {
+    ASSERT_THAT(truncate(path3.path().c_str(), 0), SyscallSucceeds());
+    struct stat after3 = {};
+    ASSERT_THAT(stat(path3.path().c_str(), &after3), SyscallSucceeds());
+    EXPECT_EQ(after3.st_size, 0);
+    EXPECT_TRUE(ts_gt(after3.st_mtim, before3.st_mtim));
+    EXPECT_TRUE(ts_gt(after3.st_ctim, before3.st_ctim));
+  }
+}
+
+// Truncate operations that change file size should always update mtime and
+// ctime.  Tests truncation via open(O_TRUNC), ftruncate(2), and truncate(2).
+TEST_F(OpenTest, TruncateSizeChangeUpdatesTimestamps) {
+  auto path1 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFileWith(
+      GetAbsoluteTestTmpdir(), "initial data 1", 0644));
+  auto path2 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFileWith(
+      GetAbsoluteTestTmpdir(), "initial data 2", 0644));
+  auto path3 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFileWith(
+      GetAbsoluteTestTmpdir(), "initial data 3", 0644));
+
+  struct stat before1 = {}, before2 = {}, before3 = {};
+  ASSERT_THAT(stat(path1.path().c_str(), &before1), SyscallSucceeds());
+  ASSERT_THAT(stat(path2.path().c_str(), &before2), SyscallSucceeds());
+  ASSERT_THAT(stat(path3.path().c_str(), &before3), SyscallSucceeds());
+  EXPECT_GT(before1.st_size, 0);
+  EXPECT_GT(before2.st_size, 0);
+  EXPECT_GT(before3.st_size, 0);
+
+  const auto ts_gt = [](const struct timespec& a, const struct timespec& b) {
+    return a.tv_sec > b.tv_sec ||
+           (a.tv_sec == b.tv_sec && a.tv_nsec > b.tv_nsec);
+  };
+
+  absl::SleepFor(absl::Milliseconds(10));
+
+  // Test 1: open(O_TRUNC) on non-zero-length file.
+  const FileDescriptor fd1 =
+      ASSERT_NO_ERRNO_AND_VALUE(Open(path1.path(), O_WRONLY | O_TRUNC));
+  struct stat after1 = {};
+  ASSERT_THAT(fstat(fd1.get(), &after1), SyscallSucceeds());
+  EXPECT_EQ(after1.st_size, 0);
+  EXPECT_TRUE(ts_gt(after1.st_mtim, before1.st_mtim));
+  EXPECT_TRUE(ts_gt(after1.st_ctim, before1.st_ctim));
+
+  // Test 2: ftruncate(2) to smaller size.
+  const FileDescriptor fd2 =
+      ASSERT_NO_ERRNO_AND_VALUE(Open(path2.path(), O_WRONLY));
+  ASSERT_THAT(ftruncate(fd2.get(), 5), SyscallSucceeds());
+  struct stat after2 = {};
+  ASSERT_THAT(fstat(fd2.get(), &after2), SyscallSucceeds());
+  EXPECT_EQ(after2.st_size, 5);
+  EXPECT_TRUE(ts_gt(after2.st_mtim, before2.st_mtim));
+  EXPECT_TRUE(ts_gt(after2.st_ctim, before2.st_ctim));
+
+  // Test 3: truncate(2) to 0.
+  ASSERT_THAT(truncate(path3.path().c_str(), 0), SyscallSucceeds());
+  struct stat after3 = {};
+  ASSERT_THAT(stat(path3.path().c_str(), &after3), SyscallSucceeds());
+  EXPECT_EQ(after3.st_size, 0);
+  EXPECT_TRUE(ts_gt(after3.st_mtim, before3.st_mtim));
+  EXPECT_TRUE(ts_gt(after3.st_ctim, before3.st_ctim));
+}
+
 }  // namespace
 
 }  // namespace testing