Fix lock-ordering violation in Create by logging BaseName instead of FullName.

Dirent.FullName takes the global renameMu, but can be called during Create,
which itself takes dirent.mu and dirent.dirMu, which is a lock-order violation:

Dirent.Create
  d.dirMu.Lock
  d.mu.Lock
  Inode.Create
    gofer.inodeOperations.Create
      gofer.NewFile
        Dirent.FullName
          d.renameMu.RLock

We only use the FullName here for logging, and in this case we can get by with
logging only the BaseName.

A `BaseName` method was added to Dirent, which simply returns the name, taking
d.parent.mu as required.

In the Create pathway, we can't call d.BaseName() because taking d.parent.mu
after d.mu violates the lock order. But we already know the base name of the
file we just created, so that's OK.

In the Open/GetFile pathway, we are free to call d.BaseName() because the other
dirent locks are not held.

PiperOrigin-RevId: 205112278
Change-Id: Ib45c734081aecc9b225249a65fa8093eb4995f10

5 files changed, 25 insertions, 6 deletions

diff --git a/pkg/sentry/fs/dirent.go b/pkg/sentry/fs/dirent.go
index 410f93b13..5eaa2189a 100644
--- a/pkg/sentry/fs/dirent.go
+++ b/pkg/sentry/fs/dirent.go
@@ -334,6 +334,17 @@ func (d *Dirent) SyncAll(ctx context.Context) {
 	}
 }
 
+// BaseName returns the base name of the dirent.
+func (d *Dirent) BaseName() string {
+	p := d.parent
+	if p == nil {
+		return d.name
+	}
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	return d.name
+}
+
 // FullName returns the fully-qualified name and a boolean value representing
 // whether this Dirent was a descendant of root.
 // If the root argument is nil it is assumed to be the root of the Dirent tree.
diff --git a/pkg/sentry/fs/gofer/file.go b/pkg/sentry/fs/gofer/file.go
index 69cee7026..039618808 100644
--- a/pkg/sentry/fs/gofer/file.go
+++ b/pkg/sentry/fs/gofer/file.go
@@ -57,7 +57,14 @@ type fileOperations struct {
 var _ fs.FileOperations = (*fileOperations)(nil)
 
 // NewFile returns a file. NewFile is not appropriate with host pipes and sockets.
-func NewFile(ctx context.Context, dirent *fs.Dirent, flags fs.FileFlags, i *inodeOperations, handles *handles) *fs.File {
+//
+// The `name` argument is only used to log a warning if we are returning a
+// writeable+executable file. (A metric counter is incremented in this case as
+// well.) Note that we cannot call d.BaseName() directly in this function,
+// because that would lead to a lock order violation, since this is called in
+// d.Create which holds d.mu, while d.BaseName() takes d.parent.mu, and the two
+// locks must be taken in the opposite order.
+func NewFile(ctx context.Context, dirent *fs.Dirent, name string, flags fs.FileFlags, i *inodeOperations, handles *handles) *fs.File {
 	// Remote file systems enforce readability/writability at an offset,
 	// see fs/9p/vfs_inode.c:v9fs_vfs_atomic_open -> fs/open.c:finish_open.
 	flags.Pread = true
@@ -70,7 +77,6 @@ func NewFile(ctx context.Context, dirent *fs.Dirent, flags fs.FileFlags, i *inod
 	}
 	if flags.Write {
 		if err := dirent.Inode.CheckPermission(ctx, fs.PermMask{Execute: true}); err == nil {
-			name, _ := dirent.FullName(fs.RootFromContext(ctx))
 			openedWX.Increment()
 			log.Warningf("Opened a writable executable: %q", name)
 		}
diff --git a/pkg/sentry/fs/gofer/gofer_test.go b/pkg/sentry/fs/gofer/gofer_test.go
index 58a2e2ef5..3df72dd37 100644
--- a/pkg/sentry/fs/gofer/gofer_test.go
+++ b/pkg/sentry/fs/gofer/gofer_test.go
@@ -545,6 +545,7 @@ func TestPreadv(t *testing.T) {
 		f := NewFile(
 			ctx,
 			fs.NewDirent(rootInode, ""),
+			"",
 			fs.FileFlags{Read: true},
 			rootInode.InodeOperations.(*inodeOperations),
 			&handles{File: contextFile{file: openFile}},
@@ -751,6 +752,7 @@ func TestPwritev(t *testing.T) {
 		f := NewFile(
 			ctx,
 			fs.NewDirent(rootInode, ""),
+			"",
 			fs.FileFlags{Write: true},
 			rootInode.InodeOperations.(*inodeOperations),
 			&handles{File: contextFile{file: openFile}},
diff --git a/pkg/sentry/fs/gofer/inode.go b/pkg/sentry/fs/gofer/inode.go
index fa9013b75..df584c382 100644
--- a/pkg/sentry/fs/gofer/inode.go
+++ b/pkg/sentry/fs/gofer/inode.go
@@ -391,7 +391,7 @@ func (i *inodeOperations) getFilePipe(ctx context.Context, d *fs.Dirent, flags f
 	if err != nil {
 		return nil, err
 	}
-	return NewFile(ctx, d, flags, i, h), nil
+	return NewFile(ctx, d, d.BaseName(), flags, i, h), nil
 }
 
 // errNotHostFile indicates that the file is not a host file.
@@ -430,7 +430,7 @@ func (i *inodeOperations) getFileDefault(ctx context.Context, d *fs.Dirent, flag
 		if err != nil {
 			return nil, err
 		}
-		return NewFile(ctx, d, flags, i, h), nil
+		return NewFile(ctx, d, d.BaseName(), flags, i, h), nil
 	}
 
 	h, ok := i.fileState.getCachedHandles(ctx, flags, d.Inode.MountSource)
@@ -443,7 +443,7 @@ func (i *inodeOperations) getFileDefault(ctx context.Context, d *fs.Dirent, flag
 	}
 	i.fileState.setHandlesForCachedIO(flags, h)
 
-	return NewFile(ctx, d, flags, i, h), nil
+	return NewFile(ctx, d, d.BaseName(), flags, i, h), nil
 }
 
 // SetPermissions implements fs.InodeOperations.SetPermissions.
diff --git a/pkg/sentry/fs/gofer/path.go b/pkg/sentry/fs/gofer/path.go
index e78172bda..bfeab3833 100644
--- a/pkg/sentry/fs/gofer/path.go
+++ b/pkg/sentry/fs/gofer/path.go
@@ -127,7 +127,7 @@ func (i *inodeOperations) Create(ctx context.Context, dir *fs.Inode, name string
 	if iops.session().cachePolicy.usePageCache(d.Inode) {
 		iops.fileState.setHandlesForCachedIO(flags, h)
 	}
-	return NewFile(ctx, d, flags, iops, h), nil
+	return NewFile(ctx, d, name, flags, iops, h), nil
 }
 
 // CreateLink uses Create to create a symlink between oldname and newname.