From da439ae2f49775afb287f0a00e1e0023221092b9 Mon Sep 17 00:00:00 2001
From: Andrei Vagin <avagin@gmail.com>
Date: Fri, 1 Apr 2022 09:43:06 -0700
Subject: [PATCH] kernel: release FDTable lock before calling file methods

This change breaks dependency of FDTable.mu and kernfs.filesystemRWMutex.

panic: WARNING: circular locking detected: kernel.taskMutex -> kernel.fdTableMutex:

gvisor.dev/gvisor/pkg/sentry/kernel.(*fdTableMutex).Lock(0xc00491a110)
        bazel-out/k8-fastbuild-ST-1a50d7a562c6/bin/pkg/sentry/kernel/fd_table_mutex.go:16 +0x34
gvisor.dev/gvisor/pkg/sentry/kernel.(*FDTable).Fork(0xc00491a100, {0x158ef70, 0xc005fdb500}, 0x2073b60)
        pkg/sentry/kernel/fd_table.go:637 +0xca
gvisor.dev/gvisor/pkg/sentry/kernel.(*Task).Unshare(0xc005fdb500, 0x4040400)
        pkg/sentry/kernel/task_clone.go:483 +0x72d

known lock chain: kernel.fdTableMutex -> kernfs.filesystemRWMutex -> kernel.taskSetRWMutex -> kernel.signalHandlersMutex -> kernel.taskMutex

gvisor.dev/gvisor/pkg/sentry/kernel.(*FDTable).String(0xc000cd36c0)
        pkg/sentry/kernel/fd_table.go:245 +0x139

====== kernfs.filesystemRWMutex -> kernel.taskSetRWMutex =====
gvisor.dev/gvisor/pkg/sentry/kernel.(*PIDNamespace).IDOfThreadGroup(0xc00019ce40, 0xc0004a2800)
        pkg/sentry/kernel/threads.go:264 +0x35
gvisor.dev/gvisor/pkg/sentry/fsimpl/proc.(*selfSymlink).Readlink(0xc000074460, {0x158ef70, 0xc00027e000}, 0xc0003ba510)
        pkg/sentry/fsimpl/proc/tasks_files.go:58 +0x6d
gvisor.dev/gvisor/pkg/sentry/fsimpl/proc.(*selfSymlink).Getlink(0xc0004b5500, {0x158ef70, 0xc00027e000}, 0x4)
        pkg/sentry/fsimpl/proc/tasks_files.go:66 +0x2d

====== kernel.taskSetRWMutex -> kernel.signalHandlersMutex =====
gvisor.dev/gvisor/pkg/sentry/kernel.(*TaskSet).newTask(0xc00019cea0, 0xc0004f1570)
        pkg/sentry/kernel/task_start.go:177 +0x656
gvisor.dev/gvisor/pkg/sentry/kernel.(*TaskSet).NewTask(0xc00014f180, {0x158eff8, 0xc000436640}, 0xc0004f1570)
        pkg/sentry/kernel/task_start.go:122 +0xa5

====== kernel.signalHandlersMutex -> kernel.taskMutex =====
gvisor.dev/gvisor/pkg/sentry/kernel.(*TaskSet).newTask(0xc00019cea0, 0xc0004f1570)
        pkg/sentry/kernel/task_start.go:228 +0x9a8
gvisor.dev/gvisor/pkg/sentry/kernel.(*TaskSet).NewTask(0xc00014f180, {0x158eff8, 0xc000436640}, 0xc0004f1570)
        pkg/sentry/kernel/task_start.go:122 +0xa5
gvisor.dev/gvisor/pkg/sentry/kernel.(*Kernel).CreateProcess(0xc00014f180, {{0xc00039bf70, 0x5}, {0x0, 0x0}, {0xc00037d440, 0x1, 0x4}, {0xc000436620, 0x2, ...}, ...})
        pkg/sentry/kernel/kernel.go:1069 +0x147d
gvisor.dev/gvisor/runsc/boot.(*Loader).createContainerProcess(0xc0003f2000, 0x1, {0x7fff0a092fb9, 0x8}, 0xc0003f2010)
        runsc/boot/loader.go:809 +0x445
gvisor.dev/gvisor/runsc/boot.(*Loader).run(0xc0003f2000)
        runsc/boot/loader.go:630 +0x1ad
gvisor.dev/gvisor/runsc/boot.(*Loader).Run(0xc0003f2000)
        runsc/boot/loader.go:581 +0x25
---
 pkg/sentry/kernel/fd_table.go | 49 +++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/pkg/sentry/kernel/fd_table.go b/pkg/sentry/kernel/fd_table.go
index 6fc4785af..28e9fc49c 100644
--- a/pkg/sentry/kernel/fd_table.go
+++ b/pkg/sentry/kernel/fd_table.go
@@ -241,28 +241,49 @@ func (f *FDTable) forEach(ctx context.Context, fn func(fd int32, file *fs.File,
 func (f *FDTable) String() string {
 	var buf strings.Builder
 	ctx := context.Background()
+	files := make(map[int32]*fs.File)
+	filesVFS2 := make(map[int32]*vfs.FileDescription)
 	f.mu.Lock()
-	defer f.mu.Unlock()
+	// Can't release f.mu from defer, because vfsObj.PathnameWithDeleted
+	// should not be called under the fdtable mutex.
 	f.forEach(ctx, func(fd int32, file *fs.File, fileVFS2 *vfs.FileDescription, flags FDFlags) {
 		switch {
 		case file != nil:
-			n, _ := file.Dirent.FullName(nil /* root */)
-			fmt.Fprintf(&buf, "\tfd:%d => name %s\n", fd, n)
+			file.IncRef()
+			files[fd] = file
 
 		case fileVFS2 != nil:
-			vfsObj := fileVFS2.Mount().Filesystem().VirtualFilesystem()
-			vd := fileVFS2.VirtualDentry()
-			if vd.Dentry() == nil {
-				panic(fmt.Sprintf("fd %d (type %T) has nil dentry: %#v", fd, fileVFS2.Impl(), fileVFS2))
-			}
-			name, err := vfsObj.PathnameWithDeleted(ctx, vfs.VirtualDentry{}, fileVFS2.VirtualDentry())
-			if err != nil {
-				fmt.Fprintf(&buf, "<err: %v>\n", err)
-				return
-			}
-			fmt.Fprintf(&buf, "\tfd:%d => name %s\n", fd, name)
+			fileVFS2.IncRef()
+			filesVFS2[fd] = fileVFS2
 		}
 	})
+	f.mu.Unlock()
+	defer func() {
+		for _, f := range files {
+			f.DecRef(ctx)
+		}
+		for _, f := range filesVFS2 {
+			f.DecRef(ctx)
+		}
+	}()
+	for fd, file := range files {
+		n, _ := file.Dirent.FullName(nil /* root */)
+		fmt.Fprintf(&buf, "\tfd:%d => name %s\n", fd, n)
+	}
+
+	for fd, fileVFS2 := range filesVFS2 {
+		vfsObj := fileVFS2.Mount().Filesystem().VirtualFilesystem()
+		vd := fileVFS2.VirtualDentry()
+		if vd.Dentry() == nil {
+			panic(fmt.Sprintf("fd %d (type %T) has nil dentry: %#v", fd, fileVFS2.Impl(), fileVFS2))
+		}
+		name, err := vfsObj.PathnameWithDeleted(ctx, vfs.VirtualDentry{}, fileVFS2.VirtualDentry())
+		if err != nil {
+			fmt.Fprintf(&buf, "<err: %v>\n", err)
+			continue
+		}
+		fmt.Fprintf(&buf, "\tfd:%d => name %s\n", fd, name)
+	}
 	return buf.String()
 }