diff --git a/pkg/sentry/fsimpl/devpts/replica.go b/pkg/sentry/fsimpl/devpts/replica.go index 67f9edccc..9b0fd72e7 100644 --- a/pkg/sentry/fsimpl/devpts/replica.go +++ b/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() diff --git a/pkg/sentry/fsimpl/fuse/BUILD b/pkg/sentry/fsimpl/fuse/BUILD index e479f950b..17d136724 100644 --- a/pkg/sentry/fsimpl/fuse/BUILD +++ b/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"], diff --git a/pkg/sentry/fsimpl/fuse/fusefs.go b/pkg/sentry/fsimpl/fuse/fusefs.go index cfc734fa6..4d1d6e113 100644 --- a/pkg/sentry/fsimpl/fuse/fusefs.go +++ b/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() diff --git a/pkg/sentry/fsimpl/fuse/inode.go b/pkg/sentry/fsimpl/fuse/inode.go index bcf8bae59..23b2c491a 100644 --- a/pkg/sentry/fsimpl/fuse/inode.go +++ b/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 diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go index 5a5c72ff1..365948795 100644 --- a/pkg/sentry/fsimpl/kernfs/filesystem.go +++ b/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 } diff --git a/pkg/sentry/fsimpl/kernfs/inode_impl_util.go b/pkg/sentry/fsimpl/kernfs/inode_impl_util.go index e5867c1e3..a378280e8 100644 --- a/pkg/sentry/fsimpl/kernfs/inode_impl_util.go +++ b/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 } diff --git a/pkg/sentry/fsimpl/kernfs/kernfs.go b/pkg/sentry/fsimpl/kernfs/kernfs.go index 215468147..9059d0584 100644 --- a/pkg/sentry/fsimpl/kernfs/kernfs.go +++ b/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 diff --git a/pkg/sentry/fsimpl/proc/task.go b/pkg/sentry/fsimpl/proc/task.go index 8e03c09a4..d41a3c001 100644 --- a/pkg/sentry/fsimpl/proc/task.go +++ b/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. diff --git a/pkg/sentry/fsimpl/proc/task_fds.go b/pkg/sentry/fsimpl/proc/task_fds.go index 117b063e5..83996a446 100644 --- a/pkg/sentry/fsimpl/proc/task_fds.go +++ b/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) } diff --git a/pkg/sync/seqatomic/generic_seqatomic_unsafe.go b/pkg/sync/seqatomic/generic_seqatomic_unsafe.go index 9578c9c52..327a5ed28 100644 --- a/pkg/sync/seqatomic/generic_seqatomic_unsafe.go +++ b/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 + } +} diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD index 2137a864e..26ecc8372 100644 --- a/test/syscalls/linux/BUILD +++ b/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", diff --git a/test/syscalls/linux/proc.cc b/test/syscalls/linux/proc.cc index 8b60d9fc8..59ffe0a4c 100644 --- a/test/syscalls/linux/proc.cc +++ b/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(); }