From 2857afc5e4ec7d47f18ceaff02eecb59aaaa1b1d Mon Sep 17 00:00:00 2001 From: Adin Scannell Date: Sat, 13 Nov 2021 15:05:00 -0800 Subject: [PATCH] Drop final amutex uses. These amutex lock uses are limited to vfs1 and provide questionable utility. They protect only offset access, and not blocking operations. In order to completely remove amutex, drop these uses. The amutex package will be removed in a subsequent commit, which migrates other (less questionable) uses to a new Context API. PiperOrigin-RevId: 409716979 --- pkg/sentry/fs/BUILD | 1 - pkg/sentry/fs/file.go | 56 ++++++++--------------------------- pkg/sentry/fs/file_state.go | 1 - pkg/sentry/fs/splice.go | 33 ++++++--------------- pkg/sentry/fsimpl/tmpfs/BUILD | 1 - pkg/sentry/kernel/BUILD | 1 - 6 files changed, 22 insertions(+), 71 deletions(-) diff --git a/pkg/sentry/fs/BUILD b/pkg/sentry/fs/BUILD index 4e573d249..8774e8bfa 100644 --- a/pkg/sentry/fs/BUILD +++ b/pkg/sentry/fs/BUILD @@ -46,7 +46,6 @@ go_library( visibility = ["//pkg/sentry:internal"], deps = [ "//pkg/abi/linux", - "//pkg/amutex", "//pkg/context", "//pkg/errors/linuxerr", "//pkg/hostarch", diff --git a/pkg/sentry/fs/file.go b/pkg/sentry/fs/file.go index e5d542419..55d7bf809 100644 --- a/pkg/sentry/fs/file.go +++ b/pkg/sentry/fs/file.go @@ -18,7 +18,6 @@ import ( "math" "sync/atomic" - "gvisor.dev/gvisor/pkg/amutex" "gvisor.dev/gvisor/pkg/context" "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/refs" @@ -91,7 +90,7 @@ type File struct { // mu is dual-purpose: first, to make read(2) and write(2) thread-safe // in conformity with POSIX, and second, to cancel operations before they // begin in response to interruptions (i.e. signals). - mu amutex.AbortableMutex `state:"nosave"` + mu sync.Mutex `state:"nosave"` // FileOperations implements file system specific behavior for this File. FileOperations FileOperations `state:"wait"` @@ -113,7 +112,6 @@ func NewFile(ctx context.Context, dirent *Dirent, flags FileFlags, fops FileOper FileOperations: fops, flags: flags, } - f.mu.Init() f.EnableLeakCheck("fs.File") return &f } @@ -197,9 +195,7 @@ func (f *File) EventUnregister(e *waiter.Entry) { // // Returns linuxerr.ErrInterrupted if seeking was interrupted. func (f *File) Seek(ctx context.Context, whence SeekWhence, offset int64) (int64, error) { - if !f.mu.Lock(ctx) { - return 0, linuxerr.ErrInterrupted - } + f.mu.Lock() defer f.mu.Unlock() newOffset, err := f.FileOperations.Seek(ctx, f, whence, offset) @@ -219,9 +215,7 @@ func (f *File) Seek(ctx context.Context, whence SeekWhence, offset int64) (int64 // // Returns linuxerr.ErrInterrupted if reading was interrupted. func (f *File) Readdir(ctx context.Context, serializer DentrySerializer) error { - if !f.mu.Lock(ctx) { - return linuxerr.ErrInterrupted - } + f.mu.Lock() defer f.mu.Unlock() offset, err := f.FileOperations.Readdir(ctx, f, serializer) @@ -236,17 +230,13 @@ func (f *File) Readdir(ctx context.Context, serializer DentrySerializer) error { func (f *File) Readv(ctx context.Context, dst usermem.IOSequence) (int64, error) { start := fsmetric.StartReadWait() defer fsmetric.FinishReadWait(fsmetric.ReadWait, start) - - if !f.mu.Lock(ctx) { - return 0, linuxerr.ErrInterrupted - } - + f.mu.Lock() + defer f.mu.Unlock() fsmetric.Reads.Increment() n, err := f.FileOperations.Read(ctx, f, dst, f.offset) if n > 0 && !f.flags.NonSeekable { atomic.AddInt64(&f.offset, n) } - f.mu.Unlock() return n, err } @@ -258,14 +248,10 @@ func (f *File) Readv(ctx context.Context, dst usermem.IOSequence) (int64, error) func (f *File) Preadv(ctx context.Context, dst usermem.IOSequence, offset int64) (int64, error) { start := fsmetric.StartReadWait() defer fsmetric.FinishReadWait(fsmetric.ReadWait, start) - - if !f.mu.Lock(ctx) { - return 0, linuxerr.ErrInterrupted - } - + f.mu.Lock() + defer f.mu.Unlock() fsmetric.Reads.Increment() n, err := f.FileOperations.Read(ctx, f, dst, offset) - f.mu.Unlock() return n, err } @@ -278,15 +264,13 @@ func (f *File) Preadv(ctx context.Context, dst usermem.IOSequence, offset int64) // // Returns linuxerr.ErrInterrupted if writing was interrupted. func (f *File) Writev(ctx context.Context, src usermem.IOSequence) (int64, error) { - if !f.mu.Lock(ctx) { - return 0, linuxerr.ErrInterrupted - } + f.mu.Lock() + defer f.mu.Unlock() unlockAppendMu := f.Dirent.Inode.lockAppendMu(f.Flags().Append) // Handle append mode. if f.Flags().Append { if err := f.offsetForAppend(ctx, &f.offset); err != nil { unlockAppendMu() - f.mu.Unlock() return 0, err } } @@ -296,7 +280,6 @@ func (f *File) Writev(ctx context.Context, src usermem.IOSequence) (int64, error switch { case ok && limit == 0: unlockAppendMu() - f.mu.Unlock() return 0, linuxerr.ErrExceedsFileSizeLimit case ok: src = src.TakeFirst64(limit) @@ -308,7 +291,6 @@ func (f *File) Writev(ctx context.Context, src usermem.IOSequence) (int64, error atomic.StoreInt64(&f.offset, f.offset+n) } unlockAppendMu() - f.mu.Unlock() return n, err } @@ -383,11 +365,8 @@ func (f *File) checkLimit(ctx context.Context, offset int64) (int64, bool) { // // Returns linuxerr.ErrInterrupted if syncing was interrupted. func (f *File) Fsync(ctx context.Context, start int64, end int64, syncType SyncType) error { - if !f.mu.Lock(ctx) { - return linuxerr.ErrInterrupted - } + f.mu.Lock() defer f.mu.Unlock() - return f.FileOperations.Fsync(ctx, f, start, end, syncType) } @@ -395,11 +374,8 @@ func (f *File) Fsync(ctx context.Context, start int64, end int64, syncType SyncT // // Returns linuxerr.ErrInterrupted if syncing was interrupted. func (f *File) Flush(ctx context.Context) error { - if !f.mu.Lock(ctx) { - return linuxerr.ErrInterrupted - } + f.mu.Lock() defer f.mu.Unlock() - return f.FileOperations.Flush(ctx, f) } @@ -407,11 +383,8 @@ func (f *File) Flush(ctx context.Context) error { // // Returns linuxerr.ErrInterrupted if interrupted. func (f *File) ConfigureMMap(ctx context.Context, opts *memmap.MMapOpts) error { - if !f.mu.Lock(ctx) { - return linuxerr.ErrInterrupted - } + f.mu.Lock() defer f.mu.Unlock() - return f.FileOperations.ConfigureMMap(ctx, f, opts) } @@ -419,11 +392,8 @@ func (f *File) ConfigureMMap(ctx context.Context, opts *memmap.MMapOpts) error { // // Returns linuxerr.ErrInterrupted if interrupted. func (f *File) UnstableAttr(ctx context.Context) (UnstableAttr, error) { - if !f.mu.Lock(ctx) { - return UnstableAttr{}, linuxerr.ErrInterrupted - } + f.mu.Lock() defer f.mu.Unlock() - return f.FileOperations.UnstableAttr(ctx, f) } diff --git a/pkg/sentry/fs/file_state.go b/pkg/sentry/fs/file_state.go index 523182d59..a8d14357a 100644 --- a/pkg/sentry/fs/file_state.go +++ b/pkg/sentry/fs/file_state.go @@ -24,7 +24,6 @@ func (f *File) beforeSave() { // afterLoad is invoked by stateify. func (f *File) afterLoad() { - f.mu.Init() if f.flags.Async && f.async != nil { f.async.Register(f) } diff --git a/pkg/sentry/fs/splice.go b/pkg/sentry/fs/splice.go index 266140f6f..474b8ddde 100644 --- a/pkg/sentry/fs/splice.go +++ b/pkg/sentry/fs/splice.go @@ -25,6 +25,7 @@ import ( // Splice moves data to this file, directly from another. // // Offsets are updated only if DstOffset and SrcOffset are set. +// +checklocksignore func Splice(ctx context.Context, dst *File, src *File, opts SpliceOpts) (int64, error) { // Verify basic file flag permissions. if !dst.Flags().Write || !src.Flags().Read { @@ -53,44 +54,28 @@ func Splice(ctx context.Context, dst *File, src *File, opts SpliceOpts) (int64, switch { case dst.UniqueID < src.UniqueID: // Acquire dst first. - if !dst.mu.Lock(ctx) { - return 0, linuxerr.ErrInterrupted - } - if !src.mu.Lock(ctx) { - dst.mu.Unlock() - return 0, linuxerr.ErrInterrupted - } + dst.mu.Lock() + src.mu.Lock() case dst.UniqueID > src.UniqueID: // Acquire src first. - if !src.mu.Lock(ctx) { - return 0, linuxerr.ErrInterrupted - } - if !dst.mu.Lock(ctx) { - src.mu.Unlock() - return 0, linuxerr.ErrInterrupted - } + src.mu.Lock() + dst.mu.Lock() case dst.UniqueID == src.UniqueID: // Acquire only one lock; it's the same file. This is a // bit of a edge case, but presumably it's possible. - if !dst.mu.Lock(ctx) { - return 0, linuxerr.ErrInterrupted - } - srcLock = false // Only need one unlock. + dst.mu.Lock() + srcLock = false } // Use both offsets (locked). opts.DstStart = dst.offset opts.SrcStart = src.offset case dstLock: // Acquire only dst. - if !dst.mu.Lock(ctx) { - return 0, linuxerr.ErrInterrupted - } + dst.mu.Lock() opts.DstStart = dst.offset // Safe: locked. case srcLock: // Acquire only src. - if !src.mu.Lock(ctx) { - return 0, linuxerr.ErrInterrupted - } + src.mu.Lock() opts.SrcStart = src.offset // Safe: locked. } diff --git a/pkg/sentry/fsimpl/tmpfs/BUILD b/pkg/sentry/fsimpl/tmpfs/BUILD index 94486bb63..1e7a817e8 100644 --- a/pkg/sentry/fsimpl/tmpfs/BUILD +++ b/pkg/sentry/fsimpl/tmpfs/BUILD @@ -56,7 +56,6 @@ go_library( visibility = ["//pkg/sentry:internal"], deps = [ "//pkg/abi/linux", - "//pkg/amutex", "//pkg/context", "//pkg/errors/linuxerr", "//pkg/fspath", diff --git a/pkg/sentry/kernel/BUILD b/pkg/sentry/kernel/BUILD index f3f16eb7a..147c09adf 100644 --- a/pkg/sentry/kernel/BUILD +++ b/pkg/sentry/kernel/BUILD @@ -219,7 +219,6 @@ go_library( "//pkg/abi", "//pkg/abi/linux", "//pkg/abi/linux/errno", - "//pkg/amutex", "//pkg/bitmap", "//pkg/bits", "//pkg/bpf",