From 9d5198a863bf446facdb50d48f2dae4a5e05f583 Mon Sep 17 00:00:00 2001 From: Lucas Manning Date: Tue, 3 Oct 2023 11:54:27 -0700 Subject: [PATCH] Add IsDescendant to FilesystemImpl. IsDescendant gives the VFS layer an easy way to check if a dentry is a descendant of another, which is important for some mounting procedures. This method does not take any locks when accessing the parent, so parent fields need to switch to be atomic to avoid data races. PiperOrigin-RevId: 570454446 --- pkg/sentry/fsimpl/gofer/dentry_impl.go | 4 +-- pkg/sentry/fsimpl/gofer/directfs_dentry.go | 19 ++++++----- pkg/sentry/fsimpl/gofer/directory.go | 2 +- pkg/sentry/fsimpl/gofer/filesystem.go | 13 ++++--- pkg/sentry/fsimpl/gofer/gofer.go | 32 +++++++++-------- pkg/sentry/fsimpl/gofer/revalidate.go | 23 +++++++------ pkg/sentry/fsimpl/gofer/save_restore.go | 10 ++++++ pkg/sentry/fsimpl/kernfs/filesystem.go | 17 ++++++---- pkg/sentry/fsimpl/kernfs/kernfs.go | 33 ++++++++++-------- pkg/sentry/fsimpl/kernfs/save_restore.go | 10 ++++++ pkg/sentry/fsimpl/overlay/copy_up.go | 9 ++--- pkg/sentry/fsimpl/overlay/filesystem.go | 11 +++--- pkg/sentry/fsimpl/overlay/overlay.go | 22 +++++++----- pkg/sentry/fsimpl/overlay/save_restore.go | 10 ++++++ pkg/sentry/fsimpl/tmpfs/directory.go | 2 +- pkg/sentry/fsimpl/tmpfs/filesystem.go | 16 ++++++--- pkg/sentry/fsimpl/tmpfs/save_restore.go | 10 ++++++ pkg/sentry/fsimpl/tmpfs/tmpfs.go | 8 +++-- pkg/sentry/vfs/anonfs.go | 3 ++ pkg/sentry/vfs/filesystem.go | 9 +++++ pkg/sentry/vfs/genericfstree/genericfstree.go | 34 ++++++++++++++----- pkg/sentry/vfs/vfs.go | 7 +++- 22 files changed, 206 insertions(+), 98 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/dentry_impl.go b/pkg/sentry/fsimpl/gofer/dentry_impl.go index 899de8377..78d54f65f 100644 --- a/pkg/sentry/fsimpl/gofer/dentry_impl.go +++ b/pkg/sentry/fsimpl/gofer/dentry_impl.go @@ -505,14 +505,14 @@ func (fs *filesystem) restoreRoot(ctx context.Context, opts *vfs.CompleteRestore func (d *dentry) restoreFile(ctx context.Context, opts *vfs.CompleteRestoreOptions) error { switch dt := d.impl.(type) { case *lisafsDentry: - inode, err := d.parent.impl.(*lisafsDentry).controlFD.Walk(ctx, d.name) + inode, err := d.parent.Load().impl.(*lisafsDentry).controlFD.Walk(ctx, d.name) if err != nil { return err } return dt.restoreFile(ctx, &inode, opts) case *directfsDentry: childFD, err := tryOpen(func(flags int) (int, error) { - return unix.Openat(d.parent.impl.(*directfsDentry).controlFD, d.name, flags, 0) + return unix.Openat(d.parent.Load().impl.(*directfsDentry).controlFD, d.name, flags, 0) }) if err != nil { return err diff --git a/pkg/sentry/fsimpl/gofer/directfs_dentry.go b/pkg/sentry/fsimpl/gofer/directfs_dentry.go index a9429cc85..3deac2609 100644 --- a/pkg/sentry/fsimpl/gofer/directfs_dentry.go +++ b/pkg/sentry/fsimpl/gofer/directfs_dentry.go @@ -148,7 +148,8 @@ func (fs *filesystem) newDirectfsDentry(controlFD int) (*dentry, error) { // Precondition: fs.renameMu is locked. func (d *directfsDentry) openHandle(ctx context.Context, flags uint32) (handle, error) { - if d.parent == nil { + parent := d.parent.Load() + if parent == nil { // This is a mount point. We don't have parent. Fallback to using lisafs. if !d.controlFDLisa.Ok() { panic("directfsDentry.controlFDLisa is not set for mount point dentry") @@ -168,7 +169,7 @@ func (d *directfsDentry) openHandle(ctx context.Context, flags uint32) (handle, // The only way to re-open an FD with different flags is via procfs or // openat(2) from the parent. Procfs does not exist here. So use parent. flags |= hostOpenFlags - openFD, err := unix.Openat(d.parent.impl.(*directfsDentry).controlFD, d.name, int(flags), 0) + openFD, err := unix.Openat(parent.impl.(*directfsDentry).controlFD, d.name, int(flags), 0) if err != nil { return noHandle, err } @@ -185,9 +186,9 @@ func (d *directfsDentry) ensureLisafsControlFD(ctx context.Context) error { var names []string root := d - for root.parent != nil { + for root.parent.Load() != nil { names = append(names, root.name) - root = root.parent.impl.(*directfsDentry) + root = root.parent.Load().impl.(*directfsDentry) } if !root.controlFDLisa.Ok() { panic("controlFDLisa is not set for mount point dentry") @@ -270,10 +271,10 @@ func (d *directfsDentry) chmod(ctx context.Context, mode uint16) error { // fchmod(2) on socket files created via bind(2) fails. We need to // fchmodat(2) it from its parent. - if d.parent != nil { + if parent := d.parent.Load(); parent != nil { // We have parent FD, just use that. Note that AT_SYMLINK_NOFOLLOW flag is // currently not supported. So we don't use it. - return unix.Fchmodat(d.parent.impl.(*directfsDentry).controlFD, d.name, uint32(mode), 0 /* flags */) + return unix.Fchmodat(parent.impl.(*directfsDentry).controlFD, d.name, uint32(mode), 0 /* flags */) } // This is a mount point socket. We don't have a parent FD. Fallback to using @@ -320,8 +321,8 @@ func (d *directfsDentry) utimensat(ctx context.Context, stat *linux.Statx) error // utimensat operates different that other syscalls. To operate on a // symlink it *requires* AT_SYMLINK_NOFOLLOW with dirFD and a non-empty // name. - if d.parent != nil { - return fsutil.Utimensat(d.parent.impl.(*directfsDentry).controlFD, d.name, utimes, unix.AT_SYMLINK_NOFOLLOW) + if parent := d.parent.Load(); parent != nil { + return fsutil.Utimensat(parent.impl.(*directfsDentry).controlFD, d.name, utimes, unix.AT_SYMLINK_NOFOLLOW) } // This is a mount point symlink. We don't have a parent FD. Fallback to @@ -521,7 +522,7 @@ func (d *directfsDentry) link(target *directfsDentry, name string) (*dentry, err // using olddirfd to call linkat(2). // Also note that d and target are from the same mount. Given target is a // non-directory and d is a directory, target.parent must exist. - if err := unix.Linkat(target.parent.impl.(*directfsDentry).controlFD, target.name, d.controlFD, name, 0); err != nil { + if err := unix.Linkat(target.parent.Load().impl.(*directfsDentry).controlFD, target.name, d.controlFD, name, 0); err != nil { return nil, err } // Note that we don't need to set uid/gid for the new child. This is a hard diff --git a/pkg/sentry/fsimpl/gofer/directory.go b/pkg/sentry/fsimpl/gofer/directory.go index acfa9f19a..1531fe3a0 100644 --- a/pkg/sentry/fsimpl/gofer/directory.go +++ b/pkg/sentry/fsimpl/gofer/directory.go @@ -51,7 +51,7 @@ func (d *dentry) isDir() bool { // +checklocks:d.childrenMu func (d *dentry) cacheNewChildLocked(child *dentry, name string) { d.IncRef() // reference held by child on its parent - child.parent = d + child.parent.Store(d) child.name = name if d.children == nil { d.children = make(map[string]*dentry) diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go index 4a82efcea..d0d4f94a7 100644 --- a/pkg/sentry/fsimpl/gofer/filesystem.go +++ b/pkg/sentry/fsimpl/gofer/filesystem.go @@ -193,15 +193,15 @@ func (fs *filesystem) stepLocked(ctx context.Context, rp resolvingPath, d *dentr if name == ".." { if isRoot, err := rp.CheckRoot(ctx, &d.vfsd); err != nil { return nil, false, err - } else if isRoot || d.parent == nil { + } else if isRoot || d.parent.Load() == nil { rp.Advance() return d, false, nil } - if err := rp.CheckMount(ctx, &d.parent.vfsd); err != nil { + if err := rp.CheckMount(ctx, &d.parent.Load().vfsd); err != nil { return nil, false, err } rp.Advance() - return d.parent, false, nil + return d.parent.Load(), false, nil } child, err := fs.getChildAndWalkPathLocked(ctx, d, rp, ds) if err != nil { @@ -1591,7 +1591,7 @@ func (fs *filesystem) StatFSAt(ctx context.Context, rp *vfs.ResolvingPath) (linu } // If d is synthetic, invoke statfs on the first ancestor of d that isn't. for d.isSynthetic() { - d = d.parent + d = d.parent.Load() } statfs, err := d.statfs(ctx) if err != nil { @@ -1786,3 +1786,8 @@ func (fs *filesystem) MountOptions() string { } return strings.Join(opts, ",") } + +// IsDescendant implements vfs.FilesystemImpl.IsDescendant. +func (fs *filesystem) IsDescendant(vfsroot, vd vfs.VirtualDentry) bool { + return genericIsDescendant(vfsroot.Dentry(), vd.Dentry().Impl().(*dentry)) +} diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go index 679019aee..7aa0120ad 100644 --- a/pkg/sentry/fsimpl/gofer/gofer.go +++ b/pkg/sentry/fsimpl/gofer/gofer.go @@ -44,6 +44,7 @@ import ( "path" "strconv" "strings" + "sync/atomic" "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/abi/linux" @@ -788,7 +789,7 @@ type dentry struct { // parent is this dentry's parent directory. Each dentry holds a reference // on its parent. If this dentry is a filesystem root, parent is nil. // parent is protected by filesystem.renameMu. - parent *dentry + parent atomic.Pointer[dentry] `state:".(*dentry)"` // name is the name of this dentry in its parent. If this dentry is a // filesystem root, name is the empty string. name is protected by @@ -1516,8 +1517,8 @@ func (d *dentry) InotifyWithParent(ctx context.Context, events, cookie uint32, e d.fs.renameMu.RLock() // The ordering below is important, Linux always notifies the parent first. - if d.parent != nil { - d.parent.watches.Notify(ctx, d.name, events, cookie, et, d.isDeleted()) + if parent := d.parent.Load(); parent != nil { + parent.watches.Notify(ctx, d.name, events, cookie, et, d.isDeleted()) } d.watches.Notify(ctx, "", events, cookie, et, d.isDeleted()) d.fs.renameMu.RUnlock() @@ -1623,10 +1624,10 @@ func (d *dentry) checkCachingLocked(ctx context.Context, renameMuWriteLocked boo d.fs.renameMu.Lock() defer d.fs.renameMu.Unlock() } - if d.parent != nil { - d.parent.childrenMu.Lock() - delete(d.parent.children, d.name) - d.parent.childrenMu.Unlock() + if parent := d.parent.Load(); parent != nil { + parent.childrenMu.Lock() + delete(parent.children, d.name) + parent.childrenMu.Unlock() } d.destroyLocked(ctx) // +checklocksforce: see above. return @@ -1730,8 +1731,8 @@ func (d *dentry) evictLocked(ctx context.Context) { d.cachingMu.Unlock() return } - if d.parent != nil { - d.parent.opMu.Lock() + if parent := d.parent.Load(); parent != nil { + parent.opMu.Lock() if !d.vfsd.IsDead() { // Note that d can't be a mount point (in any mount namespace), since VFS // holds references on mount points. @@ -1740,15 +1741,15 @@ func (d *dentry) evictLocked(ctx context.Context) { rc.DecRef(ctx) } - d.parent.childrenMu.Lock() - delete(d.parent.children, d.name) - d.parent.childrenMu.Unlock() + parent.childrenMu.Lock() + delete(parent.children, d.name) + parent.childrenMu.Unlock() // We're only deleting the dentry, not the file it // represents, so we don't need to update // victim parent.dirents etc. } - d.parent.opMu.Unlock() + parent.opMu.Unlock() } // Safe to unlock cachingMu now that d.vfsd.IsDead(). Henceforth any // concurrent caching attempts on d will attempt to destroy it and so will @@ -1848,8 +1849,9 @@ func (d *dentry) destroyLocked(ctx context.Context) { // Drop the reference held by d on its parent without recursively locking // d.fs.renameMu. - if d.parent != nil && d.parent.decRefNoCaching() == 0 { - d.parent.checkCachingLocked(ctx, true /* renameMuWriteLocked */) + + if parent := d.parent.Load(); parent != nil && parent.decRefNoCaching() == 0 { + parent.checkCachingLocked(ctx, true /* renameMuWriteLocked */) } } diff --git a/pkg/sentry/fsimpl/gofer/revalidate.go b/pkg/sentry/fsimpl/gofer/revalidate.go index 69d80799b..0dea3bc3c 100644 --- a/pkg/sentry/fsimpl/gofer/revalidate.go +++ b/pkg/sentry/fsimpl/gofer/revalidate.go @@ -153,7 +153,7 @@ func (fs *filesystem) revalidateStep(ctx context.Context, rp resolvingPath, d *d // can only be acquired from parent to child to avoid deadlocks. if isRoot, err := rp.CheckRoot(ctx, &d.vfsd); err != nil { return nil, errRevalidationStepDone{} - } else if isRoot || d.parent == nil { + } else if isRoot || d.parent.Load() == nil { rp.Advance() return d, errPartialRevalidation{} } @@ -164,11 +164,11 @@ func (fs *filesystem) revalidateStep(ctx context.Context, rp resolvingPath, d *d // // Call rp.CheckMount() before updating d.parent's metadata, since if // we traverse to another mount then d.parent's metadata is irrelevant. - if err := rp.CheckMount(ctx, &d.parent.vfsd); err != nil { + if err := rp.CheckMount(ctx, &d.parent.Load().vfsd); err != nil { return nil, errRevalidationStepDone{} } rp.Advance() - return d.parent, errPartialRevalidation{} + return d.parent.Load(), errPartialRevalidation{} default: d.childrenMu.Lock() @@ -212,10 +212,11 @@ func (d *dentry) invalidate(ctx context.Context, vfsObj *vfs.VirtualFilesystem, // The dentry will be reloaded next time it's accessed. *ds = appendDentry(*ds, d) - d.parent.opMu.RLock() - defer d.parent.opMu.RUnlock() - d.parent.childrenMu.Lock() - defer d.parent.childrenMu.Unlock() + parent := d.parent.Load() + parent.opMu.RLock() + defer parent.opMu.RUnlock() + parent.childrenMu.Lock() + defer parent.childrenMu.Unlock() if d.isSynthetic() { // Normally we don't mark invalidated dentries as deleted since @@ -227,16 +228,16 @@ func (d *dentry) invalidate(ctx context.Context, vfsObj *vfs.VirtualFilesystem, d.decRefNoCaching() *ds = appendDentry(*ds, d) - d.parent.syntheticChildren-- - d.parent.clearDirentsLocked() + parent.syntheticChildren-- + parent.clearDirentsLocked() } // Since the opMu was just reacquired above, re-check that the // parent's child with this name is still the same. Do not touch it if // it has been replaced with a different one. - if child := d.parent.children[d.name]; child == d { + if child := parent.children[d.name]; child == d { // Invalidate dentry so it gets reloaded next time it's accessed. - delete(d.parent.children, d.name) + delete(parent.children, d.name) } } diff --git a/pkg/sentry/fsimpl/gofer/save_restore.go b/pkg/sentry/fsimpl/gofer/save_restore.go index bf91ad442..1ae976d4a 100644 --- a/pkg/sentry/fsimpl/gofer/save_restore.go +++ b/pkg/sentry/fsimpl/gofer/save_restore.go @@ -168,6 +168,16 @@ func (fd *specialFileFD) afterLoad() { } } +// saveParent is called by stateify. +func (d *dentry) saveParent() *dentry { + return d.parent.Load() +} + +// loadParent is called by stateify. +func (d *dentry) loadParent(parent *dentry) { + d.parent.Store(parent) +} + // CompleteRestore implements // vfs.FilesystemImplSaveRestoreExtension.CompleteRestore. func (fs *filesystem) CompleteRestore(ctx context.Context, opts vfs.CompleteRestoreOptions) error { diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go index 4b3e67a82..622a85ece 100644 --- a/pkg/sentry/fsimpl/kernfs/filesystem.go +++ b/pkg/sentry/fsimpl/kernfs/filesystem.go @@ -58,15 +58,15 @@ func (fs *Filesystem) stepExistingLocked(ctx context.Context, rp *vfs.ResolvingP if name == ".." { if isRoot, err := rp.CheckRoot(ctx, d.VFSDentry()); err != nil { return nil, false, err - } else if isRoot || d.parent == nil { + } else if isRoot || d.parent.Load() == nil { rp.Advance() return d, false, nil } - if err := rp.CheckMount(ctx, d.parent.VFSDentry()); err != nil { + if err := rp.CheckMount(ctx, d.Parent().VFSDentry()); err != nil { return nil, false, err } rp.Advance() - return d.parent, false, nil + return d.parent.Load(), false, nil } if len(name) > linux.NAME_MAX { return nil, false, linuxerr.ENAMETOOLONG @@ -231,7 +231,7 @@ func checkCreateLocked(ctx context.Context, creds *auth.Credentials, name string // // Preconditions: Filesystem.mu must be locked for at least reading. func checkDeleteLocked(ctx context.Context, rp *vfs.ResolvingPath, d *Dentry) error { - parent := d.parent + parent := d.parent.Load() if parent == nil { return linuxerr.EBUSY } @@ -772,7 +772,7 @@ func (fs *Filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa fs.deferDecRef(srcDir) // child (src) drops ref on old parent. dstDir.IncRef() // child (src) takes a ref on the new parent. } - src.parent = dstDir + src.parent.Store(dstDir) src.name = newName if dstDir.children == nil { dstDir.children = make(map[string]*Dentry) @@ -971,7 +971,7 @@ func (fs *Filesystem) UnlinkAt(ctx context.Context, rp *vfs.ResolvingPath) error return linuxerr.EISDIR } virtfs := rp.VirtualFilesystem() - parentDentry := d.parent + parentDentry := d.parent.Load() parentDentry.dirMu.Lock() defer parentDentry.dirMu.Unlock() mntns := vfs.MountNamespaceFromContext(ctx) @@ -1082,3 +1082,8 @@ func (fs *Filesystem) deferDecRefVD(ctx context.Context, vd vfs.VirtualDentry) { vd.DecRef(ctx) } } + +// IsDescendant implements vfs.FilesystemImpl.IsDescendant. +func (fs *Filesystem) IsDescendant(vfsroot, vd vfs.VirtualDentry) bool { + return genericIsDescendant(vfsroot.Dentry(), vd.Dentry().Impl().(*Dentry)) +} diff --git a/pkg/sentry/fsimpl/kernfs/kernfs.go b/pkg/sentry/fsimpl/kernfs/kernfs.go index a54015a47..8cb2e11ed 100644 --- a/pkg/sentry/fsimpl/kernfs/kernfs.go +++ b/pkg/sentry/fsimpl/kernfs/kernfs.go @@ -60,6 +60,7 @@ package kernfs import ( "fmt" + "sync/atomic" "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/atomicbitops" @@ -226,8 +227,9 @@ type Dentry struct { // dflags* consts above. flags atomicbitops.Uint32 - parent *Dentry - name string + parent atomic.Pointer[Dentry] `state:".(*Dentry)"` + + name string // If cached is true, dentryEntry links dentry into // Filesystem.cachedDentries. cached and dentryEntry are protected by @@ -342,7 +344,7 @@ func (d *Dentry) cacheLocked(ctx context.Context) { // because it has zero references. // Note that a dentry may not always have a parent; for example magic links // as described in Inode.Getlink. - if isDead := d.VFSDentry().IsDead(); isDead || d.parent == nil { + if isDead, parent := d.VFSDentry().IsDead(), d.parent.Load(); isDead || parent == nil { if !isDead { rcs := d.fs.vfsfs.VirtualFilesystem().InvalidateDentry(ctx, d.VFSDentry()) for _, rc := range rcs { @@ -358,8 +360,8 @@ func (d *Dentry) cacheLocked(ctx context.Context) { d.inode.Watches().HandleDeletion(ctx) } d.destroy(ctx) - if d.parent != nil { - d.parent.decRefLocked(ctx) + if parent != nil { + parent.decRefLocked(ctx) } return } @@ -409,19 +411,20 @@ func (d *Dentry) evictLocked(ctx context.Context) { // after it was inserted into fs.cachedDentries. if d.refs.Load() == 0 { if !d.vfsd.IsDead() { - d.parent.dirMu.Lock() + parent := d.parent.Load() + parent.dirMu.Lock() // Note that victim can't be a mount point (in any mount // namespace), since VFS holds references on mount points. rcs := d.fs.vfsfs.VirtualFilesystem().InvalidateDentry(ctx, d.VFSDentry()) for _, rc := range rcs { d.fs.deferDecRef(rc) } - delete(d.parent.children, d.name) - d.parent.dirMu.Unlock() + delete(parent.children, d.name) + parent.dirMu.Unlock() } d.destroy(ctx) - if d.parent != nil { - d.parent.decRefLocked(ctx) + if parent := d.parent.Load(); parent != nil { + parent.decRefLocked(ctx) } } } @@ -535,8 +538,8 @@ func (d *Dentry) InotifyWithParent(ctx context.Context, events, cookie uint32, e // won't have one. if !d.inode.Anonymous() { d.fs.mu.RLock() - if d.parent != nil { - d.parent.inode.Watches().Notify(ctx, d.name, events, cookie, et, d.isDeleted()) + if parent := d.parent.Load(); parent != nil { + parent.inode.Watches().Notify(ctx, d.name, events, cookie, et, d.isDeleted()) } d.fs.mu.RUnlock() } @@ -577,7 +580,7 @@ func (d *Dentry) insertChildLocked(name string, child *Dentry) { panic(fmt.Sprintf("insertChildLocked called on non-directory Dentry: %+v.", d)) } d.IncRef() // DecRef in child's Dentry.destroy. - child.parent = d + child.parent.Store(d) child.name = name if d.children == nil { d.children = make(map[string]*Dentry) @@ -632,7 +635,7 @@ func (d *Dentry) WalkDentryTree(ctx context.Context, vfsObj *vfs.VirtualFilesyst // Don't let .. traverse above the start point of the walk. continue } - target = target.parent + target = target.parent.Load() // Parent doesn't need revalidation since we revalidated it on the // way to the child, and we're still holding fs.mu. default: @@ -660,7 +663,7 @@ func (d *Dentry) WalkDentryTree(ctx context.Context, vfsObj *vfs.VirtualFilesyst // filesystem may concurrently move d elsewhere. The caller is responsible for // ensuring the returned result remains valid while it is used. func (d *Dentry) Parent() *Dentry { - return d.parent + return d.parent.Load() } // The Inode interface maps filesystem-level operations that operate on paths to diff --git a/pkg/sentry/fsimpl/kernfs/save_restore.go b/pkg/sentry/fsimpl/kernfs/save_restore.go index 1b2245955..9d4130165 100644 --- a/pkg/sentry/fsimpl/kernfs/save_restore.go +++ b/pkg/sentry/fsimpl/kernfs/save_restore.go @@ -32,3 +32,13 @@ func (i *inodePlatformFile) afterLoad() { i.fileMapperInitOnce.Do(func() {}) } } + +// saveParent is called by stateify. +func (d *Dentry) saveParent() *Dentry { + return d.parent.Load() +} + +// loadParent is called by stateify. +func (d *Dentry) loadParent(parent *Dentry) { + d.parent.Store(parent) +} diff --git a/pkg/sentry/fsimpl/overlay/copy_up.go b/pkg/sentry/fsimpl/overlay/copy_up.go index e6c12e0c3..6e0f45d30 100644 --- a/pkg/sentry/fsimpl/overlay/copy_up.go +++ b/pkg/sentry/fsimpl/overlay/copy_up.go @@ -65,11 +65,12 @@ func (d *dentry) copyUpMaybeSyntheticMountpointLocked(ctx context.Context, forSy } // Ensure that our parent directory is copied-up. - if d.parent == nil { + parent := d.parent.Load() + if parent == nil { // d is a filesystem root with no upper layer. return linuxerr.EROFS } - if err := d.parent.copyUpMaybeSyntheticMountpointLocked(ctx, forSyntheticMountpoint); err != nil { + if err := parent.copyUpMaybeSyntheticMountpointLocked(ctx, forSyntheticMountpoint); err != nil { return err } @@ -101,8 +102,8 @@ func (d *dentry) copyUpMaybeSyntheticMountpointLocked(ctx context.Context, forSy // Perform copy-up. ftype := d.mode.Load() & linux.S_IFMT newpop := vfs.PathOperation{ - Root: d.parent.upperVD, - Start: d.parent.upperVD, + Root: parent.upperVD, + Start: parent.upperVD, Path: fspath.Parse(d.name), } // Used during copy-up of memory-mapped regular files. diff --git a/pkg/sentry/fsimpl/overlay/filesystem.go b/pkg/sentry/fsimpl/overlay/filesystem.go index 493515dd4..aa440b40a 100644 --- a/pkg/sentry/fsimpl/overlay/filesystem.go +++ b/pkg/sentry/fsimpl/overlay/filesystem.go @@ -151,15 +151,16 @@ func (fs *filesystem) stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d * if name == ".." { if isRoot, err := rp.CheckRoot(ctx, &d.vfsd); err != nil { return nil, lookupLayerNone, false, err - } else if isRoot || d.parent == nil { + } else if isRoot || d.parent.Load() == nil { rp.Advance() return d, d.topLookupLayer(), false, nil } - if err := rp.CheckMount(ctx, &d.parent.vfsd); err != nil { + if err := rp.CheckMount(ctx, &d.parent.Load().vfsd); err != nil { return nil, lookupLayerNone, false, err } rp.Advance() - return d.parent, d.parent.topLookupLayer(), false, nil + parent := d.parent.Load() + return parent, parent.topLookupLayer(), false, nil } if uint64(len(name)) > fs.maxFilenameLen { return nil, lookupLayerNone, false, linuxerr.ENAMETOOLONG @@ -344,7 +345,7 @@ func (fs *filesystem) lookupLocked(ctx context.Context, parent *dentry, name str } parent.IncRef() - child.parent = parent + child.parent.Store(parent) child.name = name return child, topLookupLayer, nil } @@ -1335,7 +1336,7 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa oldParent.DecRef(ctx) ds = appendDentry(ds, oldParent) newParent.IncRef() - renamed.parent = newParent + renamed.parent.Store(newParent) } renamed.name = newName if newParent.children == nil { diff --git a/pkg/sentry/fsimpl/overlay/overlay.go b/pkg/sentry/fsimpl/overlay/overlay.go index fed5c13a2..d4a0a787e 100644 --- a/pkg/sentry/fsimpl/overlay/overlay.go +++ b/pkg/sentry/fsimpl/overlay/overlay.go @@ -36,6 +36,7 @@ package overlay import ( "fmt" "strings" + "sync/atomic" "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/atomicbitops" @@ -473,6 +474,11 @@ func (fs *filesystem) getLowerDevMinor(layerMajor, layerMinor uint32) (uint32, e return minor, nil } +// IsDescendant implements vfs.FilesystemImpl.IsDescendant. +func (fs *filesystem) IsDescendant(vfsroot, vd vfs.VirtualDentry) bool { + return genericIsDescendant(vfsroot.Dentry(), vd.Dentry().Impl().(*dentry)) +} + // dentry implements vfs.DentryImpl. // // +stateify savable @@ -500,7 +506,7 @@ type dentry struct { // name is this dentry's name in parent. If this dentry is a filesystem // root, parent is nil and name is the empty string. parent and name are // protected by fs.renameMu. - parent *dentry + parent atomic.Pointer[dentry] `state:".(*dentry)"` name string // If this dentry represents a directory, children maps the names of @@ -693,15 +699,15 @@ func (d *dentry) destroyLocked(ctx context.Context) { d.watches.HandleDeletion(ctx) - if d.parent != nil { - d.parent.dirMu.Lock() + if parent := d.parent.Load(); parent != nil { + parent.dirMu.Lock() if !d.vfsd.IsDead() { - delete(d.parent.children, d.name) + delete(parent.children, d.name) } - d.parent.dirMu.Unlock() + parent.dirMu.Unlock() // Drop the reference held by d on its parent without recursively // locking d.fs.renameMu. - d.parent.decRefLocked(ctx) + parent.decRefLocked(ctx) } refs.Unregister(d) } @@ -736,8 +742,8 @@ func (d *dentry) InotifyWithParent(ctx context.Context, events uint32, cookie ui d.fs.renameMu.RLock() // The ordering below is important, Linux always notifies the parent first. - if d.parent != nil { - d.parent.watches.Notify(ctx, d.name, events, cookie, et, deleted) + if parent := d.parent.Load(); parent != nil { + parent.watches.Notify(ctx, d.name, events, cookie, et, deleted) } d.watches.Notify(ctx, "", events, cookie, et, deleted) d.fs.renameMu.RUnlock() diff --git a/pkg/sentry/fsimpl/overlay/save_restore.go b/pkg/sentry/fsimpl/overlay/save_restore.go index 94fb439e6..fe78f153a 100644 --- a/pkg/sentry/fsimpl/overlay/save_restore.go +++ b/pkg/sentry/fsimpl/overlay/save_restore.go @@ -23,3 +23,13 @@ func (d *dentry) afterLoad() { refs.Register(d) } } + +// saveParent is called by stateify. +func (d *dentry) saveParent() *dentry { + return d.parent.Load() +} + +// loadParent is called by stateify. +func (d *dentry) loadParent(parent *dentry) { + d.parent.Store(parent) +} diff --git a/pkg/sentry/fsimpl/tmpfs/directory.go b/pkg/sentry/fsimpl/tmpfs/directory.go index 1383a1043..c2ae5ebda 100644 --- a/pkg/sentry/fsimpl/tmpfs/directory.go +++ b/pkg/sentry/fsimpl/tmpfs/directory.go @@ -60,7 +60,7 @@ func (fs *filesystem) newDirectory(kuid auth.KUID, kgid auth.KGID, mode linux.Fi // - filesystem.mu must be locked for writing. // - dir must not already contain a child with the given name. func (dir *directory) insertChildLocked(child *dentry, name string) { - child.parent = &dir.dentry + child.parent.Store(&dir.dentry) child.name = name if dir.childMap == nil { dir.childMap = make(map[string]*dentry) diff --git a/pkg/sentry/fsimpl/tmpfs/filesystem.go b/pkg/sentry/fsimpl/tmpfs/filesystem.go index b2d7aaa12..6958412b5 100644 --- a/pkg/sentry/fsimpl/tmpfs/filesystem.go +++ b/pkg/sentry/fsimpl/tmpfs/filesystem.go @@ -68,15 +68,15 @@ func stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry) (*dentry, if name == ".." { if isRoot, err := rp.CheckRoot(ctx, &d.vfsd); err != nil { return nil, false, err - } else if isRoot || d.parent == nil { + } else if isRoot || d.parent.Load() == nil { rp.Advance() return d, false, nil } - if err := rp.CheckMount(ctx, &d.parent.vfsd); err != nil { + if err := rp.CheckMount(ctx, &d.parent.Load().vfsd); err != nil { return nil, false, err } rp.Advance() - return d.parent, false, nil + return d.parent.Load(), false, nil } if len(name) > d.inode.fs.maxFilenameLen { return nil, false, linuxerr.ENAMETOOLONG @@ -947,7 +947,8 @@ func (fs *filesystem) PrependPath(ctx context.Context, vfsroot, vd vfs.VirtualDe if mnt != nil && &d.vfsd == mnt.Root() { return nil } - if d.parent == nil { + parent := d.parent.Load() + if parent == nil { if d.name != "" { // This file must have been created by // newUnlinkedRegularFileDescription(). In Linux, @@ -964,7 +965,7 @@ func (fs *filesystem) PrependPath(ctx context.Context, vfsroot, vd vfs.VirtualDe return vfs.PrependPathAtNonMountRootError{} } b.PrependComponent(d.name) - d = d.parent + d = parent } } @@ -973,6 +974,11 @@ func (fs *filesystem) MountOptions() string { return fs.mopts } +// IsDescendant implements vfs.FilesystemImpl.IsDescendant. +func (fs *filesystem) IsDescendant(vfsroot, vd vfs.VirtualDentry) bool { + return genericIsDescendant(vfsroot.Dentry(), vd.Dentry().Impl().(*dentry)) +} + // adjustPageAcct adjusts the accounting done against filesystem size limit in // case there is any discrepency between the number of pages reserved vs the // number of pages actually allocated. diff --git a/pkg/sentry/fsimpl/tmpfs/save_restore.go b/pkg/sentry/fsimpl/tmpfs/save_restore.go index d08975d82..b6e8f033f 100644 --- a/pkg/sentry/fsimpl/tmpfs/save_restore.go +++ b/pkg/sentry/fsimpl/tmpfs/save_restore.go @@ -22,3 +22,13 @@ func (fs *filesystem) afterLoad() { } fs.mf = fs.mfp.MemoryFile() } + +// saveParent is called by stateify. +func (d *dentry) saveParent() *dentry { + return d.parent.Load() +} + +// saveParent is called by stateify. +func (d *dentry) loadParent(parent *dentry) { + d.parent.Store(parent) +} diff --git a/pkg/sentry/fsimpl/tmpfs/tmpfs.go b/pkg/sentry/fsimpl/tmpfs/tmpfs.go index f534b7e96..29ce64050 100644 --- a/pkg/sentry/fsimpl/tmpfs/tmpfs.go +++ b/pkg/sentry/fsimpl/tmpfs/tmpfs.go @@ -33,6 +33,7 @@ import ( "math" "strconv" "strings" + "sync/atomic" "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/atomicbitops" @@ -411,7 +412,7 @@ type dentry struct { // parent is this dentry's parent directory. Each referenced dentry holds a // reference on parent.dentry. If this dentry is a filesystem root, parent // is nil. parent is protected by filesystem.mu. - parent *dentry + parent atomic.Pointer[dentry] `state:".(*dentry)"` // name is the name of this dentry in its parent. If this dentry is a // filesystem root, name is the empty string. name is protected by @@ -468,8 +469,9 @@ func (d *dentry) InotifyWithParent(ctx context.Context, events, cookie uint32, e d.inode.fs.mu.RLock() // The ordering below is important, Linux always notifies the parent first. - if d.parent != nil { - d.parent.inode.watches.Notify(ctx, d.name, events, cookie, et, deleted) + parent := d.parent.Load() + if parent != nil { + parent.inode.watches.Notify(ctx, d.name, events, cookie, et, deleted) } d.inode.watches.Notify(ctx, "", events, cookie, et, deleted) d.inode.fs.mu.RUnlock() diff --git a/pkg/sentry/vfs/anonfs.go b/pkg/sentry/vfs/anonfs.go index e8e7e2e0b..187770b2a 100644 --- a/pkg/sentry/vfs/anonfs.go +++ b/pkg/sentry/vfs/anonfs.go @@ -301,6 +301,9 @@ func (fs *anonFilesystem) MountOptions() string { return "" } +// IsDescendant implements FilesystemImpl.IsDescendant. +func (fs *anonFilesystem) IsDescendant(vfsroot, vd VirtualDentry) bool { return vfsroot == vd } + // IncRef implements DentryImpl.IncRef. func (d *anonDentry) IncRef() { // no-op diff --git a/pkg/sentry/vfs/filesystem.go b/pkg/sentry/vfs/filesystem.go index 252c691c3..f2647ea1c 100644 --- a/pkg/sentry/vfs/filesystem.go +++ b/pkg/sentry/vfs/filesystem.go @@ -511,6 +511,15 @@ type FilesystemImpl interface { // If the implementation has no filesystem-specific options, it should // return the empty string. MountOptions() string + + // IsDescendant returns true if vd is a descendant of vfsroot or if vd and + // vfsroot are the same dentry. The method does not take filesystem locks when + // accessing the parents of each dentry, so it's possible for parents to be + // mutated concurrently during a call to IsDescendant. Callers should take + // appropriate caution when using this method. + // + // Preconditions: vd.Mount().Filesystem().Impl() == this FilesystemImpl. + IsDescendant(vfsroot, vd VirtualDentry) bool } // PrependPathAtVFSRootError is returned by implementations of diff --git a/pkg/sentry/vfs/genericfstree/genericfstree.go b/pkg/sentry/vfs/genericfstree/genericfstree.go index ba6e6ed49..b4dd7c088 100644 --- a/pkg/sentry/vfs/genericfstree/genericfstree.go +++ b/pkg/sentry/vfs/genericfstree/genericfstree.go @@ -26,6 +26,13 @@ import ( "gvisor.dev/gvisor/pkg/sentry/vfs" ) +// We need to define an interface instead of using atomic.Pointer because +// the Dentry type gets removed during code generation and the compiler +// complains about the unused sync/atomic type. +type atomicptr interface { + Load() *Dentry +} + // Dentry is a required type parameter that is a struct with the given fields. // // +stateify savable @@ -35,7 +42,7 @@ type Dentry struct { // parent is the parent of this Dentry in the filesystem's tree. If this // Dentry is a filesystem root, parent is nil. - parent *Dentry + parent atomicptr // name is the name of this Dentry in its parent. If this Dentry is a // filesystem root, name is unspecified. @@ -46,21 +53,31 @@ type Dentry struct { // either d2's parent or an ancestor of d2's parent. func IsAncestorDentry(d, d2 *Dentry) bool { for d2 != nil { // Stop at root, where d2.parent == nil. - if d2.parent == d { + parent := d2.parent.Load() + if parent == d { return true } - if d2.parent == d2 { + if parent == d2 { return false } - d2 = d2.parent + d2 = parent } return false } +// IsDescendant returns true if vd is a descendant of vfsroot or if vd and +// vfsroot are the same dentry. +func IsDescendant(vfsroot *vfs.Dentry, d *Dentry) bool { + for d != nil && &d.vfsd != vfsroot { + d = d.parent.Load() + } + return d != nil +} + // ParentOrSelf returns d.parent. If d.parent is nil, ParentOrSelf returns d. func ParentOrSelf(d *Dentry) *Dentry { - if d.parent != nil { - return d.parent + if parent := d.parent.Load(); parent != nil { + return parent } return d } @@ -74,11 +91,12 @@ func PrependPath(vfsroot vfs.VirtualDentry, mnt *vfs.Mount, d *Dentry, b *fspath if mnt != nil && &d.vfsd == mnt.Root() { return nil } - if d.parent == nil { + parent := d.parent.Load() + if parent == nil { return vfs.PrependPathAtNonMountRootError{} } b.PrependComponent(d.name) - d = d.parent + d = parent } } diff --git a/pkg/sentry/vfs/vfs.go b/pkg/sentry/vfs/vfs.go index a457a9132..219e3886c 100644 --- a/pkg/sentry/vfs/vfs.go +++ b/pkg/sentry/vfs/vfs.go @@ -18,7 +18,7 @@ // // EpollInstance.interestMu // FileDescription.epollMu -// Locks acquired by FilesystemImpl/FileDescriptionImpl methods +// Locks acquired by FilesystemImpl/FileDescriptionImpl methods (except IsDescendant) // VirtualFilesystem.mountMu // Dentry.mu // Locks acquired by FilesystemImpls between Prepare{Delete,Rename}Dentry and Commit{Delete,Rename*}Dentry @@ -33,6 +33,11 @@ // Locking Dentry.mu in multiple Dentries requires holding // VirtualFilesystem.mountMu. Locking EpollInstance.interestMu in multiple // EpollInstances requires holding epollCycleMu. +// +// FilesystemImpl locks are not held during calls to FilesystemImpl.IsDescendant +// since it's called under mountMu. It's possible for concurrent mutation +// to dentry ancestors during calls IsDescendant. Callers should take +// appropriate caution when using this method. package vfs import (