Support VFS2 save/restore.

Inode number consistency checks are now skipped in save/restore tests for
reasons described in greatest detail in StatTest.StateDoesntChangeAfterRename.
They pass in VFS1 due to the bug described in new test case
SimpleStatTest.DifferentFilesHaveDifferentDeviceInodeNumberPairs.

Fixes #1663

PiperOrigin-RevId: 338776148

12 files changed, 151 insertions, 97 deletions

diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD
index 572f39a5d..42fa18683 100644
--- a/test/syscalls/linux/BUILD
+++ b/test/syscalls/linux/BUILD
@@ -1285,6 +1285,7 @@ cc_binary(
         "//test/util:mount_util",
         "//test/util:multiprocess_util",
         "//test/util:posix_error",
+        "//test/util:save_util",
         "//test/util:temp_path",
         "//test/util:test_main",
         "//test/util:test_util",
@@ -3441,6 +3442,7 @@ cc_binary(
         "@com_google_absl//absl/strings",
         gtest,
         "//test/util:posix_error",
+        "//test/util:save_util",
         "//test/util:temp_path",
         "//test/util:test_main",
         "//test/util:test_util",
diff --git a/test/syscalls/linux/mknod.cc b/test/syscalls/linux/mknod.cc
index b96907b30..1635c6d0c 100644
--- a/test/syscalls/linux/mknod.cc
+++ b/test/syscalls/linux/mknod.cc
@@ -125,6 +125,16 @@ TEST(MknodTest, Socket) {
   ASSERT_THAT(unlink(filename.c_str()), SyscallSucceeds());
 }
 
+PosixErrorOr<FileDescriptor> OpenRetryEINTR(std::string const& path, int flags,
+                                            mode_t mode = 0) {
+  while (true) {
+    auto maybe_fd = Open(path, flags, mode);
+    if (maybe_fd.ok() || maybe_fd.error().errno_value() != EINTR) {
+      return maybe_fd;
+    }
+  }
+}
+
 TEST(MknodTest, Fifo) {
   const std::string fifo = NewTempAbsPath();
   ASSERT_THAT(mknod(fifo.c_str(), S_IFIFO | S_IRUSR | S_IWUSR, 0),
@@ -139,14 +149,16 @@ TEST(MknodTest, Fifo) {
 
   // Read-end of the pipe.
   ScopedThread t([&fifo, &buf, &msg]() {
-    FileDescriptor fd = ASSERT_NO_ERRNO_AND_VALUE(Open(fifo.c_str(), O_RDONLY));
+    FileDescriptor fd =
+        ASSERT_NO_ERRNO_AND_VALUE(OpenRetryEINTR(fifo.c_str(), O_RDONLY));
     EXPECT_THAT(ReadFd(fd.get(), buf.data(), buf.size()),
                 SyscallSucceedsWithValue(msg.length()));
     EXPECT_EQ(msg, std::string(buf.data()));
   });
 
   // Write-end of the pipe.
-  FileDescriptor wfd = ASSERT_NO_ERRNO_AND_VALUE(Open(fifo.c_str(), O_WRONLY));
+  FileDescriptor wfd =
+      ASSERT_NO_ERRNO_AND_VALUE(OpenRetryEINTR(fifo.c_str(), O_WRONLY));
   EXPECT_THAT(WriteFd(wfd.get(), msg.c_str(), msg.length()),
               SyscallSucceedsWithValue(msg.length()));
 }
@@ -164,15 +176,16 @@ TEST(MknodTest, FifoOtrunc) {
   std::vector<char> buf(512);
   // Read-end of the pipe.
   ScopedThread t([&fifo, &buf, &msg]() {
-    FileDescriptor fd = ASSERT_NO_ERRNO_AND_VALUE(Open(fifo.c_str(), O_RDONLY));
+    FileDescriptor fd =
+        ASSERT_NO_ERRNO_AND_VALUE(OpenRetryEINTR(fifo.c_str(), O_RDONLY));
     EXPECT_THAT(ReadFd(fd.get(), buf.data(), buf.size()),
                 SyscallSucceedsWithValue(msg.length()));
     EXPECT_EQ(msg, std::string(buf.data()));
   });
 
   // Write-end of the pipe.
-  FileDescriptor wfd =
-      ASSERT_NO_ERRNO_AND_VALUE(Open(fifo.c_str(), O_WRONLY | O_TRUNC));
+  FileDescriptor wfd = ASSERT_NO_ERRNO_AND_VALUE(
+      OpenRetryEINTR(fifo.c_str(), O_WRONLY | O_TRUNC));
   EXPECT_THAT(WriteFd(wfd.get(), msg.c_str(), msg.length()),
               SyscallSucceedsWithValue(msg.length()));
 }
@@ -192,14 +205,15 @@ TEST(MknodTest, FifoTruncNoOp) {
   std::vector<char> buf(512);
   // Read-end of the pipe.
   ScopedThread t([&fifo, &buf, &msg]() {
-    FileDescriptor fd = ASSERT_NO_ERRNO_AND_VALUE(Open(fifo.c_str(), O_RDONLY));
+    FileDescriptor fd =
+        ASSERT_NO_ERRNO_AND_VALUE(OpenRetryEINTR(fifo.c_str(), O_RDONLY));
     EXPECT_THAT(ReadFd(fd.get(), buf.data(), buf.size()),
                 SyscallSucceedsWithValue(msg.length()));
     EXPECT_EQ(msg, std::string(buf.data()));
   });
 
-  FileDescriptor wfd =
-      ASSERT_NO_ERRNO_AND_VALUE(Open(fifo.c_str(), O_WRONLY | O_TRUNC));
+  FileDescriptor wfd = ASSERT_NO_ERRNO_AND_VALUE(
+      OpenRetryEINTR(fifo.c_str(), O_WRONLY | O_TRUNC));
   EXPECT_THAT(ftruncate(wfd.get(), 0), SyscallFailsWithErrno(EINVAL));
   EXPECT_THAT(WriteFd(wfd.get(), msg.c_str(), msg.length()),
               SyscallSucceedsWithValue(msg.length()));
diff --git a/test/syscalls/linux/mount.cc b/test/syscalls/linux/mount.cc
index 3aab25b23..d65b7d031 100644
--- a/test/syscalls/linux/mount.cc
+++ b/test/syscalls/linux/mount.cc
@@ -34,6 +34,7 @@
 #include "test/util/mount_util.h"
 #include "test/util/multiprocess_util.h"
 #include "test/util/posix_error.h"
+#include "test/util/save_util.h"
 #include "test/util/temp_path.h"
 #include "test/util/test_util.h"
 #include "test/util/thread_util.h"
@@ -131,7 +132,9 @@ TEST(MountTest, UmountDetach) {
       ASSERT_NO_ERRNO_AND_VALUE(Mount("", dir.path(), "tmpfs", 0, "mode=0700",
                                       /* umountflags= */ MNT_DETACH));
   const struct stat after = ASSERT_NO_ERRNO_AND_VALUE(Stat(dir.path()));
-  EXPECT_NE(before.st_ino, after.st_ino);
+  EXPECT_FALSE(before.st_dev == after.st_dev && before.st_ino == after.st_ino)
+      << "mount point has device number " << before.st_dev
+      << " and inode number " << before.st_ino << " before and after mount";
 
   // Create files in the new mount.
   constexpr char kContents[] = "no no no";
@@ -147,12 +150,14 @@ TEST(MountTest, UmountDetach) {
   // Unmount the tmpfs.
   mount.Release()();
 
-  // Only check for inode number equality if the directory is not in overlayfs.
-  // If xino option is not enabled and if all overlayfs layers do not belong to
-  // the same filesystem then "the value of st_ino for directory objects may not
-  // be persistent and could change even while the overlay filesystem is
-  // mounted."  -- Documentation/filesystems/overlayfs.txt
-  if (!ASSERT_NO_ERRNO_AND_VALUE(IsOverlayfs(dir.path()))) {
+  // Inode numbers for gofer-accessed files may change across save/restore.
+  //
+  // For overlayfs, if xino option is not enabled and if all overlayfs layers do
+  // not belong to the same filesystem then "the value of st_ino for directory
+  // objects may not be persistent and could change even while the overlay
+  // filesystem is mounted."  -- Documentation/filesystems/overlayfs.txt
+  if (!IsRunningWithSaveRestore() &&
+      !ASSERT_NO_ERRNO_AND_VALUE(IsOverlayfs(dir.path()))) {
     const struct stat after2 = ASSERT_NO_ERRNO_AND_VALUE(Stat(dir.path()));
     EXPECT_EQ(before.st_ino, after2.st_ino);
   }
@@ -214,18 +219,23 @@ TEST(MountTest, MountTmpfs) {
 
     const struct stat s = ASSERT_NO_ERRNO_AND_VALUE(Stat(dir.path()));
     EXPECT_EQ(s.st_mode, S_IFDIR | 0700);
-    EXPECT_NE(s.st_ino, before.st_ino);
+    EXPECT_FALSE(before.st_dev == s.st_dev && before.st_ino == s.st_ino)
+        << "mount point has device number " << before.st_dev
+        << " and inode number " << before.st_ino << " before and after mount";
 
     EXPECT_NO_ERRNO(Open(JoinPath(dir.path(), "foo"), O_CREAT | O_RDWR, 0777));
   }
 
   // Now that dir is unmounted again, we should have the old inode back.
-  // Only check for inode number equality if the directory is not in overlayfs.
-  // If xino option is not enabled and if all overlayfs layers do not belong to
-  // the same filesystem then "the value of st_ino for directory objects may not
-  // be persistent and could change even while the overlay filesystem is
-  // mounted."  -- Documentation/filesystems/overlayfs.txt
-  if (!ASSERT_NO_ERRNO_AND_VALUE(IsOverlayfs(dir.path()))) {
+  //
+  // Inode numbers for gofer-accessed files may change across save/restore.
+  //
+  // For overlayfs, if xino option is not enabled and if all overlayfs layers do
+  // not belong to the same filesystem then "the value of st_ino for directory
+  // objects may not be persistent and could change even while the overlay
+  // filesystem is mounted."  -- Documentation/filesystems/overlayfs.txt
+  if (!IsRunningWithSaveRestore() &&
+      !ASSERT_NO_ERRNO_AND_VALUE(IsOverlayfs(dir.path()))) {
     const struct stat after = ASSERT_NO_ERRNO_AND_VALUE(Stat(dir.path()));
     EXPECT_EQ(before.st_ino, after.st_ino);
   }
diff --git a/test/syscalls/linux/proc_pid_smaps.cc b/test/syscalls/linux/proc_pid_smaps.cc
index 9fb1b3a2c..738923822 100644
--- a/test/syscalls/linux/proc_pid_smaps.cc
+++ b/test/syscalls/linux/proc_pid_smaps.cc
@@ -191,7 +191,7 @@ PosixErrorOr<std::vector<ProcPidSmapsEntry>> ParseProcPidSmaps(
     // amount of whitespace).
     if (!entry) {
       std::cerr << "smaps line not considered a maps line: "
-                << maybe_maps_entry.error_message() << std::endl;
+                << maybe_maps_entry.error().message() << std::endl;
       return PosixError(
           EINVAL,
           absl::StrCat("smaps field line without preceding maps line: ", l));
diff --git a/test/syscalls/linux/stat.cc b/test/syscalls/linux/stat.cc
index 92260b1e1..6e7142a42 100644
--- a/test/syscalls/linux/stat.cc
+++ b/test/syscalls/linux/stat.cc
@@ -31,6 +31,7 @@
 #include "test/util/cleanup.h"
 #include "test/util/file_descriptor.h"
 #include "test/util/fs_util.h"
+#include "test/util/save_util.h"
 #include "test/util/temp_path.h"
 #include "test/util/test_util.h"
 
@@ -328,7 +329,10 @@ TEST_F(StatTest, LeadingDoubleSlash) {
   ASSERT_THAT(lstat(double_slash_path.c_str(), &double_slash_st),
               SyscallSucceeds());
   EXPECT_EQ(st.st_dev, double_slash_st.st_dev);
-  EXPECT_EQ(st.st_ino, double_slash_st.st_ino);
+  // Inode numbers for gofer-accessed files may change across save/restore.
+  if (!IsRunningWithSaveRestore()) {
+    EXPECT_EQ(st.st_ino, double_slash_st.st_ino);
+  }
 }
 
 // Test that a rename doesn't change the underlying file.
@@ -346,8 +350,14 @@ TEST_F(StatTest, StatDoesntChangeAfterRename) {
 
   EXPECT_EQ(st_old.st_nlink, st_new.st_nlink);
   EXPECT_EQ(st_old.st_dev, st_new.st_dev);
+  // Inode numbers for gofer-accessed files on which no reference is held may
+  // change across save/restore because the information that the gofer client
+  // uses to track file identity (9P QID path) is inconsistent between gofer
+  // processes, which are restarted across save/restore.
+  //
   // Overlay filesystems may synthesize directory inode numbers on the fly.
-  if (!ASSERT_NO_ERRNO_AND_VALUE(IsOverlayfs(GetAbsoluteTestTmpdir()))) {
+  if (!IsRunningWithSaveRestore() &&
+      !ASSERT_NO_ERRNO_AND_VALUE(IsOverlayfs(GetAbsoluteTestTmpdir()))) {
     EXPECT_EQ(st_old.st_ino, st_new.st_ino);
   }
   EXPECT_EQ(st_old.st_mode, st_new.st_mode);
@@ -541,6 +551,26 @@ TEST_F(StatTest, LstatELOOPPath) {
   ASSERT_THAT(lstat(path.c_str(), &s), SyscallFailsWithErrno(ELOOP));
 }
 
+TEST(SimpleStatTest, DifferentFilesHaveDifferentDeviceInodeNumberPairs) {
+  // TODO(gvisor.dev/issue/1624): This test case fails in VFS1 save/restore
+  // tests because VFS1 gofer inode number assignment restarts after
+  // save/restore, such that the inodes for file1 and file2 (which are
+  // unreferenced and therefore not retained in sentry checkpoints before the
+  // calls to lstat()) are assigned the same inode number.
+  SKIP_IF(IsRunningWithVFS1() && IsRunningWithSaveRestore());
+
+  TempPath file1 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
+  TempPath file2 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
+
+  MaybeSave();
+  struct stat st1 = ASSERT_NO_ERRNO_AND_VALUE(Lstat(file1.path()));
+  MaybeSave();
+  struct stat st2 = ASSERT_NO_ERRNO_AND_VALUE(Lstat(file2.path()));
+  EXPECT_FALSE(st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino)
+      << "both files have device number " << st1.st_dev << " and inode number "
+      << st1.st_ino;
+}
+
 // Ensure that inode allocation for anonymous devices work correctly across
 // save/restore. In particular, inode numbers should be unique across S/R.
 TEST(SimpleStatTest, AnonDeviceAllocatesUniqueInodesAcrossSaveRestore) {
diff --git a/test/util/BUILD b/test/util/BUILD
index 26c2b6a2f..1b028a477 100644
--- a/test/util/BUILD
+++ b/test/util/BUILD
@@ -155,6 +155,10 @@ cc_library(
     ],
     hdrs = ["save_util.h"],
     defines = select_system(),
+    deps = [
+        ":logging",
+        "@com_google_absl//absl/types:optional",
+    ],
 )
 
 cc_library(
diff --git a/test/util/posix_error.cc b/test/util/posix_error.cc
index cebf7e0ac..deed0c05b 100644
--- a/test/util/posix_error.cc
+++ b/test/util/posix_error.cc
@@ -87,7 +87,7 @@ bool PosixErrorIsMatcherCommonImpl::MatchAndExplain(
     return false;
   }
 
-  if (!message_matcher_.Matches(error.error_message())) {
+  if (!message_matcher_.Matches(error.message())) {
     return false;
   }
 
diff --git a/test/util/posix_error.h b/test/util/posix_error.h
index ad666bce0..b634a7f78 100644
--- a/test/util/posix_error.h
+++ b/test/util/posix_error.h
@@ -26,11 +26,6 @@
 namespace gvisor {
 namespace testing {
 
-class PosixErrorIsMatcherCommonImpl;
-
-template <typename T>
-class PosixErrorOr;
-
 class ABSL_MUST_USE_RESULT PosixError {
  public:
   PosixError() {}
@@ -49,7 +44,8 @@ class ABSL_MUST_USE_RESULT PosixError {
   // PosixErrorOr.
   const PosixError& error() const { return *this; }
 
-  std::string error_message() const { return msg_; }
+  int errno_value() const { return errno_; }
+  std::string message() const { return msg_; }
 
   // ToString produces a full string representation of this posix error
   // including the printable representation of the errno and the error message.
@@ -61,14 +57,8 @@ class ABSL_MUST_USE_RESULT PosixError {
   void IgnoreError() const {}
 
  private:
-  int errno_value() const { return errno_; }
   int errno_ = 0;
   std::string msg_;
-
-  friend class PosixErrorIsMatcherCommonImpl;
-
-  template <typename T>
-  friend class PosixErrorOr;
 };
 
 template <typename T>
@@ -94,15 +84,12 @@ class ABSL_MUST_USE_RESULT PosixErrorOr {
   template <typename U>
   PosixErrorOr& operator=(PosixErrorOr<U> other);
 
-  // Return a reference to the error or NoError().
-  PosixError error() const;
-
-  // Returns this->error().error_message();
-  std::string error_message() const;
-
   // Returns true if this PosixErrorOr contains some T.
   bool ok() const;
 
+  // Return a copy of the contained PosixError or NoError().
+  PosixError error() const;
+
   // Returns a reference to our current value, or CHECK-fails if !this->ok().
   const T& ValueOrDie() const&;
   T& ValueOrDie() &;
@@ -115,7 +102,6 @@ class ABSL_MUST_USE_RESULT PosixErrorOr {
   void IgnoreError() const {}
 
  private:
-  int errno_value() const;
   absl::variant<T, PosixError> value_;
 
   friend class PosixErrorIsMatcherCommonImpl;
@@ -171,16 +157,6 @@ PosixError PosixErrorOr<T>::error() const {
 }
 
 template <typename T>
-int PosixErrorOr<T>::errno_value() const {
-  return error().errno_value();
-}
-
-template <typename T>
-std::string PosixErrorOr<T>::error_message() const {
-  return error().error_message();
-}
-
-template <typename T>
 bool PosixErrorOr<T>::ok() const {
   return absl::holds_alternative<T>(value_);
 }
diff --git a/test/util/save_util.cc b/test/util/save_util.cc
index 384d626f0..59d47e06e 100644
--- a/test/util/save_util.cc
+++ b/test/util/save_util.cc
@@ -21,35 +21,47 @@
 #include <atomic>
 #include <cerrno>
 
-#define GVISOR_COOPERATIVE_SAVE_TEST "GVISOR_COOPERATIVE_SAVE_TEST"
+#include "absl/types/optional.h"
 
 namespace gvisor {
 namespace testing {
 namespace {
 
-enum class CooperativeSaveMode {
-  kUnknown = 0,  // cooperative_save_mode is statically-initialized to 0
-  kAvailable,
-  kNotAvailable,
-};
-
-std::atomic<CooperativeSaveMode> cooperative_save_mode;
-
-bool CooperativeSaveEnabled() {
-  auto mode = cooperative_save_mode.load();
-  if (mode == CooperativeSaveMode::kUnknown) {
-    mode = (getenv(GVISOR_COOPERATIVE_SAVE_TEST) != nullptr)
-               ? CooperativeSaveMode::kAvailable
-               : CooperativeSaveMode::kNotAvailable;
-    cooperative_save_mode.store(mode);
+std::atomic<absl::optional<bool>> cooperative_save_present;
+std::atomic<absl::optional<bool>> random_save_present;
+
+bool CooperativeSavePresent() {
+  auto present = cooperative_save_present.load();
+  if (!present.has_value()) {
+    present = getenv("GVISOR_COOPERATIVE_SAVE_TEST") != nullptr;
+    cooperative_save_present.store(present);
+  }
+  return present.value();
+}
+
+bool RandomSavePresent() {
+  auto present = random_save_present.load();
+  if (!present.has_value()) {
+    present = getenv("GVISOR_RANDOM_SAVE_TEST") != nullptr;
+    random_save_present.store(present);
   }
-  return mode == CooperativeSaveMode::kAvailable;
+  return present.value();
 }
 
 std::atomic<int> save_disable;
 
 }  // namespace
 
+bool IsRunningWithSaveRestore() {
+  return CooperativeSavePresent() || RandomSavePresent();
+}
+
+void MaybeSave() {
+  if (CooperativeSavePresent() && save_disable.load() == 0) {
+    internal::DoCooperativeSave();
+  }
+}
+
 DisableSave::DisableSave() { save_disable++; }
 
 DisableSave::~DisableSave() { reset(); }
@@ -61,11 +73,5 @@ void DisableSave::reset() {
   }
 }
 
-namespace internal {
-bool ShouldSave() {
-  return CooperativeSaveEnabled() && (save_disable.load() == 0);
-}
-}  // namespace internal
-
 }  // namespace testing
 }  // namespace gvisor
diff --git a/test/util/save_util.h b/test/util/save_util.h
index bddad6120..e7218ae88 100644
--- a/test/util/save_util.h
+++ b/test/util/save_util.h
@@ -17,9 +17,17 @@
 
 namespace gvisor {
 namespace testing {
-// Disable save prevents saving while the given function executes.
+
+// Returns true if the environment in which the calling process is executing
+// allows the test to be checkpointed and restored during execution.
+bool IsRunningWithSaveRestore();
+
+// May perform a co-operative save cycle.
 //
-// This lasts the duration of the object, unless reset is called.
+// errno is guaranteed to be preserved.
+void MaybeSave();
+
+// Causes MaybeSave to become a no-op until destroyed or reset.
 class DisableSave {
  public:
   DisableSave();
@@ -37,13 +45,13 @@ class DisableSave {
   bool reset_ = false;
 };
 
-// May perform a co-operative save cycle.
+namespace internal {
+
+// Causes a co-operative save cycle to occur.
 //
 // errno is guaranteed to be preserved.
-void MaybeSave();
+void DoCooperativeSave();
 
-namespace internal {
-bool ShouldSave();
 }  // namespace internal
 
 }  // namespace testing
diff --git a/test/util/save_util_linux.cc b/test/util/save_util_linux.cc
index fbac94912..57431b3ea 100644
--- a/test/util/save_util_linux.cc
+++ b/test/util/save_util_linux.cc
@@ -30,19 +30,19 @@
 
 namespace gvisor {
 namespace testing {
-
-void MaybeSave() {
-  if (internal::ShouldSave()) {
-    int orig_errno = errno;
-    // We use it to trigger saving the sentry state
-    // when this syscall is called.
-    // Notice: this needs to be a valid syscall
-    // that is not used in any of the syscall tests.
-    syscall(SYS_TRIGGER_SAVE, nullptr, 0);
-    errno = orig_errno;
-  }
+namespace internal {
+
+void DoCooperativeSave() {
+  int orig_errno = errno;
+  // We use it to trigger saving the sentry state
+  // when this syscall is called.
+  // Notice: this needs to be a valid syscall
+  // that is not used in any of the syscall tests.
+  syscall(SYS_TRIGGER_SAVE, nullptr, 0);
+  errno = orig_errno;
 }
 
+}  // namespace internal
 }  // namespace testing
 }  // namespace gvisor
 
diff --git a/test/util/save_util_other.cc b/test/util/save_util_other.cc
index 931af2c29..7749ded76 100644
--- a/test/util/save_util_other.cc
+++ b/test/util/save_util_other.cc
@@ -14,13 +14,17 @@
 
 #ifndef __linux__
 
+#include "test/util/logging.h"
+
 namespace gvisor {
 namespace testing {
+namespace internal {
 
-void MaybeSave() {
-  // Saving is never available in a non-linux environment.
+void DoCooperativeSave() {
+  TEST_CHECK_MSG(false, "DoCooperativeSave not implemented");
 }
 
+}  // namespace internal
 }  // namespace testing
 }  // namespace gvisor