Fix mount table corruption.

Before this, mount propagation could cause multiple mounts to have the same
parent and mountpoint, violating the mountTable contract. Now, when there's
an existing mount at a propagation point, it's added as a child of the new
mount.

PiperOrigin-RevId: 566738099

pkg/sentry/vfs/mount.go

@@ -150,6 +150,24 @@ func (mnt *Mount) generateOptionalTags() string {
 	return optional
 }
 
+// coveringMount returns a mount that completely covers mnt if it exists and nil
+// otherwise. A mount that covers another is one that is the only child of its
+// parent and whose mountpoint is its parent's root.
+func (mnt *Mount) coveringMount() *Mount {
+	if len(mnt.children) != 1 {
+		return nil
+	}
+	// Get the child from the children map.
+	var child *Mount
+	for child = range mnt.children {
+		break
+	}
+	if child.point() != mnt.root {
+		return nil
+	}
+	return child
+}
+
 // NewFilesystem creates a new filesystem object not yet associated with any
 // mounts. It can be installed into the filesystem tree with ConnectMountAt.
 // Note that only the filesystem-specific mount options from opts are used by
@@ -419,7 +437,10 @@ func (vfs *VirtualFilesystem) UmountAt(ctx context.Context, creds *auth.Credenti
 			// 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 {
+			if umountMnt == nil {
+				continue
+			}
+			if len(umountMnt.children) == 0 || umountMnt.coveringMount() != nil {
 				umountTree = append(umountTree, umountMnt)
 			}
 		}
@@ -483,6 +504,15 @@ type umountRecursiveOptions struct {
 //
 // +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())
+		}
+	}
 	if !mnt.umounted {
 		mnt.umounted = true
 		vfs.delayDecRef(mnt)
@@ -507,6 +537,14 @@ func (vfs *VirtualFilesystem) umountRecursiveLocked(mnt *Mount, opts *umountRecu
 	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)
+	}
 }
 
 // connectLocked makes vd the mount parent/point for mnt. It consumes

pkg/sentry/vfs/propagation.go

@@ -128,17 +128,27 @@ func (vfs *VirtualFilesystem) preparePropagationTree(mnt *Mount, vd VirtualDentr
 func (vfs *VirtualFilesystem) commitPropagationTree(ctx context.Context, tree map[*Mount]VirtualDentry) {
 	// The peer mounts should have no way of being dead if we've reached this
 	// point so its safe to connect without checks.
-	vfs.mounts.seq.BeginWrite()
 	for mnt, vd := range tree {
+		// If there is already a mount at this (parent, point), disconnect it and
+		// reconnect it to the new mount once it is connected.
 		vd.dentry.mu.Lock()
-		// If mnt isn't connected yet, skip connecting during propagation.
-		if mntns := vd.mount.ns; mntns != nil {
-			vfs.connectLocked(mnt, vd, mntns)
+		child := vfs.mounts.Lookup(vd.mount, vd.dentry)
+		vfs.mounts.seq.BeginWrite()
+		if child != nil {
+			vfs.delayDecRef(vfs.disconnectLocked(child))
 		}
-		vd.dentry.mu.Unlock()
+		vfs.connectLocked(mnt, vd, vd.mount.ns)
 		vfs.delayDecRef(mnt)
+
+		if child != nil {
+			newmp := VirtualDentry{mnt, mnt.root}
+			newmp.IncRef()
+			vfs.connectLocked(child, newmp, newmp.mount.ns)
+			vfs.delayDecRef(child)
+		}
+		vfs.mounts.seq.EndWrite()
+		vd.dentry.mu.Unlock()
 	}
-	vfs.mounts.seq.EndWrite()
 }
 
 // abortPropagationTree releases any references held by the mounts and

test/syscalls/linux/mount.cc

@@ -899,6 +899,68 @@ TEST(MountTest, MaxMounts) {
   umount2(parent.path().c_str(), MNT_DETACH);
 }
 
+TEST(MountTest, PropagateToSameMountpointStacksMounts) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN)));
+  auto const dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+  auto const dir2 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+  auto const mnt = ASSERT_NO_ERRNO_AND_VALUE(Mount(
+      dir.path().c_str(), dir.path().c_str(), "", MS_BIND, "", MNT_DETACH));
+  auto const child =
+      ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(dir.path()));
+  ASSERT_THAT(mount(child.path().c_str(), child.path().c_str(), "", MS_BIND, 0),
+              SyscallSucceeds());
+  ASSERT_THAT(mount("", dir.path().c_str(), "", MS_SHARED, 0),
+              SyscallSucceeds());
+  auto const mnt2 = ASSERT_NO_ERRNO_AND_VALUE(Mount(
+      dir.path().c_str(), dir2.path().c_str(), "", MS_BIND, "", MNT_DETACH));
+
+  std::string dir2_child_path = JoinPath(dir2.path(), Basename(child.path()));
+  ASSERT_THAT(
+      mount(dir2_child_path.c_str(), dir2_child_path.c_str(), "", MS_BIND, 0),
+      SyscallSucceeds());
+
+  // Check that mounts at the child mount point have distinct parents.
+  std::vector<ProcMountInfoEntry> mounts =
+      ASSERT_NO_ERRNO_AND_VALUE(ProcSelfMountInfoEntries());
+  uint64_t parent_id = 0;
+  for (auto& minfo : mounts) {
+    if (minfo.mount_point == child.path()) {
+      if (parent_id == 0) {
+        parent_id = minfo.parent_id;
+      } else {
+        EXPECT_NE(parent_id, minfo.parent_id);
+      }
+    }
+  }
+}
+
+TEST(MountTest, UmountReparentsCoveredMounts) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN)));
+  auto const dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+  auto const dir2 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+  auto const mnt = ASSERT_NO_ERRNO_AND_VALUE(
+      Mount("", dir.path().c_str(), "tmpfs", 0, "", MNT_DETACH));
+  ASSERT_THAT(mount("", dir.path().c_str(), "", MS_SHARED, 0),
+              SyscallSucceeds());
+  auto const child =
+      ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(dir.path()));
+  ASSERT_THAT(mount("", child.path().c_str(), "tmpfs", 0, 0),
+              SyscallSucceeds());
+  auto const mnt2 = ASSERT_NO_ERRNO_AND_VALUE(Mount(
+      dir.path().c_str(), dir2.path().c_str(), "", MS_BIND, "", MNT_DETACH));
+
+  std::string dir2_child_path = JoinPath(dir2.path(), Basename(child.path()));
+  ASSERT_THAT(mount("", dir2_child_path.c_str(), "tmpfs", 0, 0),
+              SyscallSucceeds());
+
+  umount2(dir2_child_path.c_str(), MNT_DETACH);
+
+  auto optionals = ASSERT_NO_ERRNO_AND_VALUE(MountOptionals());
+  ASSERT_FALSE(optionals[child.path()].empty());
+  EXPECT_NE(optionals[child.path()][0].shared, 0);
+  EXPECT_TRUE(optionals[dir2_child_path].empty());
+}
+
 // Tests that it is possible to make a shared mount.
 TEST(MountTest, MakeShared) {
   SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN)));