Establish lock ordering for vfs.FilesystemImpl.PrependPath.

- Add type parameter Filesystem to vfs/genericfstree, which is required to
  provide `ancestryMu sync.RWMutex`, and add such a RWMutex to all FSImpls that
  use genericfstree.

- Modify genericfstree.PrependPath() and genericfstree.IsDescendant() to use
  ancestryMu to ensure atomicity. For callers of genericfstree.PrependPath(),
  this means that (broader) FSImpl locks no longer need to be held during the
  call. For callers of genericfstree.IsDescendant(), this means that
  IsDescendant() now has the same atomicity as Linux's fs/dcache.c:is_subdir(),
  so remove documentation warnings about its non-atomicity.

PiperOrigin-RevId: 696273175

Author: nixprime

pkg/sentry/fsimpl/erofs/BUILD

@@ -14,6 +14,7 @@ go_template_instance(
     template = "//pkg/sentry/vfs/genericfstree:generic_fstree",
     types = {
         "Dentry": "dentry",
+        "Filesystem": "filesystem",
     },
 )
 
@@ -70,6 +71,7 @@ go_library(
         "//pkg/sentry/memmap",
         "//pkg/sentry/socket/unix/transport",
         "//pkg/sentry/vfs",
+        "//pkg/sync",
         "//pkg/usermem",
     ],
 )

pkg/sentry/fsimpl/erofs/erofs.go

@@ -19,7 +19,6 @@ import (
 	"os"
 	"runtime"
 	"strconv"
-	"sync"
 	"sync/atomic"
 
 	"gvisor.dev/gvisor/pkg/abi/linux"
@@ -30,6 +29,7 @@ import (
 	"gvisor.dev/gvisor/pkg/sentry/kernel/auth"
 	"gvisor.dev/gvisor/pkg/sentry/memmap"
 	"gvisor.dev/gvisor/pkg/sentry/vfs"
+	"gvisor.dev/gvisor/pkg/sync"
 )
 
 // Name is the filesystem name. It is part of the interface used by users,
@@ -72,6 +72,9 @@ type filesystem struct {
 	// reduce the lock contention. Bucket is chosen based on the hash calculation
 	// on nid in filesystem.inodeBucket.
 	inodeBuckets []inodeBucket
+
+	// ancestryMu is required by genericfstree.
+	ancestryMu sync.RWMutex `state:"nosave"`
 }
 
 // InternalFilesystemOptions may be passed as

pkg/sentry/fsimpl/erofs/filesystem.go

@@ -430,15 +430,15 @@ func (fs *filesystem) RemoveXattrAt(ctx context.Context, rp *vfs.ResolvingPath,
 
 // PrependPath implements vfs.FilesystemImpl.PrependPath.
 func (fs *filesystem) PrependPath(ctx context.Context, vfsroot, vd vfs.VirtualDentry, b *fspath.Builder) error {
-	return genericPrependPath(vfsroot, vd.Mount(), vd.Dentry().Impl().(*dentry), b)
+	return genericPrependPath(fs, vfsroot, vd.Mount(), vd.Dentry().Impl().(*dentry), b)
+}
+
+// IsDescendant implements vfs.FilesystemImpl.IsDescendant.
+func (fs *filesystem) IsDescendant(vfsroot, vd vfs.VirtualDentry) bool {
+	return genericIsDescendant(fs, vfsroot.Dentry(), vd.Dentry().Impl().(*dentry))
 }
 
 // MountOptions implements vfs.FilesystemImpl.MountOptions.
 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))
-}

pkg/sentry/fsimpl/gofer/BUILD

@@ -49,6 +49,7 @@ go_template_instance(
     template = "//pkg/sentry/vfs/genericfstree:generic_fstree",
     types = {
         "Dentry": "dentry",
+        "Filesystem": "filesystem",
     },
 )
 

pkg/sentry/fsimpl/gofer/dentry_impl.go

@@ -538,14 +538,14 @@ func (d *dentry) restoreFile(ctx context.Context, opts *vfs.CompleteRestoreOptio
 		inode, err := controlFD.Walk(ctx, d.name)
 		if err != nil {
 			if !dt.isDir() || !dt.forMountpoint {
-				return fmt.Errorf("failed to walk %q of type %x: %w", genericDebugPathname(d), dt.fileType(), err)
+				return fmt.Errorf("failed to walk %q of type %x: %w", genericDebugPathname(d.fs, d), dt.fileType(), err)
 			}
 
 			// Recreate directories that were created during volume mounting, since
 			// during restore we don't attempt to remount them.
 			inode, err = controlFD.MkdirAt(ctx, d.name, linux.FileMode(d.mode.Load()), lisafs.UID(d.uid.Load()), lisafs.GID(d.gid.Load()))
 			if err != nil {
-				return fmt.Errorf("failed to create mountpoint directory at %q: %w", genericDebugPathname(d), err)
+				return fmt.Errorf("failed to create mountpoint directory at %q: %w", genericDebugPathname(d.fs, d), err)
 			}
 		}
 		return dt.restoreFile(ctx, &inode, opts)
@@ -558,21 +558,21 @@ func (d *dentry) restoreFile(ctx context.Context, opts *vfs.CompleteRestoreOptio
 		})
 		if err != nil {
 			if !dt.isDir() || !dt.forMountpoint {
-				return fmt.Errorf("failed to walk %q of type %x: %w", genericDebugPathname(d), dt.fileType(), err)
+				return fmt.Errorf("failed to walk %q of type %x: %w", genericDebugPathname(d.fs, d), dt.fileType(), err)
 			}
 
 			// Recreate directories that were created during volume mounting, since
 			// during restore we don't attempt to remount them.
 			if err := unix.Mkdirat(controlFD, d.name, d.mode.Load()); err != nil {
-				return fmt.Errorf("failed to create mountpoint directory at %q: %w", genericDebugPathname(d), err)
+				return fmt.Errorf("failed to create mountpoint directory at %q: %w", genericDebugPathname(d.fs, d), err)
 			}
 
 			// Try again...
 			childFD, err = tryOpen(func(flags int) (int, error) {
 				return unix.Openat(controlFD, d.name, flags, 0)
 			})
 			if err != nil {
-				return fmt.Errorf("failed to open %q: %w", genericDebugPathname(d), err)
+				return fmt.Errorf("failed to open %q: %w", genericDebugPathname(d.fs, d), err)
 			}
 		}
 		return dt.restoreFile(ctx, childFD, opts)

pkg/sentry/fsimpl/gofer/directfs_dentry.go

@@ -452,7 +452,7 @@ func (d *directfsDentry) getCreatedChild(name string, uid, gid int, isDir bool)
 	deleteChild := func() {
 		// Best effort attempt to remove the newly created child on failure.
 		if err := unix.Unlinkat(d.controlFD, name, unlinkFlags); err != nil {
-			log.Warningf("error unlinking newly created child %q after failure: %v", filepath.Join(genericDebugPathname(&d.dentry), name), err)
+			log.Warningf("error unlinking newly created child %q after failure: %v", filepath.Join(genericDebugPathname(d.fs, &d.dentry), name), err)
 		}
 	}
 
@@ -518,7 +518,7 @@ func (d *directfsDentry) bindAt(ctx context.Context, name string, creds *auth.Cr
 	hbep := opts.Endpoint.(transport.HostBoundEndpoint)
 	if err := hbep.SetBoundSocketFD(ctx, boundSocketFD); err != nil {
 		if err := unix.Unlinkat(d.controlFD, name, 0); err != nil {
-			log.Warningf("error unlinking newly created socket %q after failure: %v", filepath.Join(genericDebugPathname(&d.dentry), name), err)
+			log.Warningf("error unlinking newly created socket %q after failure: %v", filepath.Join(genericDebugPathname(d.fs, &d.dentry), name), err)
 		}
 		return nil, err
 	}
@@ -593,7 +593,7 @@ func (d *directfsDentry) getDirentsLocked(recordDirent func(name string, key ino
 		// TODO(gvisor.dev/issue/6665): Get rid of per-dirent stat.
 		stat, err := fsutil.StatAt(d.controlFD, name)
 		if err != nil {
-			log.Warningf("Getdent64: skipping file %q with failed stat, err: %v", path.Join(genericDebugPathname(&d.dentry), name), err)
+			log.Warningf("Getdent64: skipping file %q with failed stat, err: %v", path.Join(genericDebugPathname(d.fs, &d.dentry), name), err)
 			return
 		}
 		recordDirent(name, inoKeyFromStat(&stat), ftype)
@@ -650,7 +650,7 @@ func (d *directfsDentry) restoreFile(ctx context.Context, controlFD int, opts *v
 	var stat unix.Stat_t
 	if err := unix.Fstat(controlFD, &stat); err != nil {
 		_ = unix.Close(controlFD)
-		return fmt.Errorf("failed to stat %q: %w", genericDebugPathname(&d.dentry), err)
+		return fmt.Errorf("failed to stat %q: %w", genericDebugPathname(d.fs, &d.dentry), err)
 	}
 
 	d.controlFD = controlFD
@@ -672,12 +672,12 @@ func (d *directfsDentry) restoreFile(ctx context.Context, controlFD int, opts *v
 	if d.isRegularFile() {
 		if opts.ValidateFileSizes {
 			if d.size.RacyLoad() != uint64(stat.Size) {
-				return vfs.ErrCorruption{fmt.Errorf("gofer.dentry(%q).restoreFile: file size validation failed: size changed from %d to %d", genericDebugPathname(&d.dentry), d.size.Load(), stat.Size)}
+				return vfs.ErrCorruption{fmt.Errorf("gofer.dentry(%q).restoreFile: file size validation failed: size changed from %d to %d", genericDebugPathname(d.fs, &d.dentry), d.size.Load(), stat.Size)}
 			}
 		}
 		if opts.ValidateFileModificationTimestamps {
 			if want := dentryTimestampFromUnix(stat.Mtim); d.mtime.RacyLoad() != want {
-				return vfs.ErrCorruption{fmt.Errorf("gofer.dentry(%q).restoreFile: mtime validation failed: mtime changed from %+v to %+v", genericDebugPathname(&d.dentry), linux.NsecToStatxTimestamp(d.mtime.RacyLoad()), linux.NsecToStatxTimestamp(want))}
+				return vfs.ErrCorruption{fmt.Errorf("gofer.dentry(%q).restoreFile: mtime validation failed: mtime changed from %+v to %+v", genericDebugPathname(d.fs, &d.dentry), linux.NsecToStatxTimestamp(d.mtime.RacyLoad()), linux.NsecToStatxTimestamp(want))}
 			}
 		}
 	}
@@ -687,7 +687,7 @@ func (d *directfsDentry) restoreFile(ctx context.Context, controlFD int, opts *v
 
 	if rw, ok := d.fs.savedDentryRW[&d.dentry]; ok {
 		if err := d.ensureSharedHandle(ctx, rw.read, rw.write, false /* trunc */); err != nil {
-			return fmt.Errorf("failed to restore file handles (read=%t, write=%t) for %q: %w", rw.read, rw.write, genericDebugPathname(&d.dentry), err)
+			return fmt.Errorf("failed to restore file handles (read=%t, write=%t) for %q: %w", rw.read, rw.write, genericDebugPathname(d.fs, &d.dentry), err)
 		}
 	}
 

pkg/sentry/fsimpl/gofer/directory.go

@@ -51,8 +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.Store(d)
-	child.name = name
+	genericSetParentAndName(d.fs, child, d, name)
 	if d.children == nil {
 		d.children = make(map[string]*dentry)
 	} else if c, ok := d.children[name]; ok {

pkg/sentry/fsimpl/gofer/filesystem.go

@@ -1418,7 +1418,7 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 		return err
 	}
 	if renamed.isDir() {
-		if renamed == newParent || genericIsAncestorDentry(renamed, newParent) {
+		if renamed == newParent || genericIsAncestorDentry(fs, renamed, newParent) {
 			return linuxerr.EINVAL
 		}
 		if oldParent != newParent {
@@ -1456,7 +1456,7 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 			if !renamed.isDir() {
 				return linuxerr.EISDIR
 			}
-			if genericIsAncestorDentry(replaced, renamed) {
+			if genericIsAncestorDentry(fs, replaced, renamed) {
 				return linuxerr.ENOTEMPTY
 			}
 		} else {
@@ -1738,9 +1738,12 @@ func (fs *filesystem) RemoveXattrAt(ctx context.Context, rp *vfs.ResolvingPath,
 
 // PrependPath implements vfs.FilesystemImpl.PrependPath.
 func (fs *filesystem) PrependPath(ctx context.Context, vfsroot, vd vfs.VirtualDentry, b *fspath.Builder) error {
-	fs.renameMu.RLock()
-	defer fs.renameMu.RUnlock()
-	return genericPrependPath(vfsroot, vd.Mount(), vd.Dentry().Impl().(*dentry), b)
+	return genericPrependPath(fs, vfsroot, vd.Mount(), vd.Dentry().Impl().(*dentry), b)
+}
+
+// IsDescendant implements vfs.FilesystemImpl.IsDescendant.
+func (fs *filesystem) IsDescendant(vfsroot, vd vfs.VirtualDentry) bool {
+	return genericIsDescendant(fs, vfsroot.Dentry(), vd.Dentry().Impl().(*dentry))
 }
 
 type mopt struct {
@@ -1804,8 +1807,3 @@ 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))
-}

pkg/sentry/fsimpl/gofer/gofer.go

@@ -25,11 +25,12 @@
 //	        dentry.childrenMu
 //	        filesystem.syncMu
 //	        dentry.metadataMu
-//	          *** "memmap.Mappable locks" below this point
+//	          *** "memmap.Mappable/MappingIdentity locks" below this point
 //	          dentry.mapsMu
 //	            *** "memmap.Mappable locks taken by Translate" below this point
 //	            dentry.handleMu
 //	              dentry.dataMu
+//	          filesystem.ancestryMu
 //	          filesystem.inoMu
 //	specialFileFD.mu
 //	  specialFileFD.bufMu
@@ -218,6 +219,10 @@ type filesystem struct {
 	//		it is reachable from its parent).
 	renameMu sync.RWMutex `state:"nosave"`
 
+	// ancestryMu additionally protects dentry.parent and dentry.name as
+	// required by genericfstree.
+	ancestryMu sync.RWMutex `state:"nosave"`
+
 	dentryCache *dentryCache
 
 	// syncableDentries contains all non-synthetic dentries. specialFileFDs

pkg/sentry/fsimpl/gofer/lisafs_dentry.go

@@ -547,18 +547,18 @@ func (d *lisafsDentry) restoreFile(ctx context.Context, inode *lisafs.Inode, opt
 	if d.isRegularFile() {
 		if opts.ValidateFileSizes {
 			if inode.Stat.Mask&linux.STATX_SIZE == 0 {
-				return vfs.ErrCorruption{fmt.Errorf("gofer.dentry(%q).restoreFile: file size validation failed: file size not available", genericDebugPathname(&d.dentry))}
+				return vfs.ErrCorruption{fmt.Errorf("gofer.dentry(%q).restoreFile: file size validation failed: file size not available", genericDebugPathname(d.fs, &d.dentry))}
 			}
 			if d.size.RacyLoad() != inode.Stat.Size {
-				return vfs.ErrCorruption{fmt.Errorf("gofer.dentry(%q).restoreFile: file size validation failed: size changed from %d to %d", genericDebugPathname(&d.dentry), d.size.Load(), inode.Stat.Size)}
+				return vfs.ErrCorruption{fmt.Errorf("gofer.dentry(%q).restoreFile: file size validation failed: size changed from %d to %d", genericDebugPathname(d.fs, &d.dentry), d.size.Load(), inode.Stat.Size)}
 			}
 		}
 		if opts.ValidateFileModificationTimestamps {
 			if inode.Stat.Mask&linux.STATX_MTIME == 0 {
-				return vfs.ErrCorruption{fmt.Errorf("gofer.dentry(%q).restoreFile: mtime validation failed: mtime not available", genericDebugPathname(&d.dentry))}
+				return vfs.ErrCorruption{fmt.Errorf("gofer.dentry(%q).restoreFile: mtime validation failed: mtime not available", genericDebugPathname(d.fs, &d.dentry))}
 			}
 			if want := dentryTimestamp(inode.Stat.Mtime); d.mtime.RacyLoad() != want {
-				return vfs.ErrCorruption{fmt.Errorf("gofer.dentry(%q).restoreFile: mtime validation failed: mtime changed from %+v to %+v", genericDebugPathname(&d.dentry), linux.NsecToStatxTimestamp(d.mtime.RacyLoad()), linux.NsecToStatxTimestamp(want))}
+				return vfs.ErrCorruption{fmt.Errorf("gofer.dentry(%q).restoreFile: mtime validation failed: mtime changed from %+v to %+v", genericDebugPathname(d.fs, &d.dentry), linux.NsecToStatxTimestamp(d.mtime.RacyLoad()), linux.NsecToStatxTimestamp(want))}
 			}
 		}
 	}
@@ -568,7 +568,7 @@ func (d *lisafsDentry) restoreFile(ctx context.Context, inode *lisafs.Inode, opt
 
 	if rw, ok := d.fs.savedDentryRW[&d.dentry]; ok {
 		if err := d.ensureSharedHandle(ctx, rw.read, rw.write, false /* trunc */); err != nil {
-			return fmt.Errorf("failed to restore file handles (read=%t, write=%t) for %q: %w", rw.read, rw.write, genericDebugPathname(&d.dentry), err)
+			return fmt.Errorf("failed to restore file handles (read=%t, write=%t) for %q: %w", rw.read, rw.write, genericDebugPathname(d.fs, &d.dentry), err)
 		}
 	}