Fix refcount increments in gofer.filesystem.Sync.

fs.renameMu is released and reacquired in `dentry.destroyLocked()` allowing
a dentry to be in `fs.syncableDentries` with a negative reference count.

Fixes #5263

PiperOrigin-RevId: 385054337

3 files changed, 24 insertions, 25 deletions

diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go
index 14a97b468..05b776c2e 100644
--- a/pkg/sentry/fsimpl/gofer/filesystem.go
+++ b/pkg/sentry/fsimpl/gofer/filesystem.go
@@ -39,26 +39,14 @@ import (
 // Sync implements vfs.FilesystemImpl.Sync.
 func (fs *filesystem) Sync(ctx context.Context) error {
 	// Snapshot current syncable dentries and special file FDs.
-	fs.renameMu.RLock()
 	fs.syncMu.Lock()
 	ds := make([]*dentry, 0, len(fs.syncableDentries))
 	for d := range fs.syncableDentries {
-		// It's safe to use IncRef here even though fs.syncableDentries doesn't
-		// hold references since we hold fs.renameMu. Note that we can't use
-		// TryIncRef since cached dentries at zero references should still be
-		// synced.
-		d.IncRef()
 		ds = append(ds, d)
 	}
-	fs.renameMu.RUnlock()
 	sffds := make([]*specialFileFD, 0, len(fs.specialFileFDs))
 	for sffd := range fs.specialFileFDs {
-		// As above, fs.specialFileFDs doesn't hold references. However, unlike
-		// dentries, an FD that has reached zero references can't be
-		// resurrected, so we can use TryIncRef.
-		if sffd.vfsfd.TryIncRef() {
-			sffds = append(sffds, sffd)
-		}
+		sffds = append(sffds, sffd)
 	}
 	fs.syncMu.Unlock()
 
@@ -68,9 +56,7 @@ func (fs *filesystem) Sync(ctx context.Context) error {
 
 	// Sync syncable dentries.
 	for _, d := range ds {
-		err := d.syncCachedFile(ctx, true /* forFilesystemSync */)
-		d.DecRef(ctx)
-		if err != nil {
+		if err := d.syncCachedFile(ctx, true /* forFilesystemSync */); err != nil {
 			ctx.Infof("gofer.filesystem.Sync: dentry.syncCachedFile failed: %v", err)
 			if retErr == nil {
 				retErr = err
@@ -81,9 +67,7 @@ func (fs *filesystem) Sync(ctx context.Context) error {
 	// Sync special files, which may be writable but do not use dentry shared
 	// handles (so they won't be synced by the above).
 	for _, sffd := range sffds {
-		err := sffd.sync(ctx, true /* forFilesystemSync */)
-		sffd.vfsfd.DecRef(ctx)
-		if err != nil {
+		if err := sffd.sync(ctx, true /* forFilesystemSync */); err != nil {
 			ctx.Infof("gofer.filesystem.Sync: specialFileFD.sync failed: %v", err)
 			if retErr == nil {
 				retErr = err
diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go
index bcf989765..ec8d58cc9 100644
--- a/pkg/sentry/fsimpl/gofer/gofer.go
+++ b/pkg/sentry/fsimpl/gofer/gofer.go
@@ -582,10 +582,10 @@ func (fs *filesystem) Release(ctx context.Context) {
 		d.dataMu.Unlock()
 		// Close host FDs if they exist.
 		if d.readFD >= 0 {
-			unix.Close(int(d.readFD))
+			_ = unix.Close(int(d.readFD))
 		}
 		if d.writeFD >= 0 && d.readFD != d.writeFD {
-			unix.Close(int(d.writeFD))
+			_ = unix.Close(int(d.writeFD))
 		}
 		d.readFD = -1
 		d.writeFD = -1
@@ -1637,18 +1637,18 @@ func (d *dentry) destroyLocked(ctx context.Context) {
 	d.dataMu.Unlock()
 	// Clunk open fids and close open host FDs.
 	if !d.readFile.isNil() {
-		d.readFile.close(ctx)
+		_ = d.readFile.close(ctx)
 	}
 	if !d.writeFile.isNil() && d.readFile != d.writeFile {
-		d.writeFile.close(ctx)
+		_ = d.writeFile.close(ctx)
 	}
 	d.readFile = p9file{}
 	d.writeFile = p9file{}
 	if d.readFD >= 0 {
-		unix.Close(int(d.readFD))
+		_ = unix.Close(int(d.readFD))
 	}
 	if d.writeFD >= 0 && d.readFD != d.writeFD {
-		unix.Close(int(d.writeFD))
+		_ = unix.Close(int(d.writeFD))
 	}
 	d.readFD = -1
 	d.writeFD = -1
diff --git a/pkg/sentry/fsimpl/gofer/special_file.go b/pkg/sentry/fsimpl/gofer/special_file.go
index 29afb67d9..4b59c1c3c 100644
--- a/pkg/sentry/fsimpl/gofer/special_file.go
+++ b/pkg/sentry/fsimpl/gofer/special_file.go
@@ -42,6 +42,11 @@ import (
 type specialFileFD struct {
 	fileDescription
 
+	// releaseMu synchronizes the closing of fd.handle with fd.sync(). It's safe
+	// to access fd.handle without locking for operations that require a ref to
+	// be held by the caller, e.g. vfs.FileDescriptionImpl implementations.
+	releaseMu sync.RWMutex `state:"nosave"`
+
 	// handle is used for file I/O. handle is immutable.
 	handle handle `state:"nosave"`
 
@@ -117,7 +122,10 @@ func (fd *specialFileFD) Release(ctx context.Context) {
 	if fd.haveQueue {
 		fdnotifier.RemoveFD(fd.handle.fd)
 	}
+	fd.releaseMu.Lock()
 	fd.handle.close(ctx)
+	fd.releaseMu.Unlock()
+
 	fs := fd.vfsfd.Mount().Filesystem().Impl().(*filesystem)
 	fs.syncMu.Lock()
 	delete(fs.specialFileFDs, fd)
@@ -373,6 +381,13 @@ func (fd *specialFileFD) Sync(ctx context.Context) error {
 }
 
 func (fd *specialFileFD) sync(ctx context.Context, forFilesystemSync bool) error {
+	// Locks to ensure it didn't race with fd.Release().
+	fd.releaseMu.RLock()
+	defer fd.releaseMu.RUnlock()
+
+	if !fd.handle.isOpen() {
+		return nil
+	}
 	err := func() error {
 		// If we have a host FD, fsyncing it is likely to be faster than an fsync
 		// RPC.