Establish lock ordering for vfs.FilesystemImpl.PrependPath.

- Add fspath.Builder.Unprepend(), and make Builder.Prepend*() return the number
  of bytes prepended to make the former usable. This allows
  genericfstree.PrependPath() to discard prepended bytes and retry when a race
  with rename is detected.

- Add type parameter Filesystem to vfs/genericfstree, which is required to
  provide `renameSeq sync.SeqCount`.

- Modify genericfstree.PrependPath() and genericfstree.IsDescendant() to use
  renameSeq to ensure atomicity. For callers of genericfstree.PrependPath(),
  this means that 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

pkg/fspath/builder.go

@@ -67,31 +67,54 @@ func (b *Builder) grow(n int) {
 }
 
 // PrependComponent prepends the given path component to b's buffer. A path
-// separator is automatically inserted if appropriate.
-func (b *Builder) PrependComponent(pc string) {
+// separator is automatically inserted if appropriate. PrependComponent returns
+// the number of bytes prepended.
+func (b *Builder) PrependComponent(pc string) int {
+	n := 0
 	if b.needSep {
-		b.PrependByte('/')
+		n += b.PrependByte('/')
 	}
-	b.PrependString(pc)
+	n += b.PrependString(pc)
 	b.needSep = true
+	return n
 }
 
-// PrependString prepends the given string to b's buffer.
-func (b *Builder) PrependString(str string) {
+// PrependString prepends the given string to b's buffer. It returns the number
+// of bytes prepended.
+func (b *Builder) PrependString(str string) int {
 	if b.needToGrow(len(str)) {
 		b.grow(len(str))
 	}
 	b.start -= len(str)
 	copy(b.buf[b.start:], str)
+	return len(str)
 }
 
-// PrependByte prepends the given byte to b's buffer.
-func (b *Builder) PrependByte(c byte) {
+// PrependByte prepends the given byte to b's buffer. It returns the number of
+// bytes prepended, which is always 1.
+func (b *Builder) PrependByte(c byte) int {
 	if b.needToGrow(1) {
 		b.grow(1)
 	}
 	b.start--
 	b.buf[b.start] = c
+	return 1
+}
+
+// Unprepend discards the last n prepended bytes.
+func (b *Builder) Unprepend(n int) {
+	if n < 0 || n > b.Len() {
+		panic("invalid n")
+	}
+	if n == 0 {
+		return
+	}
+	b.start += n
+	if b.buf[b.start-1] == '/' {
+		b.needSep = true
+	} else {
+		b.needSep = false
+	}
 }
 
 // AppendString appends the given string to b's buffer.

pkg/fspath/builder_test.go

@@ -15,6 +15,7 @@
 package fspath
 
 import (
+	"fmt"
 	"testing"
 )
 
@@ -51,7 +52,106 @@ func TestBuilder(t *testing.T) {
 			}
 			b.AppendString(test.after)
 			if got := b.String(); got != test.want {
-				t.Errorf("got %q, wanted %q", got, test.want)
+				t.Errorf("got %q, want %q", got, test.want)
+			}
+		})
+	}
+}
+
+func TestBuilderUnprepend(t *testing.T) {
+	type testCase struct {
+		name       string
+		pcsInitial []string
+		strInitial string
+		pcsTemp    []string
+		strTemp    string
+		pcsFinal   []string
+		strFinal   string
+	}
+	var tests []testCase
+	for numInitial := range 3 {
+		var pcsInitial []string
+		var strInitial string
+		for i := range numInitial {
+			pc := fmt.Sprintf("init%d", i)
+			pcsInitial = append(pcsInitial, pc)
+			if len(strInitial) == 0 {
+				strInitial = pc
+			} else {
+				strInitial = pc + "/" + strInitial
+			}
+		}
+		for numTemp := range 3 {
+			var pcsTemp []string
+			var strTempPrefix string
+			for i := range numTemp {
+				pc := fmt.Sprintf("temp%d", i)
+				pcsTemp = append(pcsTemp, pc)
+				if len(strTempPrefix) == 0 {
+					strTempPrefix = pc
+					if len(strInitial) != 0 {
+						strTempPrefix = strTempPrefix + "/"
+					}
+				} else {
+					strTempPrefix = pc + "/" + strTempPrefix
+				}
+			}
+			for numFinal := range 3 {
+				var pcsFinal []string
+				var strFinalPrefix string
+				for i := range numFinal {
+					pc := fmt.Sprintf("final%d", i)
+					pcsFinal = append(pcsFinal, pc)
+					if len(strFinalPrefix) == 0 {
+						strFinalPrefix = pc
+						if len(strInitial) != 0 {
+							strFinalPrefix = strFinalPrefix + "/"
+						}
+					} else {
+						strFinalPrefix = pc + "/" + strFinalPrefix
+					}
+				}
+				tests = append(tests, testCase{
+					name:       fmt.Sprintf("Initial%d_Temp%d_Final%d", numInitial, numTemp, numFinal),
+					pcsInitial: pcsInitial,
+					strInitial: strInitial,
+					pcsTemp:    pcsTemp,
+					strTemp:    strTempPrefix + strInitial,
+					pcsFinal:   pcsFinal,
+					strFinal:   strFinalPrefix + strInitial,
+				})
+			}
+		}
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			// Since these test cases are automatically generated, log them so
+			// they can at least be manually inspected for correctness.
+			t.Logf("testCase=%+v", test)
+			var b Builder
+			for _, pc := range test.pcsInitial {
+				b.PrependComponent(pc)
+			}
+			if got, want := b.String(), test.strInitial; got != want {
+				t.Errorf("after initial insertions: got %q, want %q", got, want)
+			}
+			n := 0
+			for _, pc := range test.pcsTemp {
+				n += b.PrependComponent(pc)
+			}
+			if got, want := b.String(), test.strTemp; got != want {
+				t.Errorf("after temp insertion of %d bytes: got %q, want %q", n, got, want)
+			}
+			b.Unprepend(n)
+			if got, want := b.String(), test.strInitial; got != want {
+				t.Errorf("after removal of %d temp bytes: got %q, want %q", n, got, want)
+			}
+			for _, pc := range test.pcsFinal {
+				b.PrependComponent(pc)
+			}
+			if got, want := b.String(), test.strFinal; got != want {
+				t.Errorf("after final insertions: got %q, want %q", got, want)
 			}
 		})
 	}

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
+
+	// renameSeq is required by genericfstree.
+	renameSeq sync.SeqCount `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 {