From bb5ada8caffd5651d2560da0ad7b6e0d65e61a51 Mon Sep 17 00:00:00 2001 From: Lucas Manning Date: Thu, 15 Jun 2023 11:55:28 -0700 Subject: [PATCH] Defer dec refing mounts in InvalidateDentry. Reported-by: syzbot+f92ce6b097f3a67f5b9b@syzkaller.appspotmail.com PiperOrigin-RevId: 540648609 --- pkg/sentry/fsimpl/gofer/revalidate.go | 6 ++--- pkg/sentry/fsimpl/kernfs/filesystem.go | 6 ++--- pkg/sentry/fsimpl/kernfs/kernfs.go | 12 +++++----- pkg/sentry/vfs/dentry.go | 33 ++++++++++++++++---------- 4 files changed, 32 insertions(+), 25 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/revalidate.go b/pkg/sentry/fsimpl/gofer/revalidate.go index ea7223c7f..69d80799b 100644 --- a/pkg/sentry/fsimpl/gofer/revalidate.go +++ b/pkg/sentry/fsimpl/gofer/revalidate.go @@ -202,9 +202,9 @@ 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() - vds := vfsObj.InvalidateDentry(ctx, &d.vfsd) - for _, vd := range vds { - vd.DecRef(ctx) + rcs := vfsObj.InvalidateDentry(ctx, &d.vfsd) + for _, rc := range rcs { + rc.DecRef(ctx) } d.decRefNoCaching() diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go index 7d4d21a7c..c28959ce9 100644 --- a/pkg/sentry/fsimpl/kernfs/filesystem.go +++ b/pkg/sentry/fsimpl/kernfs/filesystem.go @@ -118,9 +118,9 @@ func (fs *Filesystem) revalidateChildLocked(ctx context.Context, vfsObj *vfs.Vir // Drop the ref owned by kernfs. fs.deferDecRef(child) } - vds := vfsObj.InvalidateDentry(ctx, child.VFSDentry()) - for _, vd := range vds { - fs.deferDecRef(vd) + rcs := vfsObj.InvalidateDentry(ctx, child.VFSDentry()) + for _, rc := range rcs { + fs.deferDecRef(rc) } child = nil } diff --git a/pkg/sentry/fsimpl/kernfs/kernfs.go b/pkg/sentry/fsimpl/kernfs/kernfs.go index acbf1b5f0..805bdd8e8 100644 --- a/pkg/sentry/fsimpl/kernfs/kernfs.go +++ b/pkg/sentry/fsimpl/kernfs/kernfs.go @@ -336,9 +336,9 @@ func (d *Dentry) cacheLocked(ctx context.Context) { // as described in Inode.Getlink. if isDead := d.VFSDentry().IsDead(); isDead || d.parent == nil { if !isDead { - vds := d.fs.vfsfs.VirtualFilesystem().InvalidateDentry(ctx, d.VFSDentry()) - for _, vd := range vds { - d.fs.deferDecRef(vd) + rcs := d.fs.vfsfs.VirtualFilesystem().InvalidateDentry(ctx, d.VFSDentry()) + for _, rc := range rcs { + d.fs.deferDecRef(rc) } } if d.cached { @@ -401,9 +401,9 @@ 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. - vds := d.fs.vfsfs.VirtualFilesystem().InvalidateDentry(ctx, d.VFSDentry()) - for _, vd := range vds { - d.fs.deferDecRef(vd) + rcs := d.fs.vfsfs.VirtualFilesystem().InvalidateDentry(ctx, d.VFSDentry()) + for _, rc := range rcs { + d.fs.deferDecRef(rc) } 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 1a63c0d65..d230a4380 100644 --- a/pkg/sentry/vfs/dentry.go +++ b/pkg/sentry/vfs/dentry.go @@ -18,6 +18,7 @@ import ( "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/errors/linuxerr" + "gvisor.dev/gvisor/pkg/refs" "gvisor.dev/gvisor/pkg/sync" ) @@ -233,7 +234,7 @@ func (vfs *VirtualFilesystem) CommitDeleteDentry(ctx context.Context, d *Dentry) d.dead = true d.mu.Unlock() if d.isMounted() { - vfs.forgetDeadMountpoint(ctx, d, false /*deferVdDecRef*/) + vfs.forgetDeadMountpoint(ctx, d, false /*skipDecRef*/) } } @@ -242,12 +243,12 @@ func (vfs *VirtualFilesystem) CommitDeleteDentry(ctx context.Context, d *Dentry) // of a file on a remote filesystem on which the file has already been // 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 { +func (vfs *VirtualFilesystem) InvalidateDentry(ctx context.Context, d *Dentry) []refs.RefCounter { d.mu.Lock() d.dead = true d.mu.Unlock() if d.isMounted() { - return vfs.forgetDeadMountpoint(ctx, d, true /*deferVdDecRef*/) + return vfs.forgetDeadMountpoint(ctx, d, true /*skipDecRef*/) } return nil } @@ -309,7 +310,7 @@ func (vfs *VirtualFilesystem) CommitRenameReplaceDentry(ctx context.Context, fro to.dead = true to.mu.Unlock() if to.isMounted() { - vfs.forgetDeadMountpoint(ctx, to, false /*deferVdDecRef*/) + vfs.forgetDeadMountpoint(ctx, to, false /*skipDecRef*/) } } } @@ -326,11 +327,13 @@ func (vfs *VirtualFilesystem) CommitRenameExchangeDentry(from, to *Dentry) { } // forgetDeadMountpoint is called when a mount point is deleted or invalidated -// to umount all mounts using it in all other mount namespaces. +// to umount all mounts using it in all other mount namespaces. If skipDecRef +// is true, the method returns a list of reference counted objects with an +// an extra reference. // // forgetDeadMountpoint is analogous to Linux's // fs/namespace.c:__detach_mounts(). -func (vfs *VirtualFilesystem) forgetDeadMountpoint(ctx context.Context, d *Dentry, deferVdDecRef bool) []VirtualDentry { +func (vfs *VirtualFilesystem) forgetDeadMountpoint(ctx context.Context, d *Dentry, skipDecRef bool) []refs.RefCounter { var ( vdsToDecRef []VirtualDentry mountsToDecRef []*Mount @@ -342,14 +345,18 @@ func (vfs *VirtualFilesystem) forgetDeadMountpoint(ctx context.Context, d *Dentr } vfs.mounts.seq.EndWrite() vfs.mountMu.Unlock() + rcs := make([]refs.RefCounter, 0, len(vdsToDecRef)+len(mountsToDecRef)) + for _, vd := range vdsToDecRef { + rcs = append(rcs, vd) + } for _, mnt := range mountsToDecRef { - mnt.DecRef(ctx) + rcs = append(rcs, mnt) } - if !deferVdDecRef { - for _, vd := range vdsToDecRef { - vd.DecRef(ctx) - } - return nil + if skipDecRef { + return rcs } - return vdsToDecRef + for _, rc := range rcs { + rc.DecRef(ctx) + } + return nil }