From f66f0e235a0b788ed4faa5c9f033ed41ad76f159 Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Wed, 20 Nov 2024 19:26:49 -0800 Subject: [PATCH] Fix memmap.MappingIdentity.Device/InodeID() lock ordering. For vfs.FileDescriptions for which FileDescriptionOptions.UseDentryMetadata is true, memmap.MappingIdentity.Device/InodeID() => FileDescription.Stat() => FilesystemImpl.StatAt() takes fsimpl locks for path traversal, which violates the lock ordering and is unnecessary since no path is being traversed. Fix this by carving out a special case where FilesystemImpl.Stat() (and FileDescriptionImpl.Stat()) are required to meet the lock ordering requirements of memmap.MappingIdentity.Device/InodeID(), and implement that special case by skipping path traversal (and gofer revalidation) locks when not required. PiperOrigin-RevId: 698608924 --- pkg/sentry/fsimpl/gofer/filesystem.go | 6 ++++++ pkg/sentry/fsimpl/kernfs/filesystem.go | 4 ++++ pkg/sentry/fsimpl/overlay/filesystem.go | 19 +++++++++++++------ pkg/sentry/fsimpl/tmpfs/filesystem.go | 16 +++++++++++----- pkg/sentry/vfs/file_description.go | 3 +++ pkg/sentry/vfs/filesystem.go | 4 ++++ 6 files changed, 41 insertions(+), 11 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go index 87a6f37b4..a5e956c18 100644 --- a/pkg/sentry/fsimpl/gofer/filesystem.go +++ b/pkg/sentry/fsimpl/gofer/filesystem.go @@ -1578,6 +1578,12 @@ func (fs *filesystem) SetStatAt(ctx context.Context, rp *vfs.ResolvingPath, opts // StatAt implements vfs.FilesystemImpl.StatAt. func (fs *filesystem) StatAt(ctx context.Context, rp *vfs.ResolvingPath, opts vfs.StatOptions) (linux.Statx, error) { + if rp.Done() && opts.Sync == linux.AT_STATX_DONT_SYNC { + var stat linux.Statx + rp.Start().Impl().(*dentry).statTo(&stat) + return stat, nil + } + var ds *[]*dentry fs.renameMu.RLock() defer fs.renameMuRUnlockAndCheckCaching(ctx, &ds) diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go index 1297d0b3f..7caa2025d 100644 --- a/pkg/sentry/fsimpl/kernfs/filesystem.go +++ b/pkg/sentry/fsimpl/kernfs/filesystem.go @@ -915,6 +915,10 @@ func (fs *Filesystem) SetStatAt(ctx context.Context, rp *vfs.ResolvingPath, opts // StatAt implements vfs.FilesystemImpl.StatAt. func (fs *Filesystem) StatAt(ctx context.Context, rp *vfs.ResolvingPath, opts vfs.StatOptions) (linux.Statx, error) { + if rp.Done() && opts.Sync == linux.AT_STATX_DONT_SYNC { + return rp.Start().Impl().(*Dentry).inode.Stat(ctx, fs.VFSFilesystem(), opts) + } + fs.mu.RLock() defer fs.processDeferredDecRefs(ctx) defer fs.mu.RUnlock() diff --git a/pkg/sentry/fsimpl/overlay/filesystem.go b/pkg/sentry/fsimpl/overlay/filesystem.go index e760610e0..91a17a470 100644 --- a/pkg/sentry/fsimpl/overlay/filesystem.go +++ b/pkg/sentry/fsimpl/overlay/filesystem.go @@ -1548,17 +1548,24 @@ func (d *dentry) setStatLocked(ctx context.Context, rp *vfs.ResolvingPath, opts // StatAt implements vfs.FilesystemImpl.StatAt. func (fs *filesystem) StatAt(ctx context.Context, rp *vfs.ResolvingPath, opts vfs.StatOptions) (linux.Statx, error) { - var ds *[]*dentry - fs.renameMu.RLock() - defer fs.renameMuRUnlockAndCheckDrop(ctx, &ds) - d, err := fs.resolveLocked(ctx, rp, &ds) - if err != nil { - return linux.Statx{}, err + var d *dentry + if rp.Done() { + d = rp.Start().Impl().(*dentry) + } else { + var ds *[]*dentry + fs.renameMu.RLock() + defer fs.renameMuRUnlockAndCheckDrop(ctx, &ds) + var err error + d, err = fs.resolveLocked(ctx, rp, &ds) + if err != nil { + return linux.Statx{}, err + } } var stat linux.Statx if layerMask := opts.Mask &^ statInternalMask; layerMask != 0 { layerVD := d.topLayer() + var err error stat, err = fs.vfsfs.VirtualFilesystem().StatAt(ctx, fs.creds, &vfs.PathOperation{ Root: layerVD, Start: layerVD, diff --git a/pkg/sentry/fsimpl/tmpfs/filesystem.go b/pkg/sentry/fsimpl/tmpfs/filesystem.go index eb71dab70..4d33f2ec9 100644 --- a/pkg/sentry/fsimpl/tmpfs/filesystem.go +++ b/pkg/sentry/fsimpl/tmpfs/filesystem.go @@ -757,11 +757,17 @@ func (fs *filesystem) SetStatAt(ctx context.Context, rp *vfs.ResolvingPath, opts // StatAt implements vfs.FilesystemImpl.StatAt. func (fs *filesystem) StatAt(ctx context.Context, rp *vfs.ResolvingPath, opts vfs.StatOptions) (linux.Statx, error) { - fs.mu.RLock() - defer fs.mu.RUnlock() - d, err := resolveLocked(ctx, rp) - if err != nil { - return linux.Statx{}, err + var d *dentry + if rp.Done() { + d = rp.Start().Impl().(*dentry) + } else { + fs.mu.RLock() + defer fs.mu.RUnlock() + var err error + d, err = resolveLocked(ctx, rp) + if err != nil { + return linux.Statx{}, err + } } var stat linux.Statx d.inode.statTo(&stat) diff --git a/pkg/sentry/vfs/file_description.go b/pkg/sentry/vfs/file_description.go index fb6a3cc75..297903bfb 100644 --- a/pkg/sentry/vfs/file_description.go +++ b/pkg/sentry/vfs/file_description.go @@ -334,6 +334,9 @@ type FileDescriptionImpl interface { OnClose(ctx context.Context) error // Stat returns metadata for the file represented by the FileDescription. + // + // If opts.Sync == linux.AT_STATX_SYNC_DONT_SYNC, Stat cannot take locks + // preceding memmap.MappingIdentity locks. Stat(ctx context.Context, opts StatOptions) (linux.Statx, error) // SetStat updates metadata for the file represented by the diff --git a/pkg/sentry/vfs/filesystem.go b/pkg/sentry/vfs/filesystem.go index 8a7c763a0..d11970ed4 100644 --- a/pkg/sentry/vfs/filesystem.go +++ b/pkg/sentry/vfs/filesystem.go @@ -365,6 +365,10 @@ type FilesystemImpl interface { SetStatAt(ctx context.Context, rp *ResolvingPath, opts SetStatOptions) error // StatAt returns metadata for the file at rp. + // + // If rp.Done() (i.e. rp refers to the dentry rp.Start()) and opts.Sync == + // linux.AT_STATX_DONT_SYNC, StatAt cannot take locks preceding + // memmap.MappingIdentity locks. StatAt(ctx context.Context, rp *ResolvingPath, opts StatOptions) (linux.Statx, error) // StatFSAt returns metadata for the filesystem containing the file at rp.