From 77b137ffd8ecc0fa11a068dd44988b57cc20c030 Mon Sep 17 00:00:00 2001 From: Lucas Manning Date: Thu, 16 Nov 2023 12:30:24 -0800 Subject: [PATCH] Fix umount not unmounting all the mounts it is supposed to. A previous change mistakenly set umountMnts list to only include mnt when opts.disconnectHeirarchy was false. This is incorrect. This change also adds shouldUmount, which is analogous to disconnect_mount in Linux. Reported-by: syzbot+2844283c3e8db313b313@syzkaller.appspotmail.com Reported-by: syzbot+c247535f3b9e7cc11217@syzkaller.appspotmail.com Reported-by: syzbot+e90cfd9ca081c7b62662@syzkaller.appspotmail.com Reported-by: syzbot+e745c9169125614f7d8f@syzkaller.appspotmail.com Reported-by: syzbot+5224e0d7bca7fdd14703@syzkaller.appspotmail.com Reported-by: syzbot+d18ca2ac1c0ec305ac40@syzkaller.appspotmail.com Reported-by: syzbot+d5906818a0c812cb7019@syzkaller.appspotmail.com Reported-by: syzbot+1f6475b690863f6b5c8e@syzkaller.appspotmail.com PiperOrigin-RevId: 583128457 --- pkg/sentry/vfs/dentry.go | 9 +++- pkg/sentry/vfs/mount.go | 92 ++++++++++++++++++++++++++++++---------- 2 files changed, 77 insertions(+), 24 deletions(-) diff --git a/pkg/sentry/vfs/dentry.go b/pkg/sentry/vfs/dentry.go index d574162c6..743654d7d 100644 --- a/pkg/sentry/vfs/dentry.go +++ b/pkg/sentry/vfs/dentry.go @@ -338,7 +338,14 @@ func (vfs *VirtualFilesystem) forgetDeadMountpoint(ctx context.Context, d *Dentr vfs.lockMounts() defer vfs.unlockMounts(ctx) for mnt := range vfs.mountpoints[d] { - vfs.umountTreeLocked(mnt, &umountRecursiveOptions{}) + if mnt.umounted { + vfs.mounts.seq.BeginWrite() + vfs.delayDecRef(vfs.disconnectLocked(mnt)) + vfs.delayDecRef(mnt) + vfs.mounts.seq.EndWrite() + } else { + vfs.umountTreeLocked(mnt, &umountRecursiveOptions{}) + } } return vfs.PopDelayedDecRefs() } diff --git a/pkg/sentry/vfs/mount.go b/pkg/sentry/vfs/mount.go index 7e73d9ff9..45dc079bf 100644 --- a/pkg/sentry/vfs/mount.go +++ b/pkg/sentry/vfs/mount.go @@ -642,19 +642,38 @@ type umountRecursiveOptions struct { propagate bool } +// shouldUmount returns if this mount should be disconnected from its parent. +// It is analogous to fs/namespace.c:disconnect_mount() in Linux. +// +// +checklocks:vfs.mountMu +func (vfs *VirtualFilesystem) shouldUmount(mnt *Mount, opts *umountRecursiveOptions) bool { + // Always disconnect when it's not a lazy unmount. + if opts.eager { + return true + } + // If a mount does not have a parent, it won't be disconnected but will be + // DecRef-ed. + if mnt.parent() == nil { + return true + } + // Always unmount if the parent is nor marked as unmounted. + if !mnt.parent().umounted { + return true + } + // If the parent is marked as unmounted, we can only unmount is + // UMOUNT_CONNECTED is false. + if !opts.disconnectHierarchy { + return false + } + return true +} + // 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) umountTreeLocked(mnt *Mount, opts *umountRecursiveOptions) { - submounts := mnt.submountsLocked() - var umountMnts []*Mount - if opts.disconnectHierarchy { - umountMnts = submounts - } else { - umountMnts = []*Mount{mnt} - } - + umountMnts := mnt.submountsLocked() for _, mnt := range umountMnts { vfs.umount(mnt) } @@ -664,16 +683,7 @@ func (vfs *VirtualFilesystem) umountTreeLocked(mnt *Mount, opts *umountRecursive vfs.mounts.seq.BeginWrite() for _, mnt := range umountMnts { - vfs.delayDecRef(mnt) - 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 _, mnt := range submounts { + if opts.eager { for { refs := mnt.refs.Load() if refs < 0 { @@ -684,12 +694,27 @@ func (vfs *VirtualFilesystem) umountTreeLocked(mnt *Mount, opts *umountRecursive } } } + if mnt.parent() != nil { + if vfs.shouldUmount(mnt, opts) { + vfs.delayDecRef(vfs.disconnectLocked(mnt)) + } else { + // Restore mnt in it's parent children list, but leave it marked as + // unmounted. These partly unmounted mounts are cleaned up in + // vfs.forgetDeadMountpoints and Mount.destroy. + mnt.parent().children[mnt] = struct{}{} + } + } + vfs.setPropagation(mnt, linux.MS_PRIVATE) } + vfs.mounts.seq.EndWrite() } // +checklocks:vfs.mountMu func (vfs *VirtualFilesystem) umount(mnt *Mount) { - mnt.umounted = true + if !mnt.umounted { + mnt.umounted = true + vfs.delayDecRef(mnt) + } if parent := mnt.parent(); parent != nil { delete(parent.children, mnt) } @@ -715,14 +740,17 @@ func (vfs *VirtualFilesystem) changeMountpoint(mnt *Mount, mp VirtualDentry) { // - vfs.mountMu must be locked. // - vfs.mounts.seq must be in a writer critical section. // - d.mu must be locked. -// - mnt.parent() == nil, i.e. mnt must not already be connected. +// - mnt.parent() == nil or mnt.parent().children doesn't contain mnt. +// i.e. mnt must not already be connected. func (vfs *VirtualFilesystem) connectLocked(mnt *Mount, vd VirtualDentry, mntns *MountNamespace) { if checkInvariants { - if mnt.parent() != nil { - panic("VFS.connectLocked called on connected mount") + if mnt.parent() != nil && mnt.parent().children != nil { + if _, ok := mnt.parent().children[mnt]; ok { + panic("VFS.connectLocked called on connected mount") + } } } - mnt.IncRef() // dropped by callers of umountRecursiveLocked + mnt.IncRef() // dropped by vfs.umount(). mnt.setKey(vd) if vd.mount.children == nil { vd.mount.children = make(map[*Mount]struct{}) @@ -832,6 +860,24 @@ func (mnt *Mount) destroy(ctx context.Context) { } mnt.vfs.mounts.seq.EndWrite() } + + // Cleanup any leftover children. + if len(mnt.children) != 0 { + mnt.vfs.mounts.seq.BeginWrite() + for child := range mnt.children { + if checkInvariants { + if !child.umounted { + panic("children of a mount that has no references should already be marked as unmounted.") + } + } + vd := mnt.vfs.disconnectLocked(child) + if vd.Ok() { + mnt.vfs.delayDecRef(vd) + } + } + mnt.vfs.mounts.seq.EndWrite() + } + if mnt.root != nil { mnt.vfs.delayDecRef(mnt.root) }