From af80b6898e9b442458f3c13c8d474d7d3f65f810 Mon Sep 17 00:00:00 2001
From: Ayush Ranjan <ayushranjan@google.com>
Date: Mon, 8 Jan 2024 19:45:40 -0800
Subject: [PATCH] Close directfs dentry's control FD on gofer filesystem
 Release().

Otherwise we will leak these control FDs and the sandbox will continue holding
on to it. Note that the dentry cache is not destroyed on Release(). This can
prevent the container filesystems from being unmounted in multi-container
scenario due to EBUSY errors.

Updates #9834

Investigated-by: Andrei Vagin <avagin@google.com>
PiperOrigin-RevId: 596770126
---
 pkg/sentry/fsimpl/gofer/dentry_impl.go     | 26 ++++++++++++++++++++++
 pkg/sentry/fsimpl/gofer/directfs_dentry.go |  5 ++++-
 pkg/sentry/fsimpl/gofer/gofer.go           | 13 ++---------
 3 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/pkg/sentry/fsimpl/gofer/dentry_impl.go b/pkg/sentry/fsimpl/gofer/dentry_impl.go
index 78d54f65f..9c97667e4 100644
--- a/pkg/sentry/fsimpl/gofer/dentry_impl.go
+++ b/pkg/sentry/fsimpl/gofer/dentry_impl.go
@@ -17,6 +17,7 @@ package gofer
 import (
 	"golang.org/x/sys/unix"
 	"gvisor.dev/gvisor/pkg/abi/linux"
+	"gvisor.dev/gvisor/pkg/atomicbitops"
 	"gvisor.dev/gvisor/pkg/context"
 	"gvisor.dev/gvisor/pkg/errors/linuxerr"
 	"gvisor.dev/gvisor/pkg/fsutil"
@@ -147,6 +148,30 @@ func (d *dentry) updateHandles(ctx context.Context, h handle, readable, writable
 	}
 }
 
+// Preconditions:
+//   - d.handleMu must be locked.
+//   - !d.isSynthetic().
+func (d *dentry) closeHostFDs() {
+	// We can use RacyLoad() because d.handleMu is locked.
+	if d.readFD.RacyLoad() >= 0 {
+		_ = unix.Close(int(d.readFD.RacyLoad()))
+	}
+	if d.writeFD.RacyLoad() >= 0 && d.readFD.RacyLoad() != d.writeFD.RacyLoad() {
+		_ = unix.Close(int(d.writeFD.RacyLoad()))
+	}
+	d.readFD = atomicbitops.FromInt32(-1)
+	d.writeFD = atomicbitops.FromInt32(-1)
+	d.mmapFD = atomicbitops.FromInt32(-1)
+
+	switch dt := d.impl.(type) {
+	case *directfsDentry:
+		if dt.controlFD >= 0 {
+			_ = unix.Close(dt.controlFD)
+			dt.controlFD = -1
+		}
+	}
+}
+
 // updateMetadataLocked updates the dentry's metadata fields. The h parameter
 // is optional. If it is not provided, an appropriate FD should be chosen to
 // stat the remote file.
@@ -213,6 +238,7 @@ func (d *dentry) setStatLocked(ctx context.Context, stat *linux.Statx) (uint32,
 	}
 }
 
+// Precondition: d.handleMu must be locked.
 func (d *dentry) destroyImpl(ctx context.Context) {
 	switch dt := d.impl.(type) {
 	case *lisafsDentry:
diff --git a/pkg/sentry/fsimpl/gofer/directfs_dentry.go b/pkg/sentry/fsimpl/gofer/directfs_dentry.go
index bef7ea8d6..7b5c7518c 100644
--- a/pkg/sentry/fsimpl/gofer/directfs_dentry.go
+++ b/pkg/sentry/fsimpl/gofer/directfs_dentry.go
@@ -91,7 +91,8 @@ func (fs *filesystem) getDirectfsRootDentry(ctx context.Context, rootHostFD int,
 type directfsDentry struct {
 	dentry
 
-	// controlFD is the host FD to this file. controlFD is immutable.
+	// controlFD is the host FD to this file. controlFD is immutable until
+	// destruction, which is synchronized with dentry.handleMu.
 	controlFD int
 
 	// controlFDLisa is a lisafs control FD on this dentry.
@@ -401,9 +402,11 @@ func fchown(fd, uid, gid int) error {
 	return unix.Fchownat(fd, "", uid, gid, unix.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW)
 }
 
+// Precondition: d.handleMu must be locked.
 func (d *directfsDentry) destroy(ctx context.Context) {
 	if d.controlFD >= 0 {
 		_ = unix.Close(d.controlFD)
+		d.controlFD = -1
 	}
 	if d.controlFDLisa.Ok() {
 		d.controlFDLisa.Close(ctx, true /* flush */)
diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go
index 5a5e9aa29..93fdbd333 100644
--- a/pkg/sentry/fsimpl/gofer/gofer.go
+++ b/pkg/sentry/fsimpl/gofer/gofer.go
@@ -678,17 +678,8 @@ func (fs *filesystem) Release(ctx context.Context) {
 		d.cache.DropAll(mf)
 		d.dirty.RemoveAll()
 		d.dataMu.Unlock()
-		// Close host FDs if they exist. We can use RacyLoad() because d.handleMu
-		// is locked.
-		if d.readFD.RacyLoad() >= 0 {
-			_ = unix.Close(int(d.readFD.RacyLoad()))
-		}
-		if d.writeFD.RacyLoad() >= 0 && d.readFD.RacyLoad() != d.writeFD.RacyLoad() {
-			_ = unix.Close(int(d.writeFD.RacyLoad()))
-		}
-		d.readFD = atomicbitops.FromInt32(-1)
-		d.writeFD = atomicbitops.FromInt32(-1)
-		d.mmapFD = atomicbitops.FromInt32(-1)
+		// Close host FDs if they exist.
+		d.closeHostFDs()
 		d.handleMu.Unlock()
 	}
 	// There can't be any specialFileFDs still using fs, since each such