From ead2fa0810ea2b5d2b4b2b2e5143902615451060 Mon Sep 17 00:00:00 2001
From: Lucas Manning <lucasmanning@google.com>
Date: Wed, 9 Nov 2022 15:50:18 -0800
Subject: [PATCH] Collect the umount propagation tree before locking the mount
 table.

Unlocking the mount table while unmounting mounts was racing with other
umount calls.

This change also modifies forgetDeadMountpoints and mount namespace
DecRef to only unmount the specified mounts and not all their peers
as well. Unmounting peers should only happen through calls to
umount.

PiperOrigin-RevId: 487368800
---
 pkg/sentry/vfs/dentry.go |  4 +--
 pkg/sentry/vfs/mount.go  | 74 ++++++++++++++++------------------------
 2 files changed, 30 insertions(+), 48 deletions(-)

diff --git a/pkg/sentry/vfs/dentry.go b/pkg/sentry/vfs/dentry.go
index e99111b67..508df80b8 100644
--- a/pkg/sentry/vfs/dentry.go
+++ b/pkg/sentry/vfs/dentry.go
@@ -336,9 +336,7 @@ func (vfs *VirtualFilesystem) forgetDeadMountpoint(ctx context.Context, d *Dentr
 	vfs.mountMu.Lock()
 	vfs.mounts.seq.BeginWrite()
 	for mnt := range vfs.mountpoints[d] {
-		vds, mounts := vfs.umountAtRecursiveLocked(ctx, VirtualDentry{mnt, d}, &umountRecursiveOptions{})
-		vdsToDecRef = append(vdsToDecRef, vds...)
-		mountsToDecRef = append(mountsToDecRef, mounts...)
+		vdsToDecRef, mountsToDecRef = vfs.umountRecursiveLocked(mnt, &umountRecursiveOptions{}, vdsToDecRef, mountsToDecRef)
 	}
 	vfs.mounts.seq.EndWrite()
 	vfs.mountMu.Unlock()
diff --git a/pkg/sentry/vfs/mount.go b/pkg/sentry/vfs/mount.go
index 7ba30a66e..c6e0fe612 100644
--- a/pkg/sentry/vfs/mount.go
+++ b/pkg/sentry/vfs/mount.go
@@ -652,6 +652,23 @@ func (vfs *VirtualFilesystem) UmountAt(ctx context.Context, creds *auth.Credenti
 		}
 	}
 
+	umountTree := []*Mount{vd.mount}
+	parent, mountpoint := vd.mount.parent(), vd.mount.point()
+	if parent.propType == Shared {
+		for peer := parent.sharedList.Front(); peer != nil; peer = peer.sharedEntry.Next() {
+			if peer == parent {
+				continue
+			}
+			umountMnt := vfs.mounts.Lookup(peer, mountpoint)
+			// From https://www.kernel.org/doc/Documentation/filesystems/sharedsubtree.txt:
+			// If any peer has some child mounts, then that mount is not unmounted,
+			// but all other mounts are unmounted.
+			if umountMnt != nil && len(umountMnt.children) == 0 {
+				umountTree = append(umountTree, umountMnt)
+			}
+		}
+	}
+
 	// TODO(gvisor.dev/issue/1035): Linux special-cases umount of the caller's
 	// root, which we don't implement yet (we'll just fail it since the caller
 	// holds a reference on it).
@@ -674,10 +691,16 @@ func (vfs *VirtualFilesystem) UmountAt(ctx context.Context, creds *auth.Credenti
 			return linuxerr.EBUSY
 		}
 	}
-	vdsToDecRef, mountsToDecRef := vfs.umountAtRecursiveLocked(ctx, vd, &umountRecursiveOptions{
-		eager:               opts.Flags&linux.MNT_DETACH == 0,
-		disconnectHierarchy: true,
-	})
+	var (
+		vdsToDecRef    []VirtualDentry
+		mountsToDecRef []*Mount
+	)
+	for _, mnt := range umountTree {
+		vdsToDecRef, mountsToDecRef = vfs.umountRecursiveLocked(mnt, &umountRecursiveOptions{
+			eager:               opts.Flags&linux.MNT_DETACH == 0,
+			disconnectHierarchy: true,
+		}, vdsToDecRef, mountsToDecRef)
+	}
 	vfs.mounts.seq.EndWrite()
 	vfs.mountMu.Unlock()
 	for _, vd := range vdsToDecRef {
@@ -689,45 +712,6 @@ func (vfs *VirtualFilesystem) UmountAt(ctx context.Context, creds *auth.Credenti
 	return nil
 }
 
-// umountAtRecursiveLocked marks the mount located at vd and its decendendents
-// as umounted and does the same for mounts at the same dentry in peers of
-// vd.mount's parent.
-//
-// Preconditions:
-//   - vd is a mountpoint.
-//   - vfs.mountMu must be locked.
-//   - vfs.mounts.seq must be in a writer critical section.
-//
-// +checklocks:vfs.mountMu
-func (vfs *VirtualFilesystem) umountAtRecursiveLocked(ctx context.Context, vd VirtualDentry, opts *umountRecursiveOptions) ([]VirtualDentry, []*Mount) {
-	parent, mountpoint := vd.mount.parent(), vd.mount.point()
-	vdsToDecRef, mountsToDecRef := vfs.umountRecursiveLocked(vd.mount, opts, nil, nil)
-	if parent != nil && parent.propType == Shared {
-		for peer := parent.sharedList.Front(); peer != nil; peer = peer.sharedEntry.Next() {
-			if peer == parent {
-				continue
-			}
-
-			vfs.mounts.seq.EndWrite()
-			mnt := vfs.mounts.Lookup(peer, mountpoint)
-			vfs.mounts.seq.BeginWrite()
-			if mnt == nil {
-				continue
-			}
-
-			// From https://www.kernel.org/doc/Documentation/filesystems/sharedsubtree.txt:
-			// If any peer has some child mounts, then that mount is not unmounted,
-			// but all other mounts are unmounted.
-			if len(mnt.children) != 0 {
-				continue
-			}
-
-			vdsToDecRef, mountsToDecRef = vfs.umountRecursiveLocked(mnt, opts, vdsToDecRef, mountsToDecRef)
-		}
-	}
-	return vdsToDecRef, mountsToDecRef
-}
-
 // +stateify savable
 type umountRecursiveOptions struct {
 	// If eager is true, ensure that future calls to Mount.tryIncMountedRef()
@@ -934,9 +918,9 @@ func (mntns *MountNamespace) DecRef(ctx context.Context) {
 	mntns.MountNamespaceRefs.DecRef(func() {
 		vfs.mountMu.Lock()
 		vfs.mounts.seq.BeginWrite()
-		vdsToDecRef, mountsToDecRef := vfs.umountAtRecursiveLocked(ctx, mntns.Root(), &umountRecursiveOptions{
+		vdsToDecRef, mountsToDecRef := vfs.umountRecursiveLocked(mntns.root, &umountRecursiveOptions{
 			disconnectHierarchy: true,
-		})
+		}, nil, nil)
 		vfs.mounts.seq.EndWrite()
 		vfs.mountMu.Unlock()
 		for _, vd := range vdsToDecRef {