kernfs: invalidate descendants of dentries failing revalidation

Adapted from cl/630063475.

This causes FUSE tests involving submounts to fail, because fuse.inode.Valid()
just returns false (fails revalidation) after the entry time expires, causing
all submounts to be unmounted; change it to perform revalidation instead, a la
Linux's fs/fuse/dir.c:fuse_dentry_revalidate(). This in turn requires that we
plumb the dentry's parent and name through kernfs.Inode.Valid().

PiperOrigin-RevId: 630476483

pkg/sentry/fsimpl/devpts/replica.go

@@ -76,7 +76,7 @@ func (ri *replicaInode) Open(ctx context.Context, rp *vfs.ResolvingPath, d *kern
 }
 
 // Valid implements kernfs.Inode.Valid.
-func (ri *replicaInode) Valid(context.Context) bool {
+func (ri *replicaInode) Valid(context.Context, *kernfs.Dentry, string) bool {
 	// Return valid if the replica still exists.
 	ri.root.mu.Lock()
 	defer ri.root.mu.Unlock()

pkg/sentry/fsimpl/fuse/BUILD

@@ -28,6 +28,20 @@ go_template_instance(
     },
 )
 
+go_template_instance(
+    name = "seqatomic_time",
+    out = "seqatomic_time_unsafe.go",
+    imports = {
+        "time": "gvisor.dev/gvisor/pkg/sentry/kernel/time",
+    },
+    package = "fuse",
+    suffix = "Time",
+    template = "//pkg/sync/seqatomic:generic_seqatomic",
+    types = {
+        "Value": "time.Time",
+    },
+)
+
 go_library(
     name = "fuse",
     srcs = [
@@ -46,6 +60,7 @@ go_library(
         "request_list.go",
         "request_response.go",
         "save_restore.go",
+        "seqatomic_time_unsafe.go",
     ],
     marshal = True,
     visibility = ["//pkg/sentry:internal"],

pkg/sentry/fsimpl/fuse/fusefs.go

@@ -301,16 +301,16 @@ func (fs *filesystem) newRoot(ctx context.Context, creds *auth.Credentials, mode
 	return &d
 }
 
-func (fs *filesystem) newInode(ctx context.Context, nodeID uint64, out linux.FUSEEntryOut) kernfs.Inode {
+func (fs *filesystem) newInode(ctx context.Context, out linux.FUSEEntryOut) kernfs.Inode {
 	attr := out.Attr
-	i := &inode{fs: fs, nodeID: nodeID}
-	i.updateEntryTime(int64(out.EntryValid), int64(out.EntryValidNSec))
+	i := &inode{fs: fs, nodeID: out.NodeID, generation: out.Generation}
 	i.attrMu.Lock()
 	defer i.attrMu.Unlock()
 
 	creds := auth.Credentials{EffectiveKGID: auth.KGID(attr.UID), EffectiveKUID: auth.KUID(attr.UID)}
-	i.init(&creds, linux.UNNAMED_MAJOR, fs.devMinor, nodeID, linux.FileMode(attr.Mode), attr.Nlink)
+	i.init(&creds, linux.UNNAMED_MAJOR, fs.devMinor, out.NodeID, linux.FileMode(attr.Mode), attr.Nlink)
 	i.updateAttrs(attr, int64(out.AttrValid), int64(out.AttrValidNSec))
+	i.updateEntryTime(int64(out.EntryValid), int64(out.EntryValidNSec))
 
 	i.OrderedChildren.Init(kernfs.OrderedChildrenOptions{})
 	i.InitRefs()

pkg/sentry/fsimpl/fuse/inode.go

@@ -16,7 +16,6 @@ package fuse
 
 import (
 	"fmt"
-	"sync"
 	gotime "time"
 
 	"gvisor.dev/gvisor/pkg/abi/linux"
@@ -32,6 +31,7 @@ import (
 	"gvisor.dev/gvisor/pkg/sentry/kernel/auth"
 	"gvisor.dev/gvisor/pkg/sentry/kernel/time"
 	"gvisor.dev/gvisor/pkg/sentry/vfs"
+	"gvisor.dev/gvisor/pkg/sync"
 )
 
 // +stateify savable
@@ -55,12 +55,18 @@ type inode struct {
 	// the owning filesystem. fs is immutable.
 	fs *filesystem
 
-	// nodeID is a unique id which identifies the inode between userspace
-	// and the sentry. Immutable.
-	nodeID uint64
+	// nodeID is a unique id which identifies the inode between userspace and
+	// the sentry. generation is used to distinguish inodes in case of nodeID
+	// reuse. Both are immutable.
+	nodeID     uint64
+	generation uint64
 
-	// entryTime is the time at which the entry becomes invalid.
-	entryTime time.Time
+	// entryTime is the time at which the entry must be revalidated. Reading
+	// entryTime requires either using entryTimeSeq and SeqAtomicLoadTime, or
+	// that attrMu is locked. Writing entryTime requires that attrMu is locked
+	// and that entryTimeSeq is in a writer critical section.
+	entryTimeSeq sync.SeqCount `state:"nosave"`
+	entryTime    time.Time
 
 	// attrVersion is the version of the last attribute change.
 	attrVersion atomicbitops.Uint64
@@ -193,9 +199,10 @@ func (i *inode) init(creds *auth.Credentials, devMajor, devMinor uint32, nodeid
 	i.ctime.Store(now)
 }
 
+// +checklocks:i.attrMu
 func (i *inode) updateEntryTime(entrySec, entryNSec int64) {
 	entryTime := time.FromTimespec(linux.Timespec{Sec: entrySec, Nsec: entryNSec})
-	i.entryTime = i.fs.clock.Now().AddTime(entryTime)
+	SeqAtomicStoreTime(&i.entryTimeSeq, &i.entryTime, i.fs.clock.Now().AddTime(entryTime))
 }
 
 // CheckPermissions implements kernfs.Inode.CheckPermissions.
@@ -374,8 +381,45 @@ func (i *inode) Open(ctx context.Context, rp *vfs.ResolvingPath, d *kernfs.Dentr
 	return &fd.vfsfd, nil
 }
 
-func (i *inode) Valid(ctx context.Context) bool {
-	return i.entryTime.After(i.fs.clock.Now())
+func (i *inode) Valid(ctx context.Context, parent *kernfs.Dentry, name string) bool {
+	now := i.fs.clock.Now()
+	if entryTime := SeqAtomicLoadTime(&i.entryTimeSeq, &i.entryTime); entryTime.After(now) {
+		return true
+	}
+
+	i.attrMu.Lock()
+	defer i.attrMu.Unlock()
+	if i.entryTime.After(now) {
+		return true
+	}
+
+	in := linux.FUSELookupIn{Name: linux.CString(name)}
+	req := i.fs.conn.NewRequest(auth.CredentialsFromContext(ctx), pidFromContext(ctx), parent.Inode().(*inode).nodeID, linux.FUSE_LOOKUP, &in)
+	res, err := i.fs.conn.Call(ctx, req)
+	if err != nil {
+		return false
+	}
+	if res.Error() != nil {
+		return false
+	}
+	var out linux.FUSEEntryOut
+	if res.UnmarshalPayload(&out) != nil {
+		return false
+	}
+	if i.nodeID != out.NodeID {
+		return false
+	}
+	// Don't enforce fuse_invalid_attr() => fuse_valid_type(),
+	// fuse_valid_size() since inode.updateAttrs() and its callers
+	// don't. But do enforce fuse_stale_inode():
+	if i.generation != out.Generation {
+		return false
+	}
+	if (i.mode.RacyLoad()^out.Attr.Mode)&linux.S_IFMT != 0 {
+		return false
+	}
+	i.updateEntryTime(int64(out.EntryValid), int64(out.EntryValidNSec))
+	return true
 }
 
 // Lookup implements kernfs.Inode.Lookup.
@@ -521,7 +565,7 @@ func (i *inode) newEntry(ctx context.Context, name string, fileType linux.FileMo
 	if opcode != linux.FUSE_LOOKUP && ((out.Attr.Mode&linux.S_IFMT)^uint32(fileType) != 0 || out.NodeID == 0 || out.NodeID == linux.FUSE_ROOT_ID) {
 		return nil, linuxerr.EIO
 	}
-	child := i.fs.newInode(ctx, out.NodeID, out.FUSEEntryOut)
+	child := i.fs.newInode(ctx, out.FUSEEntryOut)
 	if opcode == linux.FUSE_CREATE {
 		// File handler is returned by fuse server at a time of file create.
 		// Save it temporary in a created child, so Open could return it when invoked

pkg/sentry/fsimpl/kernfs/filesystem.go

@@ -71,9 +71,7 @@ func (fs *Filesystem) stepExistingLocked(ctx context.Context, rp *vfs.ResolvingP
 	if len(name) > linux.NAME_MAX {
 		return nil, false, linuxerr.ENAMETOOLONG
 	}
-	d.dirMu.Lock()
-	next, err := fs.revalidateChildLocked(ctx, rp.VirtualFilesystem(), d, name, d.children[name])
-	d.dirMu.Unlock()
+	next, err := fs.revalidateChildLocked(ctx, rp.VirtualFilesystem(), d, name)
 	if err != nil {
 		return nil, false, err
 	}
@@ -98,37 +96,30 @@ func (fs *Filesystem) stepExistingLocked(ctx context.Context, rp *vfs.ResolvingP
 	return next, false, nil
 }
 
-// revalidateChildLocked must be called after a call to parent.vfsd.Child(name)
-// or vfs.ResolvingPath.ResolveChild(name) returns childVFSD (which may be
-// nil) to verify that the returned child (or lack thereof) is correct.
+// revalidateChildLocked is called to look up the child of parent named name,
+// while verifying that any cached lookups are still correct.
 //
 // Preconditions:
 //   - Filesystem.mu must be locked for at least reading.
-//   - parent.dirMu must be locked.
 //   - parent.isDir().
 //   - name is not "." or "..".
 //
 // Postconditions: Caller must call fs.processDeferredDecRefs*.
-func (fs *Filesystem) revalidateChildLocked(ctx context.Context, vfsObj *vfs.VirtualFilesystem, parent *Dentry, name string, child *Dentry) (*Dentry, error) {
-	if child != nil {
+func (fs *Filesystem) revalidateChildLocked(ctx context.Context, vfsObj *vfs.VirtualFilesystem, parent *Dentry, name string) (*Dentry, error) {
+	parent.dirMu.Lock()
+	defer parent.dirMu.Unlock() // may be temporarily unlocked and re-locked below
+	child := parent.children[name]
+	for child != nil {
 		// Cached dentry exists, revalidate.
-		if !child.inode.Valid(ctx) {
-			childInode, err := parent.inode.Lookup(ctx, name)
-			if err != nil {
-				delete(parent.children, child.name)
-				if child.inode.Keep() {
-					fs.deferDecRef(child)
-				}
-				rcs := vfsObj.InvalidateDentry(ctx, child.VFSDentry())
-				for _, rc := range rcs {
-					fs.deferDecRef(rc)
-				}
-				return nil, err
-			}
-			fs.deferDecRef(child.inode)
-			child.inode = childInode
-			return child, nil
+		if child.inode.Valid(ctx, parent, name) {
+			break
 		}
+		delete(parent.children, child.name)
+		parent.dirMu.Unlock()
+		fs.invalidateRemovedChildLocked(ctx, vfsObj, child)
+		parent.dirMu.Lock()
+		// Check for concurrent insertion of a new cached dentry.
+		child = parent.children[name]
 	}
 	if child == nil {
 		// Dentry isn't cached; it either doesn't exist or failed revalidation.
@@ -153,6 +144,33 @@ func (fs *Filesystem) revalidateChildLocked(ctx context.Context, vfsObj *vfs.Vir
 	return child, nil
 }
 
+// Preconditions:
+//   - Filesystem.mu must be locked for at least reading.
+//   - d has been removed from its parent.children.
+//
+// Postconditions: Caller must call fs.processDeferredDecRefs*.
+func (fs *Filesystem) invalidateRemovedChildLocked(ctx context.Context, vfsObj *vfs.VirtualFilesystem, d *Dentry) {
+	if d.inode.Keep() {
+		fs.deferDecRef(d)
+	}
+	rcs := vfsObj.InvalidateDentry(ctx, d.VFSDentry())
+	for _, rc := range rcs {
+		fs.deferDecRef(rc)
+	}
+	if d.isDir() {
+		var children []*Dentry
+		d.dirMu.Lock()
+		for name, child := range d.children {
+			children = append(children, child)
+			delete(d.children, name)
+		}
+		d.dirMu.Unlock()
+		for _, child := range children {
+			fs.invalidateRemovedChildLocked(ctx, vfsObj, child)
+		}
+	}
+}
+
 // walkExistingLocked resolves rp to an existing file.
 //
 // walkExistingLocked is loosely analogous to Linux's
@@ -699,9 +717,7 @@ func (fs *Filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 
 	srcDirVFSD := oldParentVD.Dentry()
 	srcDir := srcDirVFSD.Impl().(*Dentry)
-	srcDir.dirMu.Lock()
-	src, err := fs.revalidateChildLocked(ctx, rp.VirtualFilesystem(), srcDir, oldName, srcDir.children[oldName])
-	srcDir.dirMu.Unlock()
+	src, err := fs.revalidateChildLocked(ctx, rp.VirtualFilesystem(), srcDir, oldName)
 	if err != nil {
 		return err
 	}

pkg/sentry/fsimpl/kernfs/inode_impl_util.go

@@ -789,7 +789,7 @@ func (s *StaticDirectory) DecRef(ctx context.Context) {
 type InodeAlwaysValid struct{}
 
 // Valid implements Inode.Valid.
-func (*InodeAlwaysValid) Valid(context.Context) bool {
+func (*InodeAlwaysValid) Valid(context.Context, *Dentry, string) bool {
 	return true
 }
 

pkg/sentry/fsimpl/kernfs/kernfs.go

@@ -640,11 +640,7 @@ func (d *Dentry) WalkDentryTree(ctx context.Context, vfsObj *vfs.VirtualFilesyst
 			// way to the child, and we're still holding fs.mu.
 		default:
 			var err error
-
-			d.dirMu.Lock()
-			target, err = d.fs.revalidateChildLocked(ctx, vfsObj, target, pc, target.children[pc])
-			d.dirMu.Unlock()
-
+			target, err = d.fs.revalidateChildLocked(ctx, vfsObj, target, pc)
 			if err != nil {
 				return nil, err
 			}
@@ -724,7 +720,7 @@ type Inode interface {
 
 	// Valid should return true if this inode is still valid, or needs to
 	// be resolved again by a call to Lookup.
-	Valid(ctx context.Context) bool
+	Valid(ctx context.Context, parent *Dentry, name string) bool
 
 	// Watches returns the set of inotify watches associated with this inode.
 	Watches() *vfs.Watches

pkg/sentry/fsimpl/proc/task.go

@@ -116,7 +116,7 @@ func (fs *filesystem) newTaskInode(ctx context.Context, task *kernel.Task, pidns
 // Valid implements kernfs.Inode.Valid. This inode remains valid as long
 // as the task is still running. When it's dead, another tasks with the same
 // PID could replace it.
-func (i *taskInode) Valid(ctx context.Context) bool {
+func (i *taskInode) Valid(ctx context.Context, parent *kernfs.Dentry, name string) bool {
 	return i.task.ExitState() != kernel.TaskExitDead
 }
 
@@ -169,8 +169,8 @@ func (fs *filesystem) newTaskOwnedDir(ctx context.Context, task *kernel.Task, in
 	return &taskOwnedInode{Inode: dir, owner: task}
 }
 
-func (i *taskOwnedInode) Valid(ctx context.Context) bool {
-	return i.owner.ExitState() != kernel.TaskExitDead && i.Inode.Valid(ctx)
+func (i *taskOwnedInode) Valid(ctx context.Context, parent *kernfs.Dentry, name string) bool {
+	return i.owner.ExitState() != kernel.TaskExitDead && i.Inode.Valid(ctx, parent, name)
 }
 
 // Stat implements kernfs.Inode.Stat.

pkg/sentry/fsimpl/proc/task_fds.go

@@ -245,7 +245,7 @@ func (s *fdSymlink) Getlink(ctx context.Context, mnt *vfs.Mount) (vfs.VirtualDen
 }
 
 // Valid implements kernfs.Inode.Valid.
-func (s *fdSymlink) Valid(ctx context.Context) bool {
+func (s *fdSymlink) Valid(ctx context.Context, parent *kernfs.Dentry, name string) bool {
 	return taskFDExists(ctx, s.fs, s.task, s.fd)
 }
 
@@ -349,6 +349,6 @@ func (d *fdInfoData) Generate(ctx context.Context, buf *bytes.Buffer) error {
 }
 
 // Valid implements kernfs.Inode.Valid.
-func (d *fdInfoData) Valid(ctx context.Context) bool {
+func (d *fdInfoData) Valid(ctx context.Context, parent *kernfs.Dentry, name string) bool {
 	return taskFDExists(ctx, d.fs, d.task, d.fd)
 }

pkg/sync/seqatomic/generic_seqatomic_unsafe.go

@@ -48,3 +48,27 @@ func SeqAtomicTryLoad(seq *sync.SeqCount, epoch sync.SeqCountEpoch, ptr *Value)
 	ok = seq.ReadOk(epoch)
 	return
 }
+
+// SeqAtomicStore sets *ptr to a copy of val, ensuring that any racing reader
+// critical sections are forced to retry.
+//
+//go:nosplit
+func SeqAtomicStore(seq *sync.SeqCount, ptr *Value, val Value) {
+	seq.BeginWrite()
+	SeqAtomicStoreSeqed(ptr, val)
+	seq.EndWrite()
+}
+
+// SeqAtomicStoreSeqed sets *ptr to a copy of val.
+//
+// Preconditions: ptr is protected by a SeqCount that will be in a writer
+// critical section throughout the call to SeqAtomicStore.
+//
+//go:nosplit
+func SeqAtomicStoreSeqed(ptr *Value, val Value) {
+	if sync.RaceEnabled {
+		gohacks.Memmove(unsafe.Pointer(ptr), unsafe.Pointer(&val), unsafe.Sizeof(val))
+	} else {
+		*ptr = val
+	}
+}

test/syscalls/linux/BUILD

@@ -1874,6 +1874,7 @@ cc_binary(
         "@com_google_absl//absl/container:btree",
         "@com_google_absl//absl/container:flat_hash_set",
         "@com_google_absl//absl/container:node_hash_set",
+        "@com_google_absl//absl/flags:flag",
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/strings:str_format",
         "@com_google_absl//absl/synchronization",

test/syscalls/linux/proc.cc

@@ -53,6 +53,7 @@
 #include "absl/container/btree_map.h"
 #include "absl/container/flat_hash_set.h"
 #include "absl/container/node_hash_set.h"
+#include "absl/flags/flag.h"
 #include "absl/strings/ascii.h"
 #include "absl/strings/match.h"
 #include "absl/strings/numbers.h"
@@ -101,6 +102,9 @@ using ::testing::UnorderedElementsAreArray;
 // Exported by glibc.
 extern char** environ;
 
+ABSL_FLAG(bool, proc_pid_reuse_child, false,
+          "If true, run the Proc_PidReuse child workload.");
+
 namespace gvisor {
 namespace testing {
 namespace {
@@ -2912,6 +2916,32 @@ TEST(Proc, RegressionTestB236035339) {
               SyscallFailsWithErrno(ENOTDIR));
 }
 
+// NOTE(b/338393279): Tests that after execve() from a non-leader thread
+// changes which thread owns the thread group ID, the new thread group leader
+// can access its /proc/self.
+TEST(Proc, PidReuse) {
+  const ExecveArray owned_child_argv = {"/proc/self/exe",
+                                        "--proc_pid_reuse_child"};
+  char* const* const child_argv = owned_child_argv.get();
+
+  const auto rest = [child_argv] {
+    struct stat statbuf;
+    TEST_PCHECK(stat("/proc/self/cwd", &statbuf) == 0);
+
+    ScopedThread([child_argv] {
+      execve(child_argv[0], child_argv, /* envp = */ nullptr);
+      TEST_PCHECK_MSG(false, "Survived execve to test child");
+    });
+  };
+  EXPECT_THAT(InForkedProcess(rest), IsPosixErrorOkAndHolds(0));
+}
+
+[[noreturn]] void RunProcPidReuseChild() {
+  struct stat statbuf;
+  TEST_PCHECK(stat("/proc/self/cwd", &statbuf) == 0);
+  _exit(0);
+}
+
 TEST(ProcFilesystems, ReadCapLastCap) {
   std::string lastCapStr =
       ASSERT_NO_ERRNO_AND_VALUE(GetContents("/proc/sys/kernel/cap_last_cap"));
@@ -2945,5 +2975,10 @@ int main(int argc, char** argv) {
   }
 
   gvisor::testing::TestInit(&argc, &argv);
+
+  if (absl::GetFlag(FLAGS_proc_pid_reuse_child)) {
+    gvisor::testing::RunProcPidReuseChild();
+  }
+
   return gvisor::testing::RunAllTests();
 }