Refactor the umount algorithm.

The basic recursive approach does not account for propagating the children's
umount events. There are other missed corner cases like propagating a umount
to a mount that has children. Before this change it would be ignored
unconditionally even if the child was one of the mounts that is going to be
recursively unmounted later in the method.

This change also adds support for a propagation option that controls whether
the umount should be propagated to peers/followers. This is set to false in
the case of forgetting dead mount points and destroying mount namespaces.

PiperOrigin-RevId: 582452574

pkg/sentry/vfs/dentry.go

@@ -337,10 +337,8 @@ func (vfs *VirtualFilesystem) CommitRenameExchangeDentry(from, to *Dentry) {
 func (vfs *VirtualFilesystem) forgetDeadMountpoint(ctx context.Context, d *Dentry) []refs.RefCounter {
 	vfs.lockMounts()
 	defer vfs.unlockMounts(ctx)
-	vfs.mounts.seq.BeginWrite()
 	for mnt := range vfs.mountpoints[d] {
-		vfs.umountRecursiveLocked(mnt, &umountRecursiveOptions{})
+		vfs.umountTreeLocked(mnt, &umountRecursiveOptions{})
 	}
-	vfs.mounts.seq.EndWrite()
 	return vfs.PopDelayedDecRefs()
 }

pkg/sentry/vfs/mount.go

@@ -594,28 +594,11 @@ func (vfs *VirtualFilesystem) UmountAt(ctx context.Context, creds *auth.Credenti
 	// 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).
-
-	propMounts := []*Mount{vd.mount}
-	if vd.mount.parent() != nil {
-		for m := nextPropMount(vd.mount.parent(), vd.mount.parent()); m != nil; m = nextPropMount(m, vd.mount.parent()) {
-			child := vfs.mounts.Lookup(m, vd.mount.point())
-			if child == nil {
-				continue
-			}
-			if len(child.children) != 0 && child.coveringMount() == nil {
-				continue
-			}
-			propMounts = append(propMounts, child)
-		}
-	}
-	vfs.mounts.seq.BeginWrite()
-	for _, m := range propMounts {
-		vfs.umountRecursiveLocked(m, &umountRecursiveOptions{
-			eager:               opts.Flags&linux.MNT_DETACH == 0,
-			disconnectHierarchy: true,
-		})
-	}
-	vfs.mounts.seq.EndWrite()
+	vfs.umountTreeLocked(vd.mount, &umountRecursiveOptions{
+		eager:               opts.Flags&linux.MNT_DETACH == 0,
+		disconnectHierarchy: true,
+		propagate:           true,
+	})
 	return nil
 }
 
@@ -650,59 +633,81 @@ type umountRecursiveOptions struct {
 	//
 	// disconnectHierarchy is analogous to Linux's !UMOUNT_CONNECTED.
 	disconnectHierarchy bool
+
+	// If propagate is true, mounts located at the same point on the mount's
+	// parent's peers and follows will also be umounted if they do not have any
+	// children.
+	//
+	// propagate is analogous to Linux's UMOUNT_PROPAGATE.
+	propagate bool
 }
 
-// umountRecursiveLocked marks mnt and its descendants as umounted.
-//
-// umountRecursiveLocked is analogous to Linux's fs/namespace.c:umount_tree().
-//
-// Preconditions:
-//   - vfs.mountMu must be locked.
-//   - vfs.mounts.seq must be in a writer critical section.
+// umountTreeLocked marks mnt and its descendants as umounted.
 //
+// umountTreeLocked is analogous to Linux's fs/namespace.c:umount_tree().
 // +checklocks:vfs.mountMu
-func (vfs *VirtualFilesystem) umountRecursiveLocked(mnt *Mount, opts *umountRecursiveOptions) {
-	// covered mounts are a special case where the grandchild mount is
-	// reconnected to the parent after the child is disconnected.
-	var cover *Mount
-	if parent := mnt.parent(); parent != nil && !parent.umounted {
-		if cover = mnt.coveringMount(); cover != nil {
-			vfs.delayDecRef(vfs.disconnectLocked(cover))
-			cover.setKey(mnt.getKey())
-		}
+func (vfs *VirtualFilesystem) umountTreeLocked(mnt *Mount, opts *umountRecursiveOptions) {
+	submounts := mnt.submountsLocked()
+	var umountMnts []*Mount
+	if opts.disconnectHierarchy {
+		umountMnts = submounts
+	} else {
+		umountMnts = []*Mount{mnt}
 	}
-	if !mnt.umounted {
-		mnt.umounted = true
+
+	for _, mnt := range umountMnts {
+		vfs.umount(mnt)
+	}
+	if opts.propagate {
+		umountMnts = append(umountMnts, vfs.propagateUmount(umountMnts)...)
+	}
+
+	vfs.mounts.seq.BeginWrite()
+	for _, mnt := range umountMnts {
 		vfs.delayDecRef(mnt)
-		if parent := mnt.parent(); parent != nil && (opts.disconnectHierarchy || !parent.umounted) {
+		if parent := mnt.parent(); parent != nil {
 			vfs.delayDecRef(vfs.disconnectLocked(mnt))
 		}
 		vfs.setPropagation(mnt, linux.MS_PRIVATE)
 	}
+	vfs.mounts.seq.EndWrite()
+
 	if opts.eager {
-		for {
-			refs := mnt.refs.Load()
-			if refs < 0 {
-				break
-			}
-			if mnt.refs.CompareAndSwap(refs, refs|math.MinInt64) {
-				break
+		for _, mnt := range submounts {
+			for {
+				refs := mnt.refs.Load()
+				if refs < 0 {
+					break
+				}
+				if mnt.refs.CompareAndSwap(refs, refs|math.MinInt64) {
+					break
+				}
 			}
 		}
 	}
-	for child := range mnt.children {
-		vfs.umountRecursiveLocked(child, opts)
-	}
-	if cover != nil {
-		mp := cover.getKey()
-		mp.IncRef()
-		mp.dentry.mu.Lock()
-		vfs.connectLocked(cover, mp, mp.mount.ns)
-		mp.dentry.mu.Unlock()
-		vfs.delayDecRef(cover)
+}
+
+// +checklocks:vfs.mountMu
+func (vfs *VirtualFilesystem) umount(mnt *Mount) {
+	mnt.umounted = true
+	if parent := mnt.parent(); parent != nil {
+		delete(parent.children, mnt)
 	}
 }
 
+// changeMountpoint disconnects mnt from its current mount point and connects
+// it to mp. It must be called from a vfs.mounts.seq writer critical section.
+//
+// +checklocks:vfs.mountMu
+func (vfs *VirtualFilesystem) changeMountpoint(mnt *Mount, mp VirtualDentry) {
+	mp.dentry.mu.Lock()
+	vfs.delayDecRef(vfs.disconnectLocked(mnt))
+	vfs.delayDecRef(mnt)
+	mp.IncRef()
+	vfs.connectLocked(mnt, mp, mp.mount.ns)
+	mp.dentry.mu.Unlock()
+}
+
 // connectLocked makes vd the mount parent/point for mnt. It consumes
 // references held by vd.
 //

pkg/sentry/vfs/namespace.go

@@ -195,11 +195,9 @@ func (vfs *VirtualFilesystem) CloneMountNamespace(
 func (mntns *MountNamespace) Destroy(ctx context.Context) {
 	vfs := mntns.root.fs.VirtualFilesystem()
 	vfs.lockMounts()
-	vfs.mounts.seq.BeginWrite()
-	vfs.umountRecursiveLocked(mntns.root, &umountRecursiveOptions{
+	vfs.umountTreeLocked(mntns.root, &umountRecursiveOptions{
 		disconnectHierarchy: true,
 	})
-	vfs.mounts.seq.EndWrite()
 	vfs.unlockMounts(ctx)
 }
 

pkg/sentry/vfs/propagation.go

@@ -503,6 +503,126 @@ func (vfs *VirtualFilesystem) allocMountGroupIDs(mnt *Mount, recursive bool) err
 	return nil
 }
 
+// propagateUmount returns a list of mounts that the umount of mnts propagates
+// to.
+//
+// Prerequisites: all the mounts in mnts have had vfs.umount() called on them.
+//
+// +checklocks:vfs.mountMu
+func (vfs *VirtualFilesystem) propagateUmount(mnts []*Mount) []*Mount {
+	const (
+		umountVisited = iota
+		umountRestore
+	)
+	var toUmount []*Mount
+	// Processed contains all the mounts that the algorithm has processed so far.
+	// If the mount maps to umountRestore, it should be restored after processing
+	// all the mounts. This happens in cases where a mount was speculatively
+	// unmounted that had children or is a cover mount.
+	processed := make(map[*Mount]int)
+
+	// Iterate through the mounts from the leafs back to the root.
+	for i := len(mnts) - 1; i >= 0; i-- {
+		mnt := mnts[i]
+
+		// If a mount has already been visited we know all its peers and followers
+		// have been visited so there's no need to visit them again.
+		if _, ok := processed[mnt]; ok {
+			continue
+		}
+		processed[mnt] = umountVisited
+
+		parent := mnt.parent()
+		if parent == nil {
+			continue
+		}
+		for m := nextPropMount(parent, parent); m != nil; m = nextPropMount(m, parent) {
+			child := vfs.mounts.Lookup(m, mnt.point())
+			if child == nil {
+				continue
+			}
+			if _, ok := processed[child]; ok {
+				// If the child has been visited we know its peer group and followers
+				// have all been visited so there's no need to visit them again. We can
+				// skip this propagation subtree by setting the iterator to be the last
+				// mount in the follower group.
+				if !child.followerList.Empty() {
+					m = child.followerList.Back()
+				}
+				continue
+			} else if child.umounted {
+				// If this child has already been marked for unmounting, just mark it
+				// as visited and move on. This means it was either part of the original
+				// mount list passed to this method or was umounted from another mount's
+				// propagation. In either case we can consider all its peers and
+				// followers as visited.
+				processed[child] = umountVisited
+				continue
+			}
+
+			// This loop starts at the child we are propagating the umount to and
+			// iterates through the child's parents. It continues as until it
+			// encounters a parent that's been visited.
+		loop:
+			for {
+				if child.umounted {
+					break
+				}
+				// If there are any children that have mountpoint != parent's root then
+				// the current mount cannot be unmounted.
+				for gchild := range child.children {
+					if gchild.point() == child.root {
+						continue
+					}
+					processed[child] = umountRestore
+					break loop
+				}
+				vfs.umount(child)
+				toUmount = append(toUmount, child)
+				child = child.parent()
+				// If this parent was a mount that had to be restored because it had
+				// children, it might be safe to umount now that its child is gone. If
+				// it has been visited then it's already being umounted.
+				if _, ok := processed[child]; !ok {
+					break
+				}
+			}
+		}
+	}
+
+	// Add all the children of mounts marked for umount to the umount list. This
+	// excludes "cover" mounts (mounts whose mount point is equal to their
+	// parent's root) which will be reparented in the next step.
+	for i := 0; i < len(toUmount); i++ {
+		umount := toUmount[i]
+		for child := range umount.children {
+			if child.point() == umount.root {
+				processed[child] = umountRestore
+			} else {
+				vfs.umount(child)
+				toUmount = append(toUmount, child)
+			}
+		}
+	}
+
+	vfs.mounts.seq.BeginWrite()
+	for m, status := range processed {
+		if status == umountVisited {
+			continue
+		}
+		mp := m.getKey()
+		for mp.mount.umounted {
+			mp = mp.mount.getKey()
+		}
+		if mp != m.getKey() {
+			vfs.changeMountpoint(m, mp)
+		}
+	}
+	vfs.mounts.seq.EndWrite()
+
+	return toUmount
+}
+
 // peerUnderRoot iterates through mnt's peers until it finds a mount that is in
 // ns and is reachable from root. This method is analogous to
 // fs/pnode.c:get_peer_under_root() in Linux.

test/syscalls/linux/mount.cc

@@ -1201,6 +1201,32 @@ TEST(MountTest, PropagateUmountEvent) {
   }
 }
 
+TEST(MountTest, PropagateChildUmountEvent) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN)));
+
+  TempPath const dir1 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+  ASSERT_THAT(mount("", dir1.path().c_str(), kTmpfs, 0, ""), SyscallSucceeds());
+
+  TempPath const child =
+      ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(dir1.path()));
+  ASSERT_THAT(mount("", child.path().c_str(), kTmpfs, 0, ""),
+              SyscallSucceeds());
+  ASSERT_THAT(mount("", child.path().c_str(), "", MS_SHARED, 0),
+              SyscallSucceeds());
+
+  TempPath const dir2 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+  ASSERT_THAT(mount(child.path().c_str(), dir2.path().c_str(), "", MS_BIND, 0),
+              SyscallSucceeds());
+  ASSERT_THAT(mount("", child.path().c_str(), kTmpfs, 0, ""),
+              SyscallSucceeds());
+  ASSERT_THAT(umount2(dir1.path().c_str(), MNT_DETACH), SyscallSucceeds());
+
+  auto optionals = ASSERT_NO_ERRNO_AND_VALUE(MountOptionals());
+  EXPECT_EQ(optionals[dir2.path()].size(), 1);
+
+  ASSERT_EQ(umount2(dir2.path().c_str(), MNT_DETACH), 0);
+}
+
 TEST(MountTest, UmountIgnoresPeersWithChildren) {
   SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN)));
   auto const dir1 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());