From a8bc2e1466267d8c5d9da7ddf632bc187286b6a2 Mon Sep 17 00:00:00 2001 From: Lucas Manning Date: Wed, 11 Oct 2023 15:14:34 -0700 Subject: [PATCH] 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 | 122 +++++++++++++--------------------- pkg/sentry/vfs/propagation.go | 43 ++++++------ 2 files changed, 70 insertions(+), 95 deletions(-) diff --git a/pkg/sentry/vfs/mount.go b/pkg/sentry/vfs/mount.go index f17fb4adf..5aa835a35 100644 --- a/pkg/sentry/vfs/mount.go +++ b/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. diff --git a/pkg/sentry/vfs/propagation.go b/pkg/sentry/vfs/propagation.go index 87c55ca93..83a36a2d2 100644 --- a/pkg/sentry/vfs/propagation.go +++ b/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 } }