Fix logic bug in attaching mounts.

Before this fix, the vd provided to connectMountAt wouldn't be consumed if
we ran into a dead mount, the caller was responsible. This dead mount
could be rooted at a different vd than the one provided, which meant that we
were DecRef'ing the wrong vd if the method returned an error. The method now
always consumes the vd to simplify its use.

Reported-by: syzbot+e8a05606f54650464cc0@syzkaller.appspotmail.com
PiperOrigin-RevId: 532636615

pkg/sentry/vfs/mount.go

@@ -276,18 +276,18 @@ func (vfs *VirtualFilesystem) ConnectMountAt(ctx context.Context, creds *auth.Cr
 	tree := vfs.preparePropagationTree(mnt, vd)
 	cleanup := cleanup.Make(func() {
 		vfs.abortPropagationTree(ctx, tree) // +checklocksforce
+	})
+	defer cleanup.Clean()
+	// Check if the new mount + all the propagation mounts puts us over the max.
+	if uint32(len(tree)+1)+vd.mount.ns.mounts > MountMax {
 		// We need to unlock mountMu first because DecRef takes a lock on the
 		// filesystem mutex in some implementations, which can lead to circular
 		// locking.
 		vfs.mountMu.Unlock()
 		vd.DecRef(ctx)
-	})
-	defer cleanup.Clean()
-	// Check if the new mount + all the propagation mounts puts us over the max.
-	if uint32(len(tree)+1)+vd.mount.ns.mounts > MountMax {
 		return linuxerr.ENOSPC
 	}
-	if err := vfs.connectMountAt(ctx, mnt, vd); err != nil {
+	if err := vfs.connectMountAtLocked(ctx, mnt, vd); err != nil {
 		return err
 	}
 	vfs.commitPropagationTree(ctx, tree)
@@ -296,20 +296,20 @@ func (vfs *VirtualFilesystem) ConnectMountAt(ctx context.Context, creds *auth.Cr
 	return nil
 }
 
-// connectMountAtLocked attaches mnt at vd. If the method returns an error that
+// connectMountAtLocked attaches mnt at vd. This method consumes a reference on
-// is not nil, then it did not consume a reference on vd and the caller is
+// vd.
-// responsible for calling DecRef.
 //
 // Preconditions:
 //   - mnt must be disconnected.
 //   - vfs.mountMu must be locked.
 //
 // +checklocks:vfs.mountMu
-func (vfs *VirtualFilesystem) connectMountAt(ctx context.Context, mnt *Mount, vd VirtualDentry) error {
+func (vfs *VirtualFilesystem) connectMountAtLocked(ctx context.Context, mnt *Mount, vd VirtualDentry) error {
 	vd.dentry.mu.Lock()
 	for {
 		if vd.mount.umounted || vd.dentry.dead {
 			vd.dentry.mu.Unlock()
+			vd.DecRef(ctx)
 			return linuxerr.ENOENT
 		}
 		// vd might have been mounted over between vfs.GetDentryAt() and
@@ -408,14 +408,15 @@ func (vfs *VirtualFilesystem) BindAt(ctx context.Context, creds *auth.Credential
 		// Checklocks doesn't work with anon functions.
 		vfs.setPropagation(clone, Private)  // +checklocksforce
 		vfs.abortPropagationTree(ctx, tree) // +checklocksforce
-		vfs.mountMu.Unlock()
-		targetVd.DecRef(ctx)
 	})
 	defer cleanup.Clean()
 	if uint32(1+len(tree))+targetVd.mount.ns.mounts > MountMax {
+		vfs.mountMu.Unlock()
+		targetVd.DecRef(ctx)
 		return nil, linuxerr.ENOSPC
 	}
-	if err := vfs.connectMountAt(ctx, clone, targetVd); err != nil {
+	if err := vfs.connectMountAtLocked(ctx, clone, targetVd); err != nil {
+		vfs.mountMu.Unlock()
 		return nil, err
 	}
 	vfs.commitPropagationTree(ctx, tree)

test/syscalls/linux/mount.cc

@@ -24,6 +24,7 @@
 #include <unistd.h>
 
 #include <cstdint>
+#include <cstdlib>
 #include <functional>
 #include <iostream>
 #include <memory>
@@ -1418,6 +1419,26 @@ TEST(MountTest, MountInfoHasRoot) {
   }
 }
 
+TEST(MountTest, DeadMountsAreDecRefd) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN)));
+  DisableSave ds;
+  std::string home = NewTempAbsPath();
+  ASSERT_NO_ERRNO(Mkdir(home));
+  ASSERT_THAT(chdir(home.c_str()), SyscallSucceeds());
+  constexpr char dirpath[] = "./file";
+
+  for (int i = 0; i < 10; ++i) {
+    const auto rest = [&] {
+      mkdir(dirpath, 0);
+      mount(dirpath, ".", 0, MS_BIND, 0);
+      rmdir(dirpath);
+      mkdir(dirpath, 0);
+      mount(dirpath, ".", 0, MS_BIND, 0);
+    };
+    EXPECT_THAT(InForkedProcess(rest), IsPosixErrorOkAndHolds(0));
+  }
+}
+
 }  // namespace
 
 }  // namespace testing