From c74f5866cb75bb5c712a69341c282da8e4920e59 Mon Sep 17 00:00:00 2001 From: Lucas Manning Date: Tue, 3 Oct 2023 10:04:59 -0700 Subject: [PATCH] Fix circular lock that can happen during unlink. Reported-by: syzbot+d93d90350a33fbd2c8b9@syzkaller.appspotmail.com PiperOrigin-RevId: 570420492 --- pkg/sentry/fsimpl/gofer/filesystem.go | 22 +++++++++++++++-- pkg/sentry/fsimpl/kernfs/filesystem.go | 14 ++++++++--- pkg/sentry/fsimpl/overlay/filesystem.go | 32 ++++++++++++++++++++++--- pkg/sentry/fsimpl/tmpfs/filesystem.go | 31 +++++++++++++++++++++--- pkg/sentry/vfs/dentry.go | 26 ++++++++++---------- 5 files changed, 101 insertions(+), 24 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go index 6a0ad8347..4a82efcea 100644 --- a/pkg/sentry/fsimpl/gofer/filesystem.go +++ b/pkg/sentry/fsimpl/gofer/filesystem.go @@ -26,6 +26,7 @@ import ( "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/fspath" + "gvisor.dev/gvisor/pkg/refs" "gvisor.dev/gvisor/pkg/sentry/fsimpl/host" "gvisor.dev/gvisor/pkg/sentry/fsmetric" "gvisor.dev/gvisor/pkg/sentry/kernel" @@ -540,6 +541,15 @@ func (fs *filesystem) doCreateAt(ctx context.Context, rp *vfs.ResolvingPath, dir func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir bool) error { var ds *[]*dentry fs.renameMu.RLock() + // We need to DecRef outside of fs.renameMu because forgetting a dead + // mountpoint could result in this filesystem being released which acquires + // fs.renameMu. + var toDecRef []refs.RefCounter + defer func() { + for _, ref := range toDecRef { + ref.DecRef(ctx) + } + }() defer fs.renameMuRUnlockAndCheckCaching(ctx, &ds) start := rp.Start().Impl().(*dentry) parent, err := fs.walkParentDirLocked(ctx, rp, start, &ds) @@ -701,7 +711,7 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b defer parent.childrenMu.Unlock() if child != nil { - vfsObj.CommitDeleteDentry(ctx, &child.vfsd) // +checklocksforce: see above. + toDecRef = vfsObj.CommitDeleteDentry(ctx, &child.vfsd) // +checklocksforce: see above. child.setDeleted() if child.isSynthetic() { parent.syntheticChildren-- @@ -1322,6 +1332,14 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa // Resolve newParent first to verify that it's on this Mount. var ds *[]*dentry fs.renameMu.Lock() + // We need to DecRef outside of fs.mu because forgetting a dead mountpoint + // could result in this filesystem being released which acquires fs.mu. + var toDecRef []refs.RefCounter + defer func() { + for _, ref := range toDecRef { + ref.DecRef(ctx) + } + }() defer fs.renameMuUnlockAndCheckCaching(ctx, &ds) newParent, err := fs.walkParentDirLocked(ctx, rp, rp.Start().Impl().(*dentry), &ds) if err != nil { @@ -1471,7 +1489,7 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa defer oldParent.childrenMu.Unlock() } - vfsObj.CommitRenameReplaceDentry(ctx, &renamed.vfsd, replacedVFSD) + toDecRef = vfsObj.CommitRenameReplaceDentry(ctx, &renamed.vfsd, replacedVFSD) if replaced != nil { replaced.setDeleted() if replaced.isSynthetic() { diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go index c28959ce9..4b3e67a82 100644 --- a/pkg/sentry/fsimpl/kernfs/filesystem.go +++ b/pkg/sentry/fsimpl/kernfs/filesystem.go @@ -787,7 +787,9 @@ func (fs *Filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa replaced.setDeleted() } vfs.InotifyRename(ctx, src.inode.Watches(), srcDir.inode.Watches(), dstDir.inode.Watches(), oldName, newName, src.isDir()) - virtfs.CommitRenameReplaceDentry(ctx, srcVFSD, replaceVFSD) // +checklocksforce: to may be nil, that's okay. + for _, rc := range virtfs.CommitRenameReplaceDentry(ctx, srcVFSD, replaceVFSD) { // +checklocksforce: to may be nil, that's okay. + fs.deferDecRef(rc) + } return nil } @@ -855,7 +857,10 @@ func (fs *Filesystem) RmdirAt(ctx context.Context, rp *vfs.ResolvingPath) error parent.inode.Watches().Notify(ctx, child.name, linux.IN_DELETE|linux.IN_ISDIR, 0, vfs.InodeEvent, true /* unlinked */) // Defer decref so that fs.mu and parentDentry.dirMu are unlocked by then. fs.deferDecRef(child) - virtfs.CommitDeleteDentry(ctx, vfsd) + rcs := virtfs.CommitDeleteDentry(ctx, vfsd) + for _, rc := range rcs { + fs.deferDecRef(rc) + } child.setDeleted() return nil } @@ -983,7 +988,10 @@ func (fs *Filesystem) UnlinkAt(ctx context.Context, rp *vfs.ResolvingPath) error vfs.InotifyRemoveChild(ctx, d.inode.Watches(), parentDentry.inode.Watches(), d.name) // Defer decref so that fs.mu and parentDentry.dirMu are unlocked by then. fs.deferDecRef(d) - virtfs.CommitDeleteDentry(ctx, vfsd) + rcs := virtfs.CommitDeleteDentry(ctx, vfsd) + for _, rc := range rcs { + fs.deferDecRef(rc) + } d.setDeleted() return nil } diff --git a/pkg/sentry/fsimpl/overlay/filesystem.go b/pkg/sentry/fsimpl/overlay/filesystem.go index 5e52e71cb..493515dd4 100644 --- a/pkg/sentry/fsimpl/overlay/filesystem.go +++ b/pkg/sentry/fsimpl/overlay/filesystem.go @@ -24,6 +24,7 @@ import ( "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/fspath" "gvisor.dev/gvisor/pkg/log" + "gvisor.dev/gvisor/pkg/refs" "gvisor.dev/gvisor/pkg/sentry/kernel/auth" "gvisor.dev/gvisor/pkg/sentry/socket/unix/transport" "gvisor.dev/gvisor/pkg/sentry/vfs" @@ -1093,6 +1094,14 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa // Resolve newParent first to verify that it's on this Mount. var ds *[]*dentry fs.renameMu.Lock() + // We need to DecRef outside of fs.mu because forgetting a dead mountpoint + // could result in this filesystem being released which acquires fs.mu. + var toDecRef []refs.RefCounter + defer func() { + for _, ref := range toDecRef { + ref.DecRef(ctx) + } + }() defer fs.renameMuUnlockAndCheckDrop(ctx, &ds) newParent, err := fs.walkParentDirLocked(ctx, rp, rp.Start().Impl().(*dentry), &ds) if err != nil { @@ -1308,7 +1317,7 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa // Below this point, the renamed dentry is now at newpop, and anything we // replaced is gone forever. Commit the rename, update the overlay // filesystem tree, and abandon attempts to recover from errors. - vfsObj.CommitRenameReplaceDentry(ctx, &renamed.vfsd, replacedVFSD) + toDecRef = vfsObj.CommitRenameReplaceDentry(ctx, &renamed.vfsd, replacedVFSD) delete(oldParent.children, oldName) if replaced != nil { // Lower dentries of replaced are not reachable from the overlay anymore. @@ -1355,6 +1364,14 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa func (fs *filesystem) RmdirAt(ctx context.Context, rp *vfs.ResolvingPath) error { var ds *[]*dentry fs.renameMu.RLock() + // We need to DecRef outside of fs.mu because forgetting a dead mountpoint + // could result in this filesystem being released which acquires fs.mu. + var toDecRef []refs.RefCounter + defer func() { + for _, ref := range toDecRef { + ref.DecRef(ctx) + } + }() defer fs.renameMuRUnlockAndCheckDrop(ctx, &ds) start := rp.Start().Impl().(*dentry) parent, err := fs.walkParentDirLocked(ctx, rp, start, &ds) @@ -1467,7 +1484,7 @@ func (fs *filesystem) RmdirAt(ctx context.Context, rp *vfs.ResolvingPath) error return err } - vfsObj.CommitDeleteDentry(ctx, &child.vfsd) + toDecRef = vfsObj.CommitDeleteDentry(ctx, &child.vfsd) delete(parent.children, name) ds = appendDentry(ds, child) parent.dirents = nil @@ -1606,6 +1623,15 @@ func (fs *filesystem) SymlinkAt(ctx context.Context, rp *vfs.ResolvingPath, targ func (fs *filesystem) UnlinkAt(ctx context.Context, rp *vfs.ResolvingPath) error { var ds *[]*dentry fs.renameMu.RLock() + // We need to DecRef outside of fs.renameMu because forgetting a dead + // mountpoint could result in this filesystem being released which acquires + // fs.renameMu. + var toDecRef []refs.RefCounter + defer func() { + for _, ref := range toDecRef { + ref.DecRef(ctx) + } + }() defer fs.renameMuRUnlockAndCheckDrop(ctx, &ds) start := rp.Start().Impl().(*dentry) parent, err := fs.walkParentDirLocked(ctx, rp, start, &ds) @@ -1678,7 +1704,7 @@ func (fs *filesystem) UnlinkAt(ctx context.Context, rp *vfs.ResolvingPath) error return err } - vfsObj.CommitDeleteDentry(ctx, &child.vfsd) + toDecRef = vfsObj.CommitDeleteDentry(ctx, &child.vfsd) delete(parent.children, name) if !child.isDir() { // Once a whiteout is created, non-directory dentries on the lower layers diff --git a/pkg/sentry/fsimpl/tmpfs/filesystem.go b/pkg/sentry/fsimpl/tmpfs/filesystem.go index fa2d2a7b3..b2d7aaa12 100644 --- a/pkg/sentry/fsimpl/tmpfs/filesystem.go +++ b/pkg/sentry/fsimpl/tmpfs/filesystem.go @@ -21,6 +21,7 @@ import ( "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/fspath" + "gvisor.dev/gvisor/pkg/refs" "gvisor.dev/gvisor/pkg/sentry/fsmetric" "gvisor.dev/gvisor/pkg/sentry/kernel/auth" "gvisor.dev/gvisor/pkg/sentry/socket/unix/transport" @@ -526,6 +527,14 @@ func (fs *filesystem) ReadlinkAt(ctx context.Context, rp *vfs.ResolvingPath) (st func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldParentVD vfs.VirtualDentry, oldName string, opts vfs.RenameOptions) error { // Resolve newParentDir first to verify that it's on this Mount. fs.mu.Lock() + // We need to DecRef outside of fs.mu because forgetting a dead mountpoint + // could result in this filesystem being released which acquires fs.mu. + var toDecRef []refs.RefCounter + defer func() { + for _, ref := range toDecRef { + ref.DecRef(ctx) + } + }() defer fs.mu.Unlock() newParentDir, err := walkParentDirLocked(ctx, rp, rp.Start().Impl().(*dentry)) if err != nil { @@ -649,7 +658,7 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa } oldParentDir.removeChildLocked(renamed) newParentDir.insertChildLocked(renamed, newName) - vfsObj.CommitRenameReplaceDentry(ctx, &renamed.vfsd, replacedVFSD) + toDecRef = vfsObj.CommitRenameReplaceDentry(ctx, &renamed.vfsd, replacedVFSD) oldParentDir.inode.touchCMtime() if oldParentDir != newParentDir { if renamed.inode.isDir() { @@ -667,6 +676,14 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa // RmdirAt implements vfs.FilesystemImpl.RmdirAt. func (fs *filesystem) RmdirAt(ctx context.Context, rp *vfs.ResolvingPath) error { fs.mu.Lock() + // We need to DecRef outside of fs.mu because forgetting a dead mountpoint + // could result in this filesystem being released which acquires fs.mu. + var toDecRef []refs.RefCounter + defer func() { + for _, ref := range toDecRef { + ref.DecRef(ctx) + } + }() defer fs.mu.Unlock() parentDir, err := walkParentDirLocked(ctx, rp, rp.Start().Impl().(*dentry)) if err != nil { @@ -713,7 +730,7 @@ func (fs *filesystem) RmdirAt(ctx context.Context, rp *vfs.ResolvingPath) error child.inode.decLinksLocked(ctx) child.inode.decLinksLocked(ctx) parentDir.inode.decLinksLocked(ctx) - vfsObj.CommitDeleteDentry(ctx, &child.vfsd) + toDecRef = vfsObj.CommitDeleteDentry(ctx, &child.vfsd) parentDir.inode.touchCMtime() return nil } @@ -782,6 +799,14 @@ func (fs *filesystem) SymlinkAt(ctx context.Context, rp *vfs.ResolvingPath, targ // UnlinkAt implements vfs.FilesystemImpl.UnlinkAt. func (fs *filesystem) UnlinkAt(ctx context.Context, rp *vfs.ResolvingPath) error { fs.mu.Lock() + // We need to DecRef outside of fs.mu because forgetting a dead mountpoint + // could result in this filesystem being released which acquires fs.mu. + var toDecRef []refs.RefCounter + defer func() { + for _, ref := range toDecRef { + ref.DecRef(ctx) + } + }() defer fs.mu.Unlock() parentDir, err := walkParentDirLocked(ctx, rp, rp.Start().Impl().(*dentry)) if err != nil { @@ -824,7 +849,7 @@ func (fs *filesystem) UnlinkAt(ctx context.Context, rp *vfs.ResolvingPath) error vfs.InotifyRemoveChild(ctx, &child.inode.watches, &parentDir.inode.watches, name) parentDir.removeChildLocked(child) child.inode.decLinksLocked(ctx) - vfsObj.CommitDeleteDentry(ctx, &child.vfsd) + toDecRef = vfsObj.CommitDeleteDentry(ctx, &child.vfsd) parentDir.inode.touchCMtime() return nil } diff --git a/pkg/sentry/vfs/dentry.go b/pkg/sentry/vfs/dentry.go index 48bd5588b..9fcabdcd8 100644 --- a/pkg/sentry/vfs/dentry.go +++ b/pkg/sentry/vfs/dentry.go @@ -227,14 +227,16 @@ func (vfs *VirtualFilesystem) AbortDeleteDentry(d *Dentry) { } // CommitDeleteDentry must be called after PrepareDeleteDentry if the deletion -// succeeds. +// succeeds. If d is mounted, the method returns a list of Virtual Dentries +// mounted on d that the caller is responsible for DecRefing. // +checklocksrelease:d.mu -func (vfs *VirtualFilesystem) CommitDeleteDentry(ctx context.Context, d *Dentry) { +func (vfs *VirtualFilesystem) CommitDeleteDentry(ctx context.Context, d *Dentry) []refs.RefCounter { d.dead = true d.mu.Unlock() if d.isMounted() { - vfs.forgetDeadMountpoint(ctx, d, false /*skipDecRef*/) + return vfs.forgetDeadMountpoint(ctx, d) } + return nil } // InvalidateDentry is called when d ceases to represent the file it formerly @@ -247,7 +249,7 @@ func (vfs *VirtualFilesystem) InvalidateDentry(ctx context.Context, d *Dentry) [ d.dead = true d.mu.Unlock() if d.isMounted() { - return vfs.forgetDeadMountpoint(ctx, d, true /*skipDecRef*/) + return vfs.forgetDeadMountpoint(ctx, d) } return nil } @@ -296,20 +298,22 @@ func (vfs *VirtualFilesystem) AbortRenameDentry(from, to *Dentry) { // CommitRenameReplaceDentry must be called after the file represented by from // is renamed without RENAME_EXCHANGE. If to is not nil, it represents the file -// that was replaced by from. +// that was replaced by from. If to is mounted, the method returns a list of +// Virtual Dentries mounted on to that the caller is responsible for DecRefing. // // Preconditions: PrepareRenameDentry was previously called on from and to. // +checklocksrelease:from.mu // +checklocksrelease:to.mu -func (vfs *VirtualFilesystem) CommitRenameReplaceDentry(ctx context.Context, from, to *Dentry) { +func (vfs *VirtualFilesystem) CommitRenameReplaceDentry(ctx context.Context, from, to *Dentry) []refs.RefCounter { from.mu.Unlock() if to != nil { to.dead = true to.mu.Unlock() if to.isMounted() { - vfs.forgetDeadMountpoint(ctx, to, false /*skipDecRef*/) + return vfs.forgetDeadMountpoint(ctx, to) } } + return nil } // CommitRenameExchangeDentry must be called after the files represented by @@ -330,7 +334,7 @@ func (vfs *VirtualFilesystem) CommitRenameExchangeDentry(from, to *Dentry) { // // forgetDeadMountpoint is analogous to Linux's // fs/namespace.c:__detach_mounts(). -func (vfs *VirtualFilesystem) forgetDeadMountpoint(ctx context.Context, d *Dentry, skipDecRef bool) []refs.RefCounter { +func (vfs *VirtualFilesystem) forgetDeadMountpoint(ctx context.Context, d *Dentry) []refs.RefCounter { vfs.lockMounts() defer vfs.unlockMounts(ctx) vfs.mounts.seq.BeginWrite() @@ -338,9 +342,5 @@ func (vfs *VirtualFilesystem) forgetDeadMountpoint(ctx context.Context, d *Dentr vfs.umountRecursiveLocked(mnt, &umountRecursiveOptions{}) } vfs.mounts.seq.EndWrite() - var rcs []refs.RefCounter - if skipDecRef { - rcs = vfs.PopDelayedDecRefs() - } - return rcs + return vfs.PopDelayedDecRefs() }