From 0e6fe26a3a0df4f29f23c1abd692702e7110c39a Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Mon, 25 Nov 2024 18:12:41 -0800 Subject: [PATCH] fsimpl/gofer: hold extra dentry reference if endpoint != nil Fixes #11202 PiperOrigin-RevId: 700163466 --- pkg/sentry/fsimpl/gofer/directfs_dentry.go | 4 ++- pkg/sentry/fsimpl/gofer/filesystem.go | 4 +++ pkg/sentry/fsimpl/gofer/gofer.go | 34 +++++++++++++--------- pkg/sentry/fsimpl/gofer/lisafs_dentry.go | 4 ++- pkg/sentry/fsimpl/gofer/revalidate.go | 4 ++- 5 files changed, 34 insertions(+), 16 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/directfs_dentry.go b/pkg/sentry/fsimpl/gofer/directfs_dentry.go index 9d2e8847a..375925007 100644 --- a/pkg/sentry/fsimpl/gofer/directfs_dentry.go +++ b/pkg/sentry/fsimpl/gofer/directfs_dentry.go @@ -528,8 +528,10 @@ func (d *directfsDentry) bindAt(ctx context.Context, name string, creds *auth.Cr hbep.ResetBoundSocketFD(ctx) return nil, err } - // Set the endpoint on the newly created child dentry. + // Set the endpoint on the newly created child dentry, and take the + // corresponding extra dentry reference. child.endpoint = opts.Endpoint + child.IncRef() return child, nil } diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go index a5e956c18..32a49dbd5 100644 --- a/pkg/sentry/fsimpl/gofer/filesystem.go +++ b/pkg/sentry/fsimpl/gofer/filesystem.go @@ -717,6 +717,8 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b if child.isSynthetic() { parent.syntheticChildren-- child.decRefNoCaching() + } else if child.endpoint != nil { + child.decRefNoCaching() } ds = appendDentry(ds, child) } @@ -1508,6 +1510,8 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa if replaced.isSynthetic() { newParent.syntheticChildren-- replaced.decRefNoCaching() + } else if replaced.endpoint != nil { + replaced.decRefNoCaching() } ds = appendDentry(ds, replaced) // Remove the replaced entry from its parent's cache. diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go index 19800e4ea..15cc818c7 100644 --- a/pkg/sentry/fsimpl/gofer/gofer.go +++ b/pkg/sentry/fsimpl/gofer/gofer.go @@ -722,7 +722,7 @@ func (fs *filesystem) Release(ctx context.Context) { // root dentry failed in GetFilesystem. if refs.GetLeakMode() != refs.NoLeakChecking && fs.root != nil { fs.renameMu.Lock() - fs.root.releaseSyntheticRecursiveLocked(ctx) + fs.root.releaseExtraRefsRecursiveLocked(ctx) fs.evictAllCachedDentriesLocked(ctx) fs.renameMu.Unlock() @@ -741,13 +741,14 @@ func (fs *filesystem) Release(ctx context.Context) { fs.vfsfs.VirtualFilesystem().PutAnonBlockDevMinor(fs.devMinor) } -// releaseSyntheticRecursiveLocked traverses the tree with root d and decrements -// the reference count on every synthetic dentry. Synthetic dentries have one -// reference for existence that should be dropped during filesystem.Release. +// releaseExtraRefsRecursiveLocked traverses the tree with root d and +// decrements the reference count on every synthetic dentry and dentry with +// endpoint != nil. Such dentries have one reference for existence that should +// be dropped during filesystem.Release. // // Precondition: d.fs.renameMu is locked for writing. -func (d *dentry) releaseSyntheticRecursiveLocked(ctx context.Context) { - if d.isSynthetic() { +func (d *dentry) releaseExtraRefsRecursiveLocked(ctx context.Context) { + if d.isSynthetic() || d.endpoint != nil { d.decRefNoCaching() d.checkCachingLocked(ctx, true /* renameMuWriteLocked */) } @@ -760,7 +761,7 @@ func (d *dentry) releaseSyntheticRecursiveLocked(ctx context.Context) { d.childrenMu.Unlock() for _, child := range children { if child != nil { - child.releaseSyntheticRecursiveLocked(ctx) + child.releaseExtraRefsRecursiveLocked(ctx) } } } @@ -799,10 +800,12 @@ type dentry struct { // refs is the reference count. Each dentry holds a reference on its // parent, even if disowned. An additional reference is held on all - // synthetic dentries until they are unlinked or invalidated. When refs - // reaches 0, the dentry may be added to the cache or destroyed. If refs == - // -1, the dentry has already been destroyed. refs is accessed using atomic - // memory operations. + // synthetic dentries, and all dentries for which endpoint is non-nil, + // until they are unlinked or invalidated. (Only a single additional + // reference is held on synthetic dentries that also have endpoint != nil.) + // When refs reaches 0, the dentry may be added to the cache or destroyed. + // If refs == -1, the dentry has already been destroyed. refs is accessed + // using atomic memory operations. refs atomicbitops.Int64 // fs is the owning filesystem. fs is immutable. @@ -984,8 +987,13 @@ type dentry struct { haveTarget bool target string - // If this dentry represents a synthetic socket file, endpoint is the - // transport endpoint bound to this file. + // If this dentry represents a socket file, endpoint is the transport + // endpoint bound to this file. + // + // endpoint often originates from vfs.MknodOptions.Endpoint, in which case + // it can't be recovered if the dentry is evicted from the dentry cache. + // Consequently, an extra reference is held on dentries for which endpoint + // is non-nil to prevent eviction. endpoint transport.BoundEndpoint // If this dentry represents a synthetic named pipe, pipe is the pipe diff --git a/pkg/sentry/fsimpl/gofer/lisafs_dentry.go b/pkg/sentry/fsimpl/gofer/lisafs_dentry.go index e9a5a1dd2..c1df8a7bd 100644 --- a/pkg/sentry/fsimpl/gofer/lisafs_dentry.go +++ b/pkg/sentry/fsimpl/gofer/lisafs_dentry.go @@ -415,8 +415,10 @@ func (d *lisafsDentry) mknod(ctx context.Context, name string, creds *auth.Crede hbep.ResetBoundSocketFD(ctx) return nil, err } - // Set the endpoint on the newly created child dentry. + // Set the endpoint on the newly created child dentry, and take the + // corresponding extra dentry reference. child.endpoint = opts.Endpoint + child.IncRef() return child, nil } diff --git a/pkg/sentry/fsimpl/gofer/revalidate.go b/pkg/sentry/fsimpl/gofer/revalidate.go index f93bf2014..4d720391c 100644 --- a/pkg/sentry/fsimpl/gofer/revalidate.go +++ b/pkg/sentry/fsimpl/gofer/revalidate.go @@ -239,6 +239,9 @@ func (d *dentry) invalidate(ctx context.Context, vfsObj *vfs.VirtualFilesystem, rc.DecRef(ctx) } d.decRefNoCaching() + if d.isSynthetic() || d.endpoint != nil { + d.decRefNoCaching() + } // Re-evaluate its caching status (i.e. if it has 0 references, drop it). // The dentry will be reloaded next time it's accessed. @@ -253,7 +256,6 @@ func (d *dentry) invalidate(ctx context.Context, vfsObj *vfs.VirtualFilesystem, // +checklocks:parent.childrenMu func (d *dentry) deleteSynthetic(parent *dentry, ds **[]*dentry) { d.setDeleted() - d.decRefNoCaching() *ds = appendDentry(*ds, d) parent.syntheticChildren-- parent.clearDirentsLocked()