Fix group id cleanup and tidy up some mount methods.

Before this we were failing to clean up group IDs properly which could
cause propagation to loop forever in some rare cases. This change
also addresses a couple other small issues:

- In attachTreeLocked we were doing error checks on a VirtualDentry before
  checking if that vd had been mounted over.
- In destroy we were checking mnt.parent() without vfs.mountMu locked or
  checking vfs.mounts.seq.

Reported-by: syzbot+78a0d5c373d28623a709@syzkaller.appspotmail.com
PiperOrigin-RevId: 572700791

pkg/sentry/vfs/mount.go

@@ -238,57 +238,58 @@ func (vfs *VirtualFilesystem) MountDisconnected(ctx context.Context, creds *auth
 }
 
 // attachTreeLocked attaches the mount tree at mnt to vd and propagates the
-// mount to vd.mount's peers and followers. This method is analogous to
-// fs/namespace.c:attach_recursive_mnt() in Linux.
+// mount to vd.mount's peers and followers. This method consumes the reference
+// on vd. It is analogous to fs/namespace.c:attach_recursive_mnt() in Linux.
 //
 // +checklocks:vfs.mountMu
 func (vfs *VirtualFilesystem) attachTreeLocked(ctx context.Context, mnt *Mount, vd VirtualDentry) error {
-	vdCleanup := cleanup.Make(func() {
-		vd.DecRef(ctx)
-	})
-	defer vdCleanup.Clean()
-	// This is equivalent to checking for SB_NOUSER in Linux, which is set on all
-	// anon mounts and sentry-internal filesystems like pipefs.
-	if vd.mount.neverConnected() {
-		return linuxerr.EINVAL
-	}
-	defer func() {
-		vd.mount.ns.pending = 0
-	}()
-	if err := vd.mount.ns.checkMountCount(ctx, mnt); err != nil {
-		return err
-	}
-	if vd.mount.isShared {
-		if err := vfs.allocMountGroupIDs(mnt, true); err != nil {
-			return err
-		}
-	}
-	propMnts, err := vfs.doPropagation(ctx, mnt, vd)
-	cleanup := cleanup.Make(func() {
-		// Checklocks can't understand lock state within closures, so we have to
-		// force.
-		vfs.freeMountGroupIDs(mnt.submountsLocked()) // +checklocksforce
-		for pmnt := range propMnts {
-			if !pmnt.parent().neverConnected() {
-				pmnt.parent().ns.pending -= pmnt.countSubmountsLocked() // +checklocksforce
-			}
-			vfs.abortUncommitedMount(ctx, pmnt) // +checklocksforce
-		}
-	})
-	defer cleanup.Clean()
+	mp, err := vfs.lockMountpoint(vd)
 	if err != nil {
 		return err
 	}
-	if vd.mount.isShared {
+	cleanup := cleanup.Make(func() {
+		vfs.cleanupGroupIDs(mnt.submountsLocked()) // +checklocksforce
+		mp.dentry.mu.Unlock()
+		mp.DecRef(ctx)
+	})
+	defer cleanup.Clean()
+	// This is equivalent to checking for SB_NOUSER in Linux, which is set on all
+	// anon mounts and sentry-internal filesystems like pipefs.
+	if mp.mount.neverConnected() {
+		return linuxerr.EINVAL
+	}
+	defer func() { mp.mount.ns.pending = 0 }()
+	if err := mp.mount.ns.checkMountCount(ctx, mnt); err != nil {
+		return err
+	}
+
+	var propMnts map[*Mount]struct{}
+	if mp.mount.isShared {
+		if err := vfs.allocMountGroupIDs(mnt, true); err != nil {
+			return err
+		}
+		propMnts, err = vfs.doPropagation(ctx, mnt, mp)
+		if err != nil {
+			for pmnt := range propMnts {
+				if !pmnt.parent().neverConnected() {
+					pmnt.parent().ns.pending -= pmnt.countSubmountsLocked()
+				}
+				vfs.abortUncommitedMount(ctx, pmnt)
+			}
+			return err
+		}
+	}
+	cleanup.Release()
+
+	if mp.mount.isShared {
 		for _, m := range mnt.submountsLocked() {
 			m.isShared = true
 		}
 	}
-	vdCleanup.Release()
-	if err := vfs.connectMountAtLocked(ctx, mnt, vd); err != nil {
-		return err
-	}
-	cleanup.Release()
+	vfs.mounts.seq.BeginWrite()
+	vfs.connectLocked(mnt, mp, mp.mount.ns)
+	vfs.mounts.seq.EndWrite()
+	mp.dentry.mu.Unlock()
 	vfs.commitChildren(ctx, mnt)
 	for pmnt := range propMnts {
 		vfs.commitMount(ctx, pmnt)
@@ -311,31 +312,6 @@ func (vfs *VirtualFilesystem) ConnectMountAt(ctx context.Context, creds *auth.Cr
 	return vfs.attachTreeLocked(ctx, mnt, vd)
 }
 
-// connectMountAtLocked attaches mnt at vd. This method consumes a reference on
-// vd and returns a list of VirtualDentry with an extra reference that must be
-// DecRef'd outside of vfs.mountMu.
-//
-// Preconditions:
-//   - mnt must be disconnected.
-//   - vfs.mountMu must be locked.
-//
-// +checklocks:vfs.mountMu
-func (vfs *VirtualFilesystem) connectMountAtLocked(ctx context.Context, mnt *Mount, vd VirtualDentry) error {
-	mp, err := vfs.lockMountpoint(vd)
-	if err != nil {
-		return err
-	}
-	// TODO(gvisor.dev/issue/1035): Linux requires that either both the mount
-	// point and the mount root are directories, or neither are, and returns
-	// ENOTDIR if this is not the case.
-	mntns := vd.mount.ns
-	vfs.mounts.seq.BeginWrite()
-	vfs.connectLocked(mnt, mp, mntns)
-	vfs.mounts.seq.EndWrite()
-	mp.dentry.mu.Unlock()
-	return nil
-}
-
 // lockMountpoint returns VirtualDentry with a locked Dentry. If vd is a
 // mountpoint, the method returns a VirtualDentry with a locked Dentry that is
 // the top most mount stacked on that Dentry. This method consumes a reference
@@ -411,12 +387,10 @@ func (vfs *VirtualFilesystem) cloneMount(mnt *Mount, root *Dentry, mopts *MountO
 		clone.groupID = mnt.groupID
 	}
 	if cloneType&makeSharedClone != 0 && clone.groupID == 0 {
-		gid, err := vfs.allocateGroupID()
-		if err != nil {
+		if err := vfs.allocateGroupID(clone); err != nil {
 			vfs.delayDecRef(clone)
-			return nil, linuxerr.ENOSPC
+			return nil, err
 		}
-		clone.groupID = gid
 	}
 	clone.isShared = mnt.isShared
 	if cloneType&makeFollowerClone != 0 || (cloneType&sharedToFollowerClone != 0 && mnt.isShared) {
@@ -828,20 +802,20 @@ func (mnt *Mount) DecRef(ctx context.Context) {
 }
 
 func (mnt *Mount) destroy(ctx context.Context) {
+	mnt.vfs.lockMounts()
+	defer mnt.vfs.unlockMounts(ctx)
 	if mnt.parent() != nil {
-		mnt.vfs.lockMounts()
 		mnt.vfs.mounts.seq.BeginWrite()
 		vd := mnt.vfs.disconnectLocked(mnt)
 		if vd.Ok() {
 			mnt.vfs.delayDecRef(vd)
 		}
 		mnt.vfs.mounts.seq.EndWrite()
-		mnt.vfs.unlockMounts(ctx)
 	}
 	if mnt.root != nil {
-		mnt.root.DecRef(ctx)
+		mnt.vfs.delayDecRef(mnt.root)
 	}
-	mnt.fs.DecRef(ctx)
+	mnt.vfs.delayDecRef(mnt.fs)
 }
 
 // RefType implements refs.CheckedObject.Type.

pkg/sentry/vfs/propagation.go

@@ -15,8 +15,6 @@
 package vfs
 
 import (
-	"fmt"
-
 	"gvisor.dev/gvisor/pkg/abi/linux"
 	"gvisor.dev/gvisor/pkg/bits"
 	"gvisor.dev/gvisor/pkg/context"
@@ -62,13 +60,17 @@ func (vfs *VirtualFilesystem) commitMount(ctx context.Context, mnt *Mount) {
 	if child != nil {
 		vfs.delayDecRef(vfs.disconnectLocked(child))
 	}
+	mp.dentry.mu.Lock()
 	vfs.connectLocked(mnt, mp, mp.mount.ns)
+	mp.dentry.mu.Unlock()
 	vfs.delayDecRef(mnt)
 
 	if child != nil {
 		newmp := VirtualDentry{mnt, mnt.root}
 		newmp.IncRef()
+		newmp.dentry.mu.Lock()
 		vfs.connectLocked(child, newmp, newmp.mount.ns)
+		newmp.dentry.mu.Unlock()
 		vfs.delayDecRef(child)
 	}
 	vfs.mounts.seq.EndWrite()
@@ -127,21 +129,21 @@ func (vfs *VirtualFilesystem) SetMountPropagationAt(ctx context.Context, creds *
 }
 
 // SetMountPropagation changes the propagation type of the mount.
-func (vfs *VirtualFilesystem) SetMountPropagation(mnt *Mount, propFlags uint32, recursive bool) error {
+func (vfs *VirtualFilesystem) SetMountPropagation(mnt *Mount, propFlag uint32, recursive bool) error {
 	vfs.lockMounts()
 	defer vfs.unlockMounts(context.Background())
-	if propFlags == linux.MS_SHARED {
+	if propFlag == linux.MS_SHARED {
 		if err := vfs.allocMountGroupIDs(mnt, recursive); err != nil {
-			return fmt.Errorf("allocMountGroupIDs: %v", err)
+			return err
 		}
 	}
 
 	if !recursive {
-		vfs.setPropagation(mnt, propFlags)
+		vfs.setPropagation(mnt, propFlag)
 		return nil
 	}
 	for _, m := range mnt.submountsLocked() {
-		vfs.setPropagation(m, propFlags)
+		vfs.setPropagation(m, propFlag)
 	}
 	return nil
 }
@@ -445,21 +447,22 @@ func (vfs *VirtualFilesystem) arePropMountsBusy(mnt *Mount) bool {
 	return false
 }
 
-// allocateGroupID returns a new mount group id if one is available, and
-// error otherwise. If the group ID bitmap is full, double the size of the
-// bitmap before allocating the new group id. It is analogous to
-// fs/namespace.c:mnt_alloc_group_id() in Linux.
+// allocateGroupID populates mnt.groupID with a new group id if one is
+// available, and returns an error otherwise. If the group ID bitmap is full,
+// double the size of the bitmap before allocating the new group id. It is
+// analogous to fs/namespace.c:mnt_alloc_group_id() in Linux.
 //
 // +checklocks:vfs.mountMu
-func (vfs *VirtualFilesystem) allocateGroupID() (uint32, error) {
+func (vfs *VirtualFilesystem) allocateGroupID(mnt *Mount) error {
 	groupID, err := vfs.groupIDBitmap.FirstZero(1)
 	if err != nil {
 		if err := vfs.groupIDBitmap.Grow(uint32(vfs.groupIDBitmap.Size())); err != nil {
-			return 0, err
+			return linuxerr.ENOSPC
 		}
 	}
 	vfs.groupIDBitmap.Add(groupID)
-	return groupID, nil
+	mnt.groupID = groupID
+	return nil
 }
 
 // freeGroupID marks a groupID as available for reuse. It is analogous to
@@ -471,14 +474,14 @@ func (vfs *VirtualFilesystem) freeGroupID(mnt *Mount) {
 	mnt.groupID = 0
 }
 
-// freeMountGroupIDs zeroes out all of the mounts' groupIDs and returns them
+// cleanupGroupIDs zeroes out all of the mounts' groupIDs and returns them
 // to the pool of available ids. It is analogous to
 // fs/namespace.c:cleanup_group_ids() in Linux.
 //
 // +checklocks:vfs.mountMu
-func (vfs *VirtualFilesystem) freeMountGroupIDs(mnts []*Mount) {
+func (vfs *VirtualFilesystem) cleanupGroupIDs(mnts []*Mount) {
 	for _, m := range mnts {
-		if m.groupID != 0 && m.isShared {
+		if m.groupID != 0 && !m.isShared {
 			vfs.freeGroupID(m)
 		}
 	}
@@ -498,10 +501,8 @@ func (vfs *VirtualFilesystem) allocMountGroupIDs(mnt *Mount, recursive bool) err
 	}
 	for _, m := range mnts {
 		if m.groupID == 0 && !m.isShared {
-			gid, err := vfs.allocateGroupID()
-			m.groupID = gid
-			if err != nil {
-				vfs.freeMountGroupIDs(mnts)
+			if err := vfs.allocateGroupID(m); err != nil {
+				vfs.cleanupGroupIDs(mnts)
 				return err
 			}
 		}