From 084a5022563f2c71b76abf66b16f72ea0126df99 Mon Sep 17 00:00:00 2001 From: Lucas Manning Date: Mon, 12 Jun 2023 20:18:32 -0700 Subject: [PATCH] Change InvalidateDentry to return a list of vds with an extra reference. DecRef'ing dead mountpoints while holding a filesystem lock can cause nested locking, since we need to lock the filesystem to access the cache. Reported-by: syzbot+31c9a17cf680d45205bf@syzkaller.appspotmail.com PiperOrigin-RevId: 539834013 --- pkg/sentry/fsimpl/gofer/revalidate.go | 5 ++++- pkg/sentry/fsimpl/kernfs/filesystem.go | 5 ++++- pkg/sentry/fsimpl/kernfs/kernfs.go | 10 ++++++++-- pkg/sentry/vfs/dentry.go | 24 +++++++++++++++--------- 4 files changed, 31 insertions(+), 13 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/revalidate.go b/pkg/sentry/fsimpl/gofer/revalidate.go index 2448a3fa2..ea7223c7f 100644 --- a/pkg/sentry/fsimpl/gofer/revalidate.go +++ b/pkg/sentry/fsimpl/gofer/revalidate.go @@ -202,7 +202,10 @@ func (d *dentry) invalidate(ctx context.Context, vfsObj *vfs.VirtualFilesystem, // this, take a dentry reference first, then drop it while // deferring the call to dentry.checkCachingLocked(). d.IncRef() - vfsObj.InvalidateDentry(ctx, &d.vfsd) + vds := vfsObj.InvalidateDentry(ctx, &d.vfsd) + for _, vd := range vds { + vd.DecRef(ctx) + } d.decRefNoCaching() // Re-evaluate its caching status (i.e. if it has 0 references, drop it). diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go index 6eb7bbeba..7d4d21a7c 100644 --- a/pkg/sentry/fsimpl/kernfs/filesystem.go +++ b/pkg/sentry/fsimpl/kernfs/filesystem.go @@ -118,7 +118,10 @@ func (fs *Filesystem) revalidateChildLocked(ctx context.Context, vfsObj *vfs.Vir // Drop the ref owned by kernfs. fs.deferDecRef(child) } - vfsObj.InvalidateDentry(ctx, child.VFSDentry()) + vds := vfsObj.InvalidateDentry(ctx, child.VFSDentry()) + for _, vd := range vds { + fs.deferDecRef(vd) + } child = nil } } diff --git a/pkg/sentry/fsimpl/kernfs/kernfs.go b/pkg/sentry/fsimpl/kernfs/kernfs.go index 1f0f82104..acbf1b5f0 100644 --- a/pkg/sentry/fsimpl/kernfs/kernfs.go +++ b/pkg/sentry/fsimpl/kernfs/kernfs.go @@ -336,7 +336,10 @@ func (d *Dentry) cacheLocked(ctx context.Context) { // as described in Inode.Getlink. if isDead := d.VFSDentry().IsDead(); isDead || d.parent == nil { if !isDead { - d.fs.vfsfs.VirtualFilesystem().InvalidateDentry(ctx, d.VFSDentry()) + vds := d.fs.vfsfs.VirtualFilesystem().InvalidateDentry(ctx, d.VFSDentry()) + for _, vd := range vds { + d.fs.deferDecRef(vd) + } } if d.cached { d.fs.cachedDentries.Remove(d) @@ -398,7 +401,10 @@ func (d *Dentry) evictLocked(ctx context.Context) { d.parent.dirMu.Lock() // Note that victim can't be a mount point (in any mount // namespace), since VFS holds references on mount points. - d.fs.vfsfs.VirtualFilesystem().InvalidateDentry(ctx, d.VFSDentry()) + vds := d.fs.vfsfs.VirtualFilesystem().InvalidateDentry(ctx, d.VFSDentry()) + for _, vd := range vds { + d.fs.deferDecRef(vd) + } delete(d.parent.children, d.name) d.parent.dirMu.Unlock() } diff --git a/pkg/sentry/vfs/dentry.go b/pkg/sentry/vfs/dentry.go index 508df80b8..1a63c0d65 100644 --- a/pkg/sentry/vfs/dentry.go +++ b/pkg/sentry/vfs/dentry.go @@ -233,21 +233,23 @@ func (vfs *VirtualFilesystem) CommitDeleteDentry(ctx context.Context, d *Dentry) d.dead = true d.mu.Unlock() if d.isMounted() { - vfs.forgetDeadMountpoint(ctx, d) + vfs.forgetDeadMountpoint(ctx, d, false /*deferVdDecRef*/) } } // InvalidateDentry is called when d ceases to represent the file it formerly // did for reasons outside of VFS' control (e.g. d represents the local state // of a file on a remote filesystem on which the file has already been -// deleted). -func (vfs *VirtualFilesystem) InvalidateDentry(ctx context.Context, d *Dentry) { +// deleted). If d is mounted, the method returns a list of Virtual Dentries +// mounted on d that the caller is responsible for DecRefing. +func (vfs *VirtualFilesystem) InvalidateDentry(ctx context.Context, d *Dentry) []VirtualDentry { d.mu.Lock() d.dead = true d.mu.Unlock() if d.isMounted() { - vfs.forgetDeadMountpoint(ctx, d) + return vfs.forgetDeadMountpoint(ctx, d, true /*deferVdDecRef*/) } + return nil } // PrepareRenameDentry must be called before attempting to rename the file @@ -307,7 +309,7 @@ func (vfs *VirtualFilesystem) CommitRenameReplaceDentry(ctx context.Context, fro to.dead = true to.mu.Unlock() if to.isMounted() { - vfs.forgetDeadMountpoint(ctx, to) + vfs.forgetDeadMountpoint(ctx, to, false /*deferVdDecRef*/) } } } @@ -328,7 +330,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) { +func (vfs *VirtualFilesystem) forgetDeadMountpoint(ctx context.Context, d *Dentry, deferVdDecRef bool) []VirtualDentry { var ( vdsToDecRef []VirtualDentry mountsToDecRef []*Mount @@ -340,10 +342,14 @@ func (vfs *VirtualFilesystem) forgetDeadMountpoint(ctx context.Context, d *Dentr } vfs.mounts.seq.EndWrite() vfs.mountMu.Unlock() - for _, vd := range vdsToDecRef { - vd.DecRef(ctx) - } for _, mnt := range mountsToDecRef { mnt.DecRef(ctx) } + if !deferVdDecRef { + for _, vd := range vdsToDecRef { + vd.DecRef(ctx) + } + return nil + } + return vdsToDecRef }