Fix gofer rename ctime and cleanup stat_times test

There is a lot of redundancy that we can simplify in the stat_times
test. This will make it easier to add new tests. However, the
simplification reveals that cached uattrs on goferfs don't properly
update ctime on rename.

PiperOrigin-RevId: 248773425
Change-Id: I52662728e1e9920981555881f9a85f9ce04041cf

12 files changed, 213 insertions, 150 deletions

diff --git a/pkg/sentry/fs/fsutil/inode.go b/pkg/sentry/fs/fsutil/inode.go
index 151be1d0d..5e1bfeb58 100644
--- a/pkg/sentry/fs/fsutil/inode.go
+++ b/pkg/sentry/fs/fsutil/inode.go
@@ -341,7 +341,7 @@ func (InodeNotDirectory) RemoveDirectory(context.Context, *fs.Inode, string) err
 }
 
 // Rename implements fs.FileOperations.Rename.
-func (InodeNotDirectory) Rename(context.Context, *fs.Inode, string, *fs.Inode, string, bool) error {
+func (InodeNotDirectory) Rename(context.Context, *fs.Inode, *fs.Inode, string, *fs.Inode, string, bool) error {
 	return syserror.EINVAL
 }
 
@@ -381,7 +381,7 @@ func (InodeNoopTruncate) Truncate(context.Context, *fs.Inode, int64) error {
 type InodeNotRenameable struct{}
 
 // Rename implements fs.InodeOperations.Rename.
-func (InodeNotRenameable) Rename(context.Context, *fs.Inode, string, *fs.Inode, string, bool) error {
+func (InodeNotRenameable) Rename(context.Context, *fs.Inode, *fs.Inode, string, *fs.Inode, string, bool) error {
 	return syserror.EINVAL
 }
 
diff --git a/pkg/sentry/fs/fsutil/inode_cached.go b/pkg/sentry/fs/fsutil/inode_cached.go
index 03cad37f3..bc0b8c744 100644
--- a/pkg/sentry/fs/fsutil/inode_cached.go
+++ b/pkg/sentry/fs/fsutil/inode_cached.go
@@ -451,6 +451,14 @@ func (c *CachingInodeOperations) touchModificationTimeLocked(now time.Time) {
 	c.dirtyAttr.StatusChangeTime = true
 }
 
+// TouchStatusChangeTime updates the cached status change time in-place to the
+// current time.
+func (c *CachingInodeOperations) TouchStatusChangeTime(ctx context.Context) {
+	c.attrMu.Lock()
+	c.touchStatusChangeTimeLocked(ktime.NowFromContext(ctx))
+	c.attrMu.Unlock()
+}
+
 // touchStatusChangeTimeLocked updates the cached status change time
 // in-place to the current time.
 //
diff --git a/pkg/sentry/fs/gofer/path.go b/pkg/sentry/fs/gofer/path.go
index 875de8b5f..babfa4560 100644
--- a/pkg/sentry/fs/gofer/path.go
+++ b/pkg/sentry/fs/gofer/path.go
@@ -344,7 +344,7 @@ func (i *inodeOperations) RemoveDirectory(ctx context.Context, dir *fs.Inode, na
 }
 
 // Rename renames this node.
-func (i *inodeOperations) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
+func (i *inodeOperations) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
 	if len(newName) > maxFilenameLen {
 		return syserror.ENAMETOOLONG
 	}
@@ -389,6 +389,11 @@ func (i *inodeOperations) Rename(ctx context.Context, oldParent *fs.Inode, oldNa
 			newParentInodeOperations.markDirectoryDirty()
 		}
 	}
+
+	// Rename always updates ctime.
+	if i.session().cachePolicy.cacheUAttrs(inode) {
+		i.cachingInodeOps.TouchStatusChangeTime(ctx)
+	}
 	return nil
 }
 
diff --git a/pkg/sentry/fs/host/inode.go b/pkg/sentry/fs/host/inode.go
index d36ac9a87..ebf2154bc 100644
--- a/pkg/sentry/fs/host/inode.go
+++ b/pkg/sentry/fs/host/inode.go
@@ -301,7 +301,7 @@ func (i *inodeOperations) RemoveDirectory(ctx context.Context, dir *fs.Inode, na
 }
 
 // Rename implements fs.InodeOperations.Rename.
-func (i *inodeOperations) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
+func (i *inodeOperations) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
 	op, ok := oldParent.InodeOperations.(*inodeOperations)
 	if !ok {
 		return syscall.EXDEV
diff --git a/pkg/sentry/fs/inode.go b/pkg/sentry/fs/inode.go
index 22f316daf..aef1a1cb9 100644
--- a/pkg/sentry/fs/inode.go
+++ b/pkg/sentry/fs/inode.go
@@ -216,7 +216,7 @@ func (i *Inode) Rename(ctx context.Context, oldParent *Dirent, renamed *Dirent,
 	if i.overlay != nil {
 		return overlayRename(ctx, i.overlay, oldParent, renamed, newParent, newName, replacement)
 	}
-	return i.InodeOperations.Rename(ctx, oldParent.Inode, renamed.name, newParent.Inode, newName, replacement)
+	return i.InodeOperations.Rename(ctx, renamed.Inode, oldParent.Inode, renamed.name, newParent.Inode, newName, replacement)
 }
 
 // Bind calls i.InodeOperations.Bind with i as the directory.
diff --git a/pkg/sentry/fs/inode_operations.go b/pkg/sentry/fs/inode_operations.go
index abafe4791..3211f1817 100644
--- a/pkg/sentry/fs/inode_operations.go
+++ b/pkg/sentry/fs/inode_operations.go
@@ -131,14 +131,15 @@ type InodeOperations interface {
 	RemoveDirectory(ctx context.Context, dir *Inode, name string) error
 
 	// Rename atomically renames oldName under oldParent to newName under
-	// newParent where oldParent and newParent are directories.
+	// newParent where oldParent and newParent are directories. inode is
+	// the Inode of this InodeOperations.
 	//
 	// If replacement is true, then newName already exists and this call
 	// will replace it with oldName.
 	//
 	// Implementations are responsible for rejecting renames that replace
 	// non-empty directories.
-	Rename(ctx context.Context, oldParent *Inode, oldName string, newParent *Inode, newName string, replacement bool) error
+	Rename(ctx context.Context, inode *Inode, oldParent *Inode, oldName string, newParent *Inode, newName string, replacement bool) error
 
 	// Bind binds a new socket under dir at the given name.
 	//
diff --git a/pkg/sentry/fs/inode_overlay.go b/pkg/sentry/fs/inode_overlay.go
index ead487097..ea574224f 100644
--- a/pkg/sentry/fs/inode_overlay.go
+++ b/pkg/sentry/fs/inode_overlay.go
@@ -389,7 +389,7 @@ func overlayRename(ctx context.Context, o *overlayEntry, oldParent *Dirent, rena
 		return err
 	}
 	oldName := renamed.name
-	if err := o.upper.InodeOperations.Rename(ctx, oldParent.Inode.overlay.upper, oldName, newParent.Inode.overlay.upper, newName, replacement); err != nil {
+	if err := o.upper.InodeOperations.Rename(ctx, renamed.Inode.overlay.upper, oldParent.Inode.overlay.upper, oldName, newParent.Inode.overlay.upper, newName, replacement); err != nil {
 		return err
 	}
 	if renamed.Inode.overlay.lowerExists {
diff --git a/pkg/sentry/fs/mock.go b/pkg/sentry/fs/mock.go
index a71144b2c..064943c5b 100644
--- a/pkg/sentry/fs/mock.go
+++ b/pkg/sentry/fs/mock.go
@@ -132,7 +132,7 @@ func (n *MockInodeOperations) CreateDirectory(context.Context, *Inode, string, F
 }
 
 // Rename implements fs.InodeOperations.Rename.
-func (n *MockInodeOperations) Rename(ctx context.Context, oldParent *Inode, oldName string, newParent *Inode, newName string, replacement bool) error {
+func (n *MockInodeOperations) Rename(ctx context.Context, inode *Inode, oldParent *Inode, oldName string, newParent *Inode, newName string, replacement bool) error {
 	n.renameCalled = true
 	return nil
 }
diff --git a/pkg/sentry/fs/ramfs/dir.go b/pkg/sentry/fs/ramfs/dir.go
index eb98b59cc..c97ad26f5 100644
--- a/pkg/sentry/fs/ramfs/dir.go
+++ b/pkg/sentry/fs/ramfs/dir.go
@@ -401,7 +401,7 @@ func (d *Dir) GetFile(ctx context.Context, dirent *fs.Dirent, flags fs.FileFlags
 }
 
 // Rename implements fs.InodeOperations.Rename.
-func (*Dir) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
+func (*Dir) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
 	return Rename(ctx, oldParent.InodeOperations, oldName, newParent.InodeOperations, newName, replacement)
 }
 
diff --git a/pkg/sentry/fs/tmpfs/inode_file.go b/pkg/sentry/fs/tmpfs/inode_file.go
index c90062a22..3fe659543 100644
--- a/pkg/sentry/fs/tmpfs/inode_file.go
+++ b/pkg/sentry/fs/tmpfs/inode_file.go
@@ -149,7 +149,7 @@ func (f *fileInodeOperations) Mappable(*fs.Inode) memmap.Mappable {
 }
 
 // Rename implements fs.InodeOperations.Rename.
-func (*fileInodeOperations) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
+func (*fileInodeOperations) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
 	return rename(ctx, oldParent, oldName, newParent, newName, replacement)
 }
 
diff --git a/pkg/sentry/fs/tmpfs/tmpfs.go b/pkg/sentry/fs/tmpfs/tmpfs.go
index 6ad5c5adb..263d10cfe 100644
--- a/pkg/sentry/fs/tmpfs/tmpfs.go
+++ b/pkg/sentry/fs/tmpfs/tmpfs.go
@@ -238,7 +238,7 @@ func (d *Dir) newCreateOps() *ramfs.CreateOps {
 }
 
 // Rename implements fs.InodeOperations.Rename.
-func (d *Dir) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
+func (d *Dir) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
 	return rename(ctx, oldParent, oldName, newParent, newName, replacement)
 }
 
@@ -271,7 +271,7 @@ func NewSymlink(ctx context.Context, target string, owner fs.FileOwner, msrc *fs
 }
 
 // Rename implements fs.InodeOperations.Rename.
-func (s *Symlink) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
+func (s *Symlink) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
 	return rename(ctx, oldParent, oldName, newParent, newName, replacement)
 }
 
@@ -301,7 +301,7 @@ func NewSocket(ctx context.Context, socket transport.BoundEndpoint, owner fs.Fil
 }
 
 // Rename implements fs.InodeOperations.Rename.
-func (s *Socket) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
+func (s *Socket) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
 	return rename(ctx, oldParent, oldName, newParent, newName, replacement)
 }
 
@@ -338,7 +338,7 @@ func NewFifo(ctx context.Context, owner fs.FileOwner, perms fs.FilePermissions,
 }
 
 // Rename implements fs.InodeOperations.Rename.
-func (f *Fifo) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
+func (f *Fifo) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error {
 	return rename(ctx, oldParent, oldName, newParent, newName, replacement)
 }
 
diff --git a/test/syscalls/linux/stat_times.cc b/test/syscalls/linux/stat_times.cc
index 9b53739a0..195c87ca5 100644
--- a/test/syscalls/linux/stat_times.cc
+++ b/test/syscalls/linux/stat_times.cc
@@ -33,23 +33,88 @@ namespace {
 using ::testing::IsEmpty;
 using ::testing::Not;
 
-class StatTimesTest : public ::testing::Test {
- protected:
-  std::tuple<absl::Time, absl::Time, absl::Time> GetTime(const TempPath& file) {
-    struct stat statbuf = {};
-    EXPECT_THAT(stat(file.path().c_str(), &statbuf), SyscallSucceeds());
-
-    const auto atime = absl::TimeFromTimespec(statbuf.st_atim);
-    const auto mtime = absl::TimeFromTimespec(statbuf.st_mtim);
-    const auto ctime = absl::TimeFromTimespec(statbuf.st_ctim);
-    return std::make_tuple(atime, mtime, ctime);
-  }
+std::tuple<absl::Time, absl::Time, absl::Time> GetTime(const TempPath& file) {
+  struct stat statbuf = {};
+  EXPECT_THAT(stat(file.path().c_str(), &statbuf), SyscallSucceeds());
+
+  const auto atime = absl::TimeFromTimespec(statbuf.st_atim);
+  const auto mtime = absl::TimeFromTimespec(statbuf.st_mtim);
+  const auto ctime = absl::TimeFromTimespec(statbuf.st_ctim);
+  return std::make_tuple(atime, mtime, ctime);
+}
+
+enum class AtimeEffect {
+  Unchanged,
+  Changed,
+};
+
+enum class MtimeEffect {
+  Unchanged,
+  Changed,
 };
 
-TEST_F(StatTimesTest, FileCreationTimes) {
+enum class CtimeEffect {
+  Unchanged,
+  Changed,
+};
+
+// Tests that fn modifies the atime/mtime/ctime of path as specified.
+void CheckTimes(const TempPath& path, std::function<void()> fn,
+                AtimeEffect atime_effect, MtimeEffect mtime_effect,
+                CtimeEffect ctime_effect) {
+  absl::Time atime, mtime, ctime;
+  std::tie(atime, mtime, ctime) = GetTime(path);
+
+  // FIXME(b/132819225): gVisor filesystem timestamps inconsistently use the
+  // internal or host clock, which may diverge slightly. Allow some slack on
+  // times to account for the difference.
+  //
+  // Here we sleep for 1s so that initial creation of path doesn't fall within
+  // the before slack window.
+  absl::SleepFor(absl::Seconds(1));
+
+  const absl::Time before = absl::Now() - absl::Seconds(1);
+
+  // Perform the op.
+  fn();
+
+  const absl::Time after = absl::Now() + absl::Seconds(1);
+
+  absl::Time atime2, mtime2, ctime2;
+  std::tie(atime2, mtime2, ctime2) = GetTime(path);
+
+  if (atime_effect == AtimeEffect::Changed) {
+    EXPECT_LE(before, atime2);
+    EXPECT_GE(after, atime2);
+    EXPECT_GT(atime2, atime);
+  } else {
+    EXPECT_EQ(atime2, atime);
+  }
+
+  if (mtime_effect == MtimeEffect::Changed) {
+    EXPECT_LE(before, mtime2);
+    EXPECT_GE(after, mtime2);
+    EXPECT_GT(mtime2, mtime);
+  } else {
+    EXPECT_EQ(mtime2, mtime);
+  }
+
+  if (ctime_effect == CtimeEffect::Changed) {
+    EXPECT_LE(before, ctime2);
+    EXPECT_GE(after, ctime2);
+    EXPECT_GT(ctime2, ctime);
+  } else {
+    EXPECT_EQ(ctime2, ctime);
+  }
+}
+
+// File creation time is reflected in atime, mtime, and ctime.
+TEST(StatTimesTest, FileCreation) {
   const DisableSave ds;  // Timing-related test.
 
   // Get a time for when the file is created.
+  //
+  // FIXME(b/132819225): See above.
   const absl::Time before = absl::Now() - absl::Seconds(1);
   const TempPath file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
   const absl::Time after = absl::Now() + absl::Seconds(1);
@@ -65,153 +130,137 @@ TEST_F(StatTimesTest, FileCreationTimes) {
   EXPECT_GE(after, ctime);
 }
 
-TEST_F(StatTimesTest, FileCtimeChanges) {
-  auto file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
-
-  MaybeSave();  // FIXME(b/69865927): ctime is inconsistent.
-
-  absl::Time atime, mtime, ctime;
-  std::tie(atime, mtime, ctime) = GetTime(file);
-
-  absl::SleepFor(absl::Seconds(1));
-
-  // Chmod should only change ctime.
-  EXPECT_THAT(chmod(file.path().c_str(), 0666), SyscallSucceeds());
+// Calling chmod on a file changes ctime.
+TEST(StatTimesTest, FileChmod) {
+  TempPath file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
 
-  absl::Time atime2, mtime2, ctime2;
-  std::tie(atime2, mtime2, ctime2) = GetTime(file);
-  EXPECT_EQ(atime2, atime);
-  EXPECT_EQ(mtime2, mtime);
-  EXPECT_GT(ctime2, ctime);
+  auto fn = [&] {
+    EXPECT_THAT(chmod(file.path().c_str(), 0666), SyscallSucceeds());
+  };
+  CheckTimes(file, fn, AtimeEffect::Unchanged, MtimeEffect::Unchanged,
+             CtimeEffect::Changed);
+}
 
-  absl::SleepFor(absl::Seconds(1));
+// Renaming a file changes ctime.
+TEST(StatTimesTest, FileRename) {
+  TempPath file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
 
-  // Rename should only change ctime.
-  const auto newpath = NewTempAbsPath();
-  EXPECT_THAT(rename(file.path().c_str(), newpath.c_str()), SyscallSucceeds());
-  file.reset(newpath);
+  const std::string newpath = NewTempAbsPath();
 
-  std::tie(atime, mtime, ctime) = GetTime(file);
-  EXPECT_EQ(atime, atime2);
-  EXPECT_EQ(mtime, mtime2);
-  EXPECT_GT(ctime, ctime2);
+  auto fn = [&] {
+    ASSERT_THAT(rename(file.release().c_str(), newpath.c_str()),
+                SyscallSucceeds());
+    file.reset(newpath);
+  };
+  CheckTimes(file, fn, AtimeEffect::Unchanged, MtimeEffect::Unchanged,
+             CtimeEffect::Changed);
+}
 
-  absl::SleepFor(absl::Seconds(1));
+// Renaming a file changes ctime, even with an open FD.
+//
+// NOTE(b/132732387): This is a regression test for fs/gofer failing to update
+// cached ctime.
+TEST(StatTimesTest, FileRenameOpenFD) {
+  TempPath file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
+
+  // Holding an FD shouldn't affect behavior.
+  const FileDescriptor fd =
+      ASSERT_NO_ERRNO_AND_VALUE(Open(file.path(), O_RDONLY));
+
+  const std::string newpath = NewTempAbsPath();
+
+  // FIXME(b/132814682): Restore fails with an uncached gofer and an open FD
+  // across rename.
+  //
+  // N.B. The logic here looks backwards because it isn't possible to
+  // conditionally disable save, only conditionally re-enable it.
+  DisableSave ds;
+  if (!getenv("GVISOR_GOFER_UNCACHED")) {
+    ds.reset();
+  }
 
-  // Utimes should only change ctime and the time that we ask to change (atime
-  // to now in this case).
-  const absl::Time before = absl::Now() - absl::Seconds(1);
-  const struct timespec ts[2] = {{0, UTIME_NOW}, {0, UTIME_OMIT}};
-  ASSERT_THAT(utimensat(AT_FDCWD, file.path().c_str(), ts, 0),
-              SyscallSucceeds());
-  const absl::Time after = absl::Now() + absl::Seconds(1);
+  auto fn = [&] {
+    ASSERT_THAT(rename(file.release().c_str(), newpath.c_str()),
+                SyscallSucceeds());
+    file.reset(newpath);
+  };
+  CheckTimes(file, fn, AtimeEffect::Unchanged, MtimeEffect::Unchanged,
+             CtimeEffect::Changed);
+}
 
-  std::tie(atime2, mtime2, ctime2) = GetTime(file);
-  EXPECT_LE(before, atime2);
-  EXPECT_GE(after, atime2);
-  EXPECT_EQ(mtime2, mtime);
-  EXPECT_GT(ctime2, ctime);
+// Calling utimes on a file changes ctime and the time that we ask to change
+// (atime to now in this case).
+TEST(StatTimesTest, FileUtimes) {
+  TempPath file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
+
+  auto fn = [&] {
+    const struct timespec ts[2] = {{0, UTIME_NOW}, {0, UTIME_OMIT}};
+    ASSERT_THAT(utimensat(AT_FDCWD, file.path().c_str(), ts, 0),
+                SyscallSucceeds());
+  };
+  CheckTimes(file, fn, AtimeEffect::Changed, MtimeEffect::Unchanged,
+             CtimeEffect::Changed);
 }
 
-TEST_F(StatTimesTest, FileMtimeChanges) {
-  const auto file = ASSERT_NO_ERRNO_AND_VALUE(
+// Truncating a file changes mtime and ctime.
+TEST(StatTimesTest, FileTruncate) {
+  const TempPath file = ASSERT_NO_ERRNO_AND_VALUE(
       TempPath::CreateFileWith(GetAbsoluteTestTmpdir(), "yaaass", 0666));
 
-  absl::Time atime, mtime, ctime;
-  std::tie(atime, mtime, ctime) = GetTime(file);
-
-  absl::SleepFor(absl::Seconds(1));
-
-  // Truncate should only change mtime and ctime.
-  EXPECT_THAT(truncate(file.path().c_str(), 0), SyscallSucceeds());
-
-  absl::Time atime2, mtime2, ctime2;
-  std::tie(atime2, mtime2, ctime2) = GetTime(file);
-  EXPECT_EQ(atime2, atime);
-  EXPECT_GT(mtime2, mtime);
-  EXPECT_GT(ctime2, ctime);
+  auto fn = [&] {
+    EXPECT_THAT(truncate(file.path().c_str(), 0), SyscallSucceeds());
+  };
+  CheckTimes(file, fn, AtimeEffect::Unchanged, MtimeEffect::Changed,
+             CtimeEffect::Changed);
+}
 
-  absl::SleepFor(absl::Seconds(1));
+// Writing a file changes mtime and ctime.
+TEST(StatTimesTest, FileWrite) {
+  const TempPath file = ASSERT_NO_ERRNO_AND_VALUE(
+      TempPath::CreateFileWith(GetAbsoluteTestTmpdir(), "yaaass", 0666));
 
-  // Write should only change mtime and ctime.
-  const auto fd = ASSERT_NO_ERRNO_AND_VALUE(Open(file.path(), O_RDWR, 0));
-  const std::string contents = "all the single dollars";
-  EXPECT_THAT(write(fd.get(), contents.data(), contents.size()),
-              SyscallSucceeds());
+  const FileDescriptor fd =
+      ASSERT_NO_ERRNO_AND_VALUE(Open(file.path(), O_RDWR, 0));
 
-  std::tie(atime, mtime, ctime) = GetTime(file);
-  EXPECT_EQ(atime, atime2);
-  EXPECT_GT(mtime, mtime2);
-  EXPECT_GT(ctime, ctime2);
+  auto fn = [&] {
+    const std::string contents = "all the single dollars";
+    EXPECT_THAT(WriteFd(fd.get(), contents.data(), contents.size()),
+                SyscallSucceeds());
+  };
+  CheckTimes(file, fn, AtimeEffect::Unchanged, MtimeEffect::Changed,
+             CtimeEffect::Changed);
 }
 
-TEST_F(StatTimesTest, FileAtimeChanges) {
+// Reading a file changes atime.
+TEST(StatTimesTest, FileRead) {
   const std::string contents = "bills bills bills";
-  const auto file = ASSERT_NO_ERRNO_AND_VALUE(
+  const TempPath file = ASSERT_NO_ERRNO_AND_VALUE(
       TempPath::CreateFileWith(GetAbsoluteTestTmpdir(), contents, 0666));
 
-  MaybeSave();  // FIXME(b/69865927): ctime is inconsistent.
-
-  absl::Time atime, mtime, ctime;
-  std::tie(atime, mtime, ctime) = GetTime(file);
-
-  absl::SleepFor(absl::Seconds(1));
-
-  const auto fd = ASSERT_NO_ERRNO_AND_VALUE(Open(file.path(), O_RDONLY, 0));
+  const FileDescriptor fd =
+      ASSERT_NO_ERRNO_AND_VALUE(Open(file.path(), O_RDONLY, 0));
 
-  // Read should only change atime.
-  char buf[20];
-  const absl::Time before = absl::Now() - absl::Seconds(1);
-  int read_result;
-  ASSERT_THAT(read_result = read(fd.get(), buf, sizeof(buf)),
-              SyscallSucceeds());
-  const absl::Time after = absl::Now() + absl::Seconds(1);
-
-  EXPECT_EQ(std::string(buf, read_result), contents);
-
-  absl::Time atime2, mtime2, ctime2;
-  std::tie(atime2, mtime2, ctime2) = GetTime(file);
-
-  EXPECT_LE(before, atime2);
-  EXPECT_GE(after, atime2);
-  EXPECT_GT(atime2, atime);
-  EXPECT_EQ(mtime2, mtime);
-  EXPECT_EQ(ctime2, ctime);
+  auto fn = [&] {
+    char buf[20];
+    ASSERT_THAT(ReadFd(fd.get(), buf, sizeof(buf)),
+                SyscallSucceedsWithValue(contents.size()));
+  };
+  CheckTimes(file, fn, AtimeEffect::Changed, MtimeEffect::Unchanged,
+             CtimeEffect::Unchanged);
 }
 
-TEST_F(StatTimesTest, DirAtimeChanges) {
-  const auto dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
-  const auto file =
+// Listing files in a directory changes atime.
+TEST(StatTimesTest, DirList) {
+  const TempPath dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+  const TempPath file =
       ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFileIn(dir.path()));
 
-  MaybeSave();  // FIXME(b/69865927): ctime is inconsistent.
-
-  absl::Time atime, mtime, ctime;
-  std::tie(atime, mtime, ctime) = GetTime(dir);
-
-  absl::SleepFor(absl::Seconds(1));
-
-  const absl::Time before = absl::Now() - absl::Seconds(1);
-
-  // NOTE(b/37756234): Keep an fd open. This ensures that the inode backing the
-  // directory won't be destroyed before the final GetTime to avoid writing out
-  // timestamps and causing side effects.
-  const auto fd = ASSERT_NO_ERRNO_AND_VALUE(Open(dir.path(), O_RDONLY, 0));
-
-  // Listing the directory contents should only change atime.
-  auto contents = ASSERT_NO_ERRNO_AND_VALUE(ListDir(dir.path(), false));
-  EXPECT_THAT(contents, Not(IsEmpty()));
-
-  const absl::Time after = absl::Now() + absl::Seconds(1);
-
-  absl::Time atime2, mtime2, ctime2;
-  std::tie(atime2, mtime2, ctime2) = GetTime(dir);
-
-  EXPECT_LE(before, atime2);
-  EXPECT_GE(after, atime2);
-  EXPECT_GT(atime2, atime);
-  EXPECT_EQ(mtime2, mtime);
-  EXPECT_EQ(ctime2, ctime);
+  auto fn = [&] {
+    const auto contents = ASSERT_NO_ERRNO_AND_VALUE(ListDir(dir.path(), false));
+    EXPECT_THAT(contents, Not(IsEmpty()));
+  };
+  CheckTimes(dir, fn, AtimeEffect::Changed, MtimeEffect::Unchanged,
+             CtimeEffect::Unchanged);
 }
 
 }  // namespace