From ed9678b679dce0cec02d404812a28a136791d8f9 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Tue, 12 Mar 2024 20:03:43 -0700 Subject: [PATCH] Delete pgalloc.MemoryFileProvider. The work done in c087777e37a1 ("Plumb restore context to afterLoad()") makes pgalloc.MemoryFileProvider redundant as structs can now easily restore pgalloc.MemoryFile in stateify's afterLoad() method. This allows structs to have a pgalloc.MemoryFile field and use that directly, instead of going through the provided interface. This cleans up a lot of code and also should be more performant (avoids an interface method call on many hot paths). PiperOrigin-RevId: 615258927 --- pkg/sentry/contexttest/contexttest.go | 7 ----- pkg/sentry/fsimpl/gofer/gofer.go | 20 +++++++------- pkg/sentry/fsimpl/gofer/gofer_test.go | 2 +- pkg/sentry/fsimpl/gofer/regular_file.go | 18 ++++++------- pkg/sentry/fsimpl/gofer/save_restore.go | 6 +++++ pkg/sentry/fsimpl/iouringfs/iouringfs.go | 26 ++++++++----------- .../fsimpl/iouringfs/iouringfs_state.go | 9 +++++-- pkg/sentry/fsimpl/testutil/kernel.go | 6 ++--- pkg/sentry/fsimpl/tmpfs/tmpfs.go | 2 +- pkg/sentry/kernel/kcov.go | 12 ++++----- pkg/sentry/kernel/kernel.go | 9 ++----- pkg/sentry/kernel/shm/shm.go | 22 ++++++++++------ pkg/sentry/kernel/task_context.go | 2 -- pkg/sentry/kernel/task_image.go | 2 +- pkg/sentry/kernel/timekeeper.go | 4 +-- pkg/sentry/kernel/timekeeper_test.go | 6 ++--- pkg/sentry/kernel/vdso.go | 24 ++++++++++------- pkg/sentry/loader/vdso.go | 7 +++-- pkg/sentry/mm/aio_context.go | 16 ++++++------ pkg/sentry/mm/aio_context_state.go | 11 +++++++- pkg/sentry/mm/lifecycle.go | 6 ++--- pkg/sentry/mm/mm.go | 3 +-- pkg/sentry/mm/mm_test.go | 3 +-- pkg/sentry/mm/save_restore.go | 9 +++++-- pkg/sentry/mm/special_mappable.go | 20 +++++--------- pkg/sentry/pgalloc/context.go | 12 --------- pkg/sentry/pgalloc/save_restore.go | 16 ------------ runsc/boot/loader.go | 4 +-- 28 files changed, 132 insertions(+), 152 deletions(-) diff --git a/pkg/sentry/contexttest/contexttest.go b/pkg/sentry/contexttest/contexttest.go index 46caaebc5..4ed00774f 100644 --- a/pkg/sentry/contexttest/contexttest.go +++ b/pkg/sentry/contexttest/contexttest.go @@ -116,8 +116,6 @@ func (t *TestContext) Value(key any) any { return t.l case pgalloc.CtxMemoryFile: return t.mf - case pgalloc.CtxMemoryFileProvider: - return t case platform.CtxPlatform: return t.platform case uniqueid.CtxGlobalUniqueID: @@ -136,11 +134,6 @@ func (t *TestContext) Value(key any) any { } } -// MemoryFile implements pgalloc.MemoryFileProvider.MemoryFile. -func (t *TestContext) MemoryFile() *pgalloc.MemoryFile { - return t.mf -} - // RootContext returns a Context that may be used in tests that need root // credentials. Uses ptrace as the platform.Platform. func RootContext(tb testing.TB) context.Context { diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go index 103daabc3..917188abb 100644 --- a/pkg/sentry/fsimpl/gofer/gofer.go +++ b/pkg/sentry/fsimpl/gofer/gofer.go @@ -183,9 +183,9 @@ type FilesystemType struct{} type filesystem struct { vfsfs vfs.Filesystem - // mfp is used to allocate memory that caches regular file contents. mfp is + // mf is used to allocate memory that caches regular file contents. mf is // immutable. - mfp pgalloc.MemoryFileProvider + mf *pgalloc.MemoryFile `state:"nosave"` // Immutable options. opts filesystemOptions @@ -395,9 +395,9 @@ func (FilesystemType) Release(ctx context.Context) {} // GetFilesystem implements vfs.FilesystemType.GetFilesystem. func (fstype FilesystemType) GetFilesystem(ctx context.Context, vfsObj *vfs.VirtualFilesystem, creds *auth.Credentials, source string, opts vfs.GetFilesystemOptions) (*vfs.Filesystem, *vfs.Dentry, error) { - mfp := pgalloc.MemoryFileProviderFromContext(ctx) - if mfp == nil { - ctx.Warningf("gofer.FilesystemType.GetFilesystem: context does not provide a pgalloc.MemoryFileProvider") + mf := pgalloc.MemoryFileFromContext(ctx) + if mf == nil { + ctx.Warningf("gofer.FilesystemType.GetFilesystem: CtxMemoryFile is nil") return nil, nil, linuxerr.EINVAL } @@ -522,7 +522,7 @@ func (fstype FilesystemType) GetFilesystem(ctx context.Context, vfsObj *vfs.Virt return nil, nil, err } fs := &filesystem{ - mfp: mfp, + mf: mf, opts: fsopts, iopts: iopts, clock: ktime.RealtimeClockFromContext(ctx), @@ -660,7 +660,7 @@ func getFDFromMountOptionsMap(ctx context.Context, mopts map[string]string) (int func (fs *filesystem) Release(ctx context.Context) { fs.released.Store(1) - mf := fs.mfp.MemoryFile() + mf := fs.mf fs.syncMu.Lock() for elem := fs.syncableDentries.Front(); elem != nil; elem = elem.Next() { d := elem.d @@ -1385,7 +1385,7 @@ func (d *dentry) updateSizeAndUnlockDataMuLocked(newSize uint64) { // truncated pages have been removed from the remote file, they // should be dropped without being written back. d.dataMu.Lock() - d.cache.Truncate(newSize, d.fs.mfp.MemoryFile()) + d.cache.Truncate(newSize, d.fs.mf) d.dirty.KeepClean(memmap.MappableRange{newSize, oldpgend}) d.dataMu.Unlock() } @@ -1758,7 +1758,7 @@ func (d *dentry) evictLocked(ctx context.Context) { // destroyDisconnected destroys an uncached, unparented dentry. There are no // locking preconditions. func (d *dentry) destroyDisconnected(ctx context.Context) { - mf := d.fs.mfp.MemoryFile() + mf := d.fs.mf d.handleMu.Lock() d.dataMu.Lock() @@ -2080,7 +2080,7 @@ func (d *dentry) syncCachedFile(ctx context.Context, forFilesystemSync bool) err // Write back dirty pages to the remote file. d.dataMu.Lock() h := d.writeHandle() - err := fsutil.SyncDirtyAll(ctx, &d.cache, &d.dirty, d.size.Load(), d.fs.mfp.MemoryFile(), h.writeFromBlocksAt) + err := fsutil.SyncDirtyAll(ctx, &d.cache, &d.dirty, d.size.Load(), d.fs.mf, h.writeFromBlocksAt) d.dataMu.Unlock() if err != nil { return err diff --git a/pkg/sentry/fsimpl/gofer/gofer_test.go b/pkg/sentry/fsimpl/gofer/gofer_test.go index 90e0319e3..60e5097f1 100644 --- a/pkg/sentry/fsimpl/gofer/gofer_test.go +++ b/pkg/sentry/fsimpl/gofer/gofer_test.go @@ -27,7 +27,7 @@ import ( func TestDestroyIdempotent(t *testing.T) { ctx := contexttest.Context(t) fs := filesystem{ - mfp: pgalloc.MemoryFileProviderFromContext(ctx), + mf: pgalloc.MemoryFileFromContext(ctx), inoByKey: make(map[inoKey]uint64), clock: time.RealtimeClockFromContext(ctx), // Test relies on no dentry being held in the cache. diff --git a/pkg/sentry/fsimpl/gofer/regular_file.go b/pkg/sentry/fsimpl/gofer/regular_file.go index ad1bd7ae3..6c1ab5179 100644 --- a/pkg/sentry/fsimpl/gofer/regular_file.go +++ b/pkg/sentry/fsimpl/gofer/regular_file.go @@ -309,7 +309,7 @@ func (fd *regularFileFD) writeCache(ctx context.Context, d *dentry, offset int64 d.mapsMu.Unlock() // Finally free pages removed from the cache. - mf := d.fs.mfp.MemoryFile() + mf := d.fs.mf for _, freedFR := range freed { mf.DecRef(freedFR) } @@ -374,7 +374,7 @@ func (rw *dentryReadWriter) ReadToBlocks(dsts safemem.BlockSeq) (uint64, error) } // Otherwise read from/through the cache. - mf := rw.d.fs.mfp.MemoryFile() + mf := rw.d.fs.mf fillCache := mf.ShouldCacheEvictable() var dataMuUnlock func() if fillCache { @@ -501,7 +501,7 @@ func (rw *dentryReadWriter) WriteFromBlocks(srcs safemem.BlockSeq) (uint64, erro } // Otherwise write to/through the cache. - mf := rw.d.fs.mfp.MemoryFile() + mf := rw.d.fs.mf rw.d.dataMu.Lock() // Compute the range to write (overflow-checked). @@ -608,7 +608,7 @@ func (d *dentry) writeback(ctx context.Context, offset, size int64) error { return fsutil.SyncDirty(ctx, memmap.MappableRange{ Start: uint64(offset), End: uint64(end), - }, &d.cache, &d.dirty, dentrySize, d.fs.mfp.MemoryFile(), h.writeFromBlocksAt) + }, &d.cache, &d.dirty, dentrySize, d.fs.mf, h.writeFromBlocksAt) } // Seek implements vfs.FileDescriptionImpl.Seek. @@ -716,7 +716,7 @@ func (d *dentry) AddMapping(ctx context.Context, ms memmap.MappingSpace, ar host if d.fs.mayCachePagesInMemoryFile() { // d.Evict() will refuse to evict memory-mapped pages, so tell the // MemoryFile to not bother trying. - mf := d.fs.mfp.MemoryFile() + mf := d.fs.mf for _, r := range mapped { mf.MarkUnevictable(d, pgalloc.EvictableRange{r.Start, r.End}) } @@ -736,7 +736,7 @@ func (d *dentry) RemoveMapping(ctx context.Context, ms memmap.MappingSpace, ar h // Pages that are no longer referenced by any application memory // mappings are now considered unused; allow MemoryFile to evict them // when necessary. - mf := d.fs.mfp.MemoryFile() + mf := d.fs.mf d.dataMu.Lock() for _, r := range unmapped { // Since these pages are no longer mapped, they are no longer @@ -792,7 +792,7 @@ func (d *dentry) Translate(ctx context.Context, required, optional memmap.Mappab optional.End = pgend } - mf := d.fs.mfp.MemoryFile() + mf := d.fs.mf h := d.readHandle() _, cerr := d.cache.Fill(ctx, required, maxFillRange(required, optional), d.size.Load(), mf, usage.PageCache, pgalloc.AllocateAndWritePopulate, h.readToBlocksAt) @@ -862,7 +862,7 @@ func (d *dentry) InvalidateUnsavable(ctx context.Context) error { // Write the cache's contents back to the remote file so that if we have a // host fd after restore, the remote file's contents are coherent. - mf := d.fs.mfp.MemoryFile() + mf := d.fs.mf d.handleMu.RLock() defer d.handleMu.RUnlock() h := d.writeHandle() @@ -884,7 +884,7 @@ func (d *dentry) InvalidateUnsavable(ctx context.Context) error { // Evict implements pgalloc.EvictableMemoryUser.Evict. func (d *dentry) Evict(ctx context.Context, er pgalloc.EvictableRange) { mr := memmap.MappableRange{er.Start, er.End} - mf := d.fs.mfp.MemoryFile() + mf := d.fs.mf d.mapsMu.Lock() defer d.mapsMu.Unlock() d.handleMu.RLock() diff --git a/pkg/sentry/fsimpl/gofer/save_restore.go b/pkg/sentry/fsimpl/gofer/save_restore.go index ea537eb28..e16e11e7e 100644 --- a/pkg/sentry/fsimpl/gofer/save_restore.go +++ b/pkg/sentry/fsimpl/gofer/save_restore.go @@ -27,6 +27,7 @@ import ( "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/refs" "gvisor.dev/gvisor/pkg/safemem" + "gvisor.dev/gvisor/pkg/sentry/pgalloc" "gvisor.dev/gvisor/pkg/sentry/vfs" ) @@ -133,6 +134,11 @@ func (d *dentry) beforeSave() { } } +// afterLoad is invoked by stateify. +func (fs *filesystem) afterLoad(ctx goContext.Context) { + fs.mf = pgalloc.MemoryFileFromContext(ctx) +} + // afterLoad is invoked by stateify. func (d *dentry) afterLoad(goContext.Context) { d.readFD = atomicbitops.FromInt32(-1) diff --git a/pkg/sentry/fsimpl/iouringfs/iouringfs.go b/pkg/sentry/fsimpl/iouringfs/iouringfs.go index 635cf764a..f7ecebdda 100644 --- a/pkg/sentry/fsimpl/iouringfs/iouringfs.go +++ b/pkg/sentry/fsimpl/iouringfs/iouringfs.go @@ -50,7 +50,7 @@ type FileDescription struct { vfs.DentryMetadataFileDescriptionImpl vfs.NoLockFD - mfp pgalloc.MemoryFileProvider + mf *pgalloc.MemoryFile `state:"nosave"` rbmf ringsBufferFile sqemf sqEntriesFile @@ -95,9 +95,9 @@ func New(ctx context.Context, vfsObj *vfs.VirtualFilesystem, entries uint32, par vd := vfsObj.NewAnonVirtualDentry("[io_uring]") defer vd.DecRef(ctx) - mfp := pgalloc.MemoryFileProviderFromContext(ctx) - if mfp == nil { - panic(fmt.Sprintf("context.Context %T lacks non-nil value for key %T", ctx, pgalloc.CtxMemoryFileProvider)) + mf := pgalloc.MemoryFileFromContext(ctx) + if mf == nil { + panic(fmt.Sprintf("context.Context %T lacks non-nil value for key %T", ctx, pgalloc.CtxMemoryFile)) } numSqEntries, ok := roundUpPowerOfTwo(entries) @@ -123,7 +123,6 @@ func New(ctx context.Context, vfsObj *vfs.VirtualFilesystem, entries uint32, par numSqEntries*uint32((*linux.IORingIndex)(nil).SizeBytes())) ringsBufferSize = uint64(hostarch.Addr(ringsBufferSize).MustRoundUp()) - mf := mfp.MemoryFile() memCgID := pgalloc.MemoryCgroupIDFromContext(ctx) rbfr, err := mf.Allocate(ringsBufferSize, pgalloc.AllocOpts{Kind: usage.Anonymous, MemCgID: memCgID}) if err != nil { @@ -139,7 +138,7 @@ func New(ctx context.Context, vfsObj *vfs.VirtualFilesystem, entries uint32, par } iouringfd := &FileDescription{ - mfp: mfp, + mf: mf, rbmf: ringsBufferFile{ fr: rbfr, }, @@ -215,18 +214,15 @@ func New(ctx context.Context, vfsObj *vfs.VirtualFilesystem, entries uint32, par // Release implements vfs.FileDescriptionImpl.Release. func (fd *FileDescription) Release(ctx context.Context) { - mf := pgalloc.MemoryFileProviderFromContext(ctx).MemoryFile() - mf.DecRef(fd.rbmf.fr) - mf.DecRef(fd.sqemf.fr) + fd.mf.DecRef(fd.rbmf.fr) + fd.mf.DecRef(fd.sqemf.fr) } // mapSharedBuffers caches internal mappings for the ring's shared memory // regions. func (fd *FileDescription) mapSharedBuffers() error { - mf := fd.mfp.MemoryFile() - // Mapping for the IORings header struct. - rb, err := mf.MapInternal(fd.rbmf.fr, hostarch.ReadWrite) + rb, err := fd.mf.MapInternal(fd.rbmf.fr, hostarch.ReadWrite) if err != nil { return err } @@ -242,7 +238,7 @@ func (fd *FileDescription) mapSharedBuffers() error { fd.cqesBuf.init(cqes) // Mapping for the SQEs array. - sqes, err := mf.MapInternal(fd.sqemf.fr, hostarch.ReadWrite) + sqes, err := fd.mf.MapInternal(fd.sqemf.fr, hostarch.ReadWrite) if err != nil { return err } @@ -572,7 +568,7 @@ func (sqemf *sqEntriesFile) Translate(ctx context.Context, required, optional me return []memmap.Translation{ { Source: source, - File: pgalloc.MemoryFileProviderFromContext(ctx).MemoryFile(), + File: pgalloc.MemoryFileFromContext(ctx), Offset: sqemf.fr.Start + source.Start, Perms: at, }, @@ -618,7 +614,7 @@ func (rbmf *ringsBufferFile) Translate(ctx context.Context, required, optional m return []memmap.Translation{ { Source: source, - File: pgalloc.MemoryFileProviderFromContext(ctx).MemoryFile(), + File: pgalloc.MemoryFileFromContext(ctx), Offset: rbmf.fr.Start + source.Start, Perms: at, }, diff --git a/pkg/sentry/fsimpl/iouringfs/iouringfs_state.go b/pkg/sentry/fsimpl/iouringfs/iouringfs_state.go index 4a57dcb82..26ed5d210 100644 --- a/pkg/sentry/fsimpl/iouringfs/iouringfs_state.go +++ b/pkg/sentry/fsimpl/iouringfs/iouringfs_state.go @@ -14,7 +14,11 @@ package iouringfs -import "context" +import ( + "context" + + "gvisor.dev/gvisor/pkg/sentry/pgalloc" +) // beforeSave is invoked by stateify. func (fd *FileDescription) beforeSave() { @@ -24,7 +28,8 @@ func (fd *FileDescription) beforeSave() { } // afterLoad is invoked by stateify. -func (fd *FileDescription) afterLoad(context.Context) { +func (fd *FileDescription) afterLoad(ctx context.Context) { + fd.mf = pgalloc.MemoryFileFromContext(ctx) // Remap shared buffers. fd.remap = true fd.runC = make(chan struct{}, 1) diff --git a/pkg/sentry/fsimpl/testutil/kernel.go b/pkg/sentry/fsimpl/testutil/kernel.go index 3c9db90d5..5cf98481f 100644 --- a/pkg/sentry/fsimpl/testutil/kernel.go +++ b/pkg/sentry/fsimpl/testutil/kernel.go @@ -77,13 +77,13 @@ func Boot() (*kernel.Kernel, error) { k.SetMemoryFile(mf) // Pass k as the platform since it is savable, unlike the actual platform. - vdso, err := loader.PrepareVDSO(k) + vdso, err := loader.PrepareVDSO(k.MemoryFile()) if err != nil { return nil, fmt.Errorf("creating vdso: %v", err) } // Create timekeeper. - tk := kernel.NewTimekeeper(k, vdso.ParamPage.FileRange()) + tk := kernel.NewTimekeeper(k.MemoryFile(), vdso.ParamPage.FileRange()) tk.SetClocks(time.NewCalibratedClocks()) creds := auth.NewRootCredentials(auth.NewRootUserNamespace()) @@ -129,7 +129,7 @@ func CreateTask(ctx context.Context, name string, tc *kernel.ThreadGroup, mntns if err != nil { return nil, err } - m := mm.NewMemoryManager(k, k, k.SleepForAddressSpaceActivation) + m := mm.NewMemoryManager(k, k.MemoryFile(), k.SleepForAddressSpaceActivation) m.SetExecutable(ctx, exe) creds := auth.CredentialsFromContext(ctx) diff --git a/pkg/sentry/fsimpl/tmpfs/tmpfs.go b/pkg/sentry/fsimpl/tmpfs/tmpfs.go index f0996b7ec..5c698b5cb 100644 --- a/pkg/sentry/fsimpl/tmpfs/tmpfs.go +++ b/pkg/sentry/fsimpl/tmpfs/tmpfs.go @@ -134,7 +134,7 @@ type FilesystemOpts struct { MaxFilenameLen int // MemoryFile is the memory file that will be used to store file data. If - // this is nil, then MemoryFileProviderFromContext() is used. + // this is nil, then MemoryFileFromContext() is used. MemoryFile *pgalloc.MemoryFile // DisableDefaultSizeLimit disables setting a default size limit. In Linux, diff --git a/pkg/sentry/kernel/kcov.go b/pkg/sentry/kernel/kcov.go index 19b81ad7d..a1a198833 100644 --- a/pkg/sentry/kernel/kcov.go +++ b/pkg/sentry/kernel/kcov.go @@ -41,8 +41,8 @@ const kcovAreaSizeMax = 10 * 1024 * 1024 // To give the illusion that the data is always up to date, we update the shared // memory every time before we return to userspace. type Kcov struct { - // mfp provides application memory. It is immutable after creation. - mfp pgalloc.MemoryFileProvider + // mf stores application memory. It is immutable after creation. + mf *pgalloc.MemoryFile // mu protects all of the fields below. mu sync.RWMutex @@ -74,7 +74,7 @@ type Kcov struct { // NewKcov creates and returns a Kcov instance. func (k *Kernel) NewKcov() *Kcov { return &Kcov{ - mfp: k, + mf: k.mf, } } @@ -94,7 +94,7 @@ func (kcov *Kcov) TaskWork(t *Task) { } rw := &kcovReadWriter{ - mf: kcov.mfp.MemoryFile(), + mf: kcov.mf, fr: kcov.mappable.FileRange(), } @@ -246,7 +246,7 @@ func (kcov *Kcov) ConfigureMMap(ctx context.Context, opts *memmap.MMapOpts) erro Kind: usage.Anonymous, MemCgID: pgalloc.MemoryCgroupIDFromContext(ctx), } - fr, err := kcov.mfp.MemoryFile().Allocate(kcov.size*8, opts) + fr, err := kcov.mf.Allocate(kcov.size*8, opts) if err != nil { return err } @@ -258,7 +258,7 @@ func (kcov *Kcov) ConfigureMMap(ctx context.Context, opts *memmap.MMapOpts) erro } // For convenience, a special mappable is used here. Note that these mappings // will look different under /proc/[pid]/maps than they do on Linux. - kcov.mappable = mm.NewSpecialMappable(fmt.Sprintf("[kcov:%d]", t.ThreadID()), kcov.mfp, fr) + kcov.mappable = mm.NewSpecialMappable(fmt.Sprintf("[kcov:%d]", t.ThreadID()), kcov.mf, fr) } kcov.mappable.IncRef() opts.Mappable = kcov.mappable diff --git a/pkg/sentry/kernel/kernel.go b/pkg/sentry/kernel/kernel.go index 4b49f0511..d0109772c 100644 --- a/pkg/sentry/kernel/kernel.go +++ b/pkg/sentry/kernel/kernel.go @@ -142,8 +142,7 @@ type Kernel struct { // All of the following fields are immutable unless otherwise specified. // Platform is the platform that is used to execute tasks in the created - // Kernel. See comment on pgalloc.MemoryFileProvider for why Platform is - // embedded anonymously (the same issue applies). + // Kernel. platform.Platform `state:"nosave"` // mf provides application memory. @@ -894,8 +893,6 @@ func (ctx *createProcessContext) Value(key any) any { return ctx.getMemoryCgroupID() case pgalloc.CtxMemoryFile: return ctx.kernel.mf - case pgalloc.CtxMemoryFileProvider: - return ctx.kernel case platform.CtxPlatform: return ctx.kernel case uniqueid.CtxGlobalUniqueID: @@ -1533,7 +1530,7 @@ func (k *Kernel) SetMemoryFile(mf *pgalloc.MemoryFile) { k.mf = mf } -// MemoryFile implements pgalloc.MemoryFileProvider.MemoryFile. +// MemoryFile returns the MemoryFile that provides application memory. func (k *Kernel) MemoryFile() *pgalloc.MemoryFile { return k.mf } @@ -1671,8 +1668,6 @@ func (ctx *supervisorContext) Value(key any) any { return limits.NewLimitSet() case pgalloc.CtxMemoryFile: return ctx.Kernel.mf - case pgalloc.CtxMemoryFileProvider: - return ctx.Kernel case platform.CtxPlatform: return ctx.Kernel case uniqueid.CtxGlobalUniqueID: diff --git a/pkg/sentry/kernel/shm/shm.go b/pkg/sentry/kernel/shm/shm.go index 847680ffb..c6e9c2440 100644 --- a/pkg/sentry/kernel/shm/shm.go +++ b/pkg/sentry/kernel/shm/shm.go @@ -34,6 +34,7 @@ package shm import ( + goContext "context" "fmt" "gvisor.dev/gvisor/pkg/abi/linux" @@ -201,9 +202,9 @@ func (r *Registry) FindOrCreate(ctx context.Context, pid int32, key ipc.Key, siz // // Precondition: Caller must hold r.mu. func (r *Registry) newShmLocked(ctx context.Context, pid int32, key ipc.Key, creator *auth.Credentials, mode linux.FileMode, size uint64) (*Shm, error) { - mfp := pgalloc.MemoryFileProviderFromContext(ctx) - if mfp == nil { - panic(fmt.Sprintf("context.Context %T lacks non-nil value for key %T", ctx, pgalloc.CtxMemoryFileProvider)) + mf := pgalloc.MemoryFileFromContext(ctx) + if mf == nil { + panic(fmt.Sprintf("context.Context %T lacks non-nil value for key %T", ctx, pgalloc.CtxMemoryFile)) } devID, ok := deviceIDFromContext(ctx) if !ok { @@ -211,13 +212,13 @@ func (r *Registry) newShmLocked(ctx context.Context, pid int32, key ipc.Key, cre } effectiveSize := uint64(hostarch.Addr(size).MustRoundUp()) - fr, err := mfp.MemoryFile().Allocate(effectiveSize, pgalloc.AllocOpts{Kind: usage.Anonymous, MemCgID: pgalloc.MemoryCgroupIDFromContext(ctx)}) + fr, err := mf.Allocate(effectiveSize, pgalloc.AllocOpts{Kind: usage.Anonymous, MemCgID: pgalloc.MemoryCgroupIDFromContext(ctx)}) if err != nil { return nil, err } shm := &Shm{ - mfp: mfp, + mf: mf, registry: r, devID: devID, size: size, @@ -331,7 +332,7 @@ type Shm struct { // via MappingIdentity. ShmRefs - mfp pgalloc.MemoryFileProvider + mf *pgalloc.MemoryFile `state:"nosave"` // registry points to the shm registry containing this segment. Immutable. registry *Registry @@ -379,6 +380,11 @@ type Shm struct { pendingDestruction bool } +// afterLoad is invoked by stateify. +func (s *Shm) afterLoad(ctx goContext.Context) { + s.mf = pgalloc.MemoryFileFromContext(ctx) +} + // ID returns object's ID. func (s *Shm) ID() ipc.ID { return s.obj.ID @@ -436,7 +442,7 @@ func (s *Shm) InodeID() uint64 { // Precondition: Caller must not hold s.mu. func (s *Shm) DecRef(ctx context.Context) { s.ShmRefs.DecRef(func() { - s.mfp.MemoryFile().DecRef(s.fr) + s.mf.DecRef(s.fr) s.registry.remove(s) }) } @@ -498,7 +504,7 @@ func (s *Shm) Translate(ctx context.Context, required, optional memmap.MappableR return []memmap.Translation{ { Source: source, - File: s.mfp.MemoryFile(), + File: s.mf, Offset: s.fr.Start + source.Start, Perms: hostarch.AnyAccess, }, diff --git a/pkg/sentry/kernel/task_context.go b/pkg/sentry/kernel/task_context.go index 1e391c281..b1c1f667a 100644 --- a/pkg/sentry/kernel/task_context.go +++ b/pkg/sentry/kernel/task_context.go @@ -120,8 +120,6 @@ func (t *Task) contextValue(key any, isTaskGoroutine bool) any { return t.memCgID.Load() case pgalloc.CtxMemoryFile: return t.k.mf - case pgalloc.CtxMemoryFileProvider: - return t.k case platform.CtxPlatform: return t.k case shm.CtxDeviceID: diff --git a/pkg/sentry/kernel/task_image.go b/pkg/sentry/kernel/task_image.go index 18b674b72..e7fcffedd 100644 --- a/pkg/sentry/kernel/task_image.go +++ b/pkg/sentry/kernel/task_image.go @@ -146,7 +146,7 @@ func (t *Task) Stack() *arch.Stack { // args.MemoryManager does not need to be set by the caller. func (k *Kernel) LoadTaskImage(ctx context.Context, args loader.LoadArgs) (*TaskImage, *syserr.Error) { // Prepare a new user address space to load into. - m := mm.NewMemoryManager(k, k, k.SleepForAddressSpaceActivation) + m := mm.NewMemoryManager(k, k.mf, k.SleepForAddressSpaceActivation) defer m.DecUsers(ctx) args.MemoryManager = m diff --git a/pkg/sentry/kernel/timekeeper.go b/pkg/sentry/kernel/timekeeper.go index 5560b48d2..285e52543 100644 --- a/pkg/sentry/kernel/timekeeper.go +++ b/pkg/sentry/kernel/timekeeper.go @@ -97,9 +97,9 @@ type Timekeeper struct { // NewTimekeeper does not take ownership of paramPage. // // SetClocks must be called on the returned Timekeeper before it is usable. -func NewTimekeeper(mfp pgalloc.MemoryFileProvider, paramPage memmap.FileRange) *Timekeeper { +func NewTimekeeper(mf *pgalloc.MemoryFile, paramPage memmap.FileRange) *Timekeeper { t := Timekeeper{ - params: NewVDSOParamPage(mfp, paramPage), + params: NewVDSOParamPage(mf, paramPage), } t.realtimeClock = &timekeeperClock{tk: &t, c: sentrytime.Realtime} t.monotonicClock = &timekeeperClock{tk: &t, c: sentrytime.Monotonic} diff --git a/pkg/sentry/kernel/timekeeper_test.go b/pkg/sentry/kernel/timekeeper_test.go index 18358561d..e8720566f 100644 --- a/pkg/sentry/kernel/timekeeper_test.go +++ b/pkg/sentry/kernel/timekeeper_test.go @@ -53,13 +53,13 @@ func (c *mockClocks) GetTime(id sentrytime.ClockID) (int64, error) { // SetClocks called. func stateTestClocklessTimekeeper(tb testing.TB) *Timekeeper { ctx := contexttest.Context(tb) - mfp := pgalloc.MemoryFileProviderFromContext(ctx) - fr, err := mfp.MemoryFile().Allocate(hostarch.PageSize, pgalloc.AllocOpts{Kind: usage.Anonymous}) + mf := pgalloc.MemoryFileFromContext(ctx) + fr, err := mf.Allocate(hostarch.PageSize, pgalloc.AllocOpts{Kind: usage.Anonymous}) if err != nil { tb.Fatalf("failed to allocate memory: %v", err) } return &Timekeeper{ - params: NewVDSOParamPage(mfp, fr), + params: NewVDSOParamPage(mf, fr), } } diff --git a/pkg/sentry/kernel/vdso.go b/pkg/sentry/kernel/vdso.go index 7011f60fd..0e2f5fd38 100644 --- a/pkg/sentry/kernel/vdso.go +++ b/pkg/sentry/kernel/vdso.go @@ -15,6 +15,7 @@ package kernel import ( + "context" "fmt" "gvisor.dev/gvisor/pkg/hostarch" @@ -57,9 +58,9 @@ type vdsoParams struct { // // +stateify savable type VDSOParamPage struct { - // The parameter page is fr, allocated from mfp.MemoryFile(). - mfp pgalloc.MemoryFileProvider - fr memmap.FileRange + // The parameter page is fr, allocated from mf. + mf *pgalloc.MemoryFile `state:"nosave"` + fr memmap.FileRange // seq is the current sequence count written to the page. // @@ -78,17 +79,22 @@ type VDSOParamPage struct { copyScratchBuffer []byte } +// afterLoad is invoked by stateify. +func (v *VDSOParamPage) afterLoad(ctx context.Context) { + v.mf = pgalloc.MemoryFileFromContext(ctx) +} + // NewVDSOParamPage returns a VDSOParamPage. // // Preconditions: -// - fr is a single page allocated from mfp.MemoryFile(). VDSOParamPage does -// not take ownership of fr; it must remain allocated for the lifetime of the +// - fr is a single page allocated from mf. VDSOParamPage does not take +// ownership of fr; it must remain allocated for the lifetime of the // VDSOParamPage. // - VDSOParamPage must be the only writer to fr. -// - mfp.MemoryFile().MapInternal(fr) must return a single safemem.Block. -func NewVDSOParamPage(mfp pgalloc.MemoryFileProvider, fr memmap.FileRange) *VDSOParamPage { +// - mf.MapInternal(fr) must return a single safemem.Block. +func NewVDSOParamPage(mf *pgalloc.MemoryFile, fr memmap.FileRange) *VDSOParamPage { return &VDSOParamPage{ - mfp: mfp, + mf: mf, fr: fr, copyScratchBuffer: make([]byte, (*vdsoParams)(nil).SizeBytes()), } @@ -96,7 +102,7 @@ func NewVDSOParamPage(mfp pgalloc.MemoryFileProvider, fr memmap.FileRange) *VDSO // access returns a mapping of the param page. func (v *VDSOParamPage) access() (safemem.Block, error) { - bs, err := v.mfp.MemoryFile().MapInternal(v.fr, hostarch.ReadWrite) + bs, err := v.mf.MapInternal(v.fr, hostarch.ReadWrite) if err != nil { return safemem.Block{}, err } diff --git a/pkg/sentry/loader/vdso.go b/pkg/sentry/loader/vdso.go index 2f4a1477c..e3696f9b5 100644 --- a/pkg/sentry/loader/vdso.go +++ b/pkg/sentry/loader/vdso.go @@ -179,7 +179,7 @@ type VDSO struct { // PrepareVDSO validates the system VDSO and returns a VDSO, containing the // param page for updating by the kernel. -func PrepareVDSO(mfp pgalloc.MemoryFileProvider) (*VDSO, error) { +func PrepareVDSO(mf *pgalloc.MemoryFile) (*VDSO, error) { vdsoFile := &byteFullReader{data: vdsodata.Binary} // First make sure the VDSO is valid. vdsoFile does not use ctx, so a @@ -195,7 +195,6 @@ func PrepareVDSO(mfp pgalloc.MemoryFileProvider) (*VDSO, error) { return nil, fmt.Errorf("VDSO size overflows? %#x", len(vdsodata.Binary)) } - mf := mfp.MemoryFile() vdso, err := mf.Allocate(uint64(size), pgalloc.AllocOpts{Kind: usage.System}) if err != nil { return nil, fmt.Errorf("unable to allocate VDSO memory: %v", err) @@ -221,11 +220,11 @@ func PrepareVDSO(mfp pgalloc.MemoryFileProvider) (*VDSO, error) { } return &VDSO{ - ParamPage: mm.NewSpecialMappable("[vvar]", mfp, paramPage), + ParamPage: mm.NewSpecialMappable("[vvar]", mf, paramPage), // TODO(gvisor.dev/issue/157): Don't advertise the VDSO, as // some applications may not be able to handle multiple [vdso] // hints. - vdso: mm.NewSpecialMappable("", mfp, vdso), + vdso: mm.NewSpecialMappable("", mf, vdso), os: info.os, arch: info.arch, phdrs: info.phdrs, diff --git a/pkg/sentry/mm/aio_context.go b/pkg/sentry/mm/aio_context.go index f5210b48c..185b94e49 100644 --- a/pkg/sentry/mm/aio_context.go +++ b/pkg/sentry/mm/aio_context.go @@ -246,18 +246,18 @@ func (aio *AIOContext) Drain() { type aioMappable struct { aioMappableRefs - mfp pgalloc.MemoryFileProvider - fr memmap.FileRange + mf *pgalloc.MemoryFile `state:"nosave"` + fr memmap.FileRange } var aioRingBufferSize = uint64(hostarch.Addr(linux.AIORingSize).MustRoundUp()) -func newAIOMappable(ctx context.Context, mfp pgalloc.MemoryFileProvider) (*aioMappable, error) { - fr, err := mfp.MemoryFile().Allocate(aioRingBufferSize, pgalloc.AllocOpts{Kind: usage.Anonymous, MemCgID: pgalloc.MemoryCgroupIDFromContext(ctx)}) +func newAIOMappable(ctx context.Context, mf *pgalloc.MemoryFile) (*aioMappable, error) { + fr, err := mf.Allocate(aioRingBufferSize, pgalloc.AllocOpts{Kind: usage.Anonymous, MemCgID: pgalloc.MemoryCgroupIDFromContext(ctx)}) if err != nil { return nil, err } - m := aioMappable{mfp: mfp, fr: fr} + m := aioMappable{mf: mf, fr: fr} m.InitRefs() return &m, nil } @@ -265,7 +265,7 @@ func newAIOMappable(ctx context.Context, mfp pgalloc.MemoryFileProvider) (*aioMa // DecRef implements refs.RefCounter.DecRef. func (m *aioMappable) DecRef(ctx context.Context) { m.aioMappableRefs.DecRef(func() { - m.mfp.MemoryFile().DecRef(m.fr) + m.mf.DecRef(m.fr) }) } @@ -346,7 +346,7 @@ func (m *aioMappable) Translate(ctx context.Context, required, optional memmap.M return []memmap.Translation{ { Source: source, - File: m.mfp.MemoryFile(), + File: m.mf, Offset: m.fr.Start + source.Start, Perms: hostarch.AnyAccess, }, @@ -368,7 +368,7 @@ func (mm *MemoryManager) NewAIOContext(ctx context.Context, events uint32) (uint // libaio peeks inside looking for a magic number. This function allocates // a page per context and keeps it set to zeroes to ensure it will not // match AIO_RING_MAGIC and make libaio happy. - m, err := newAIOMappable(ctx, mm.mfp) + m, err := newAIOMappable(ctx, mm.mf) if err != nil { return 0, err } diff --git a/pkg/sentry/mm/aio_context_state.go b/pkg/sentry/mm/aio_context_state.go index 61ebe8af0..c60de6575 100644 --- a/pkg/sentry/mm/aio_context_state.go +++ b/pkg/sentry/mm/aio_context_state.go @@ -14,9 +14,18 @@ package mm -import "context" +import ( + "context" + + "gvisor.dev/gvisor/pkg/sentry/pgalloc" +) // afterLoad is invoked by stateify. func (aio *AIOContext) afterLoad(context.Context) { aio.requestReady = make(chan struct{}, 1) } + +// afterLoad is invoked by stateify. +func (m *aioMappable) afterLoad(ctx context.Context) { + m.mf = pgalloc.MemoryFileFromContext(ctx) +} diff --git a/pkg/sentry/mm/lifecycle.go b/pkg/sentry/mm/lifecycle.go index ae0d2d282..0b5a836fe 100644 --- a/pkg/sentry/mm/lifecycle.go +++ b/pkg/sentry/mm/lifecycle.go @@ -28,11 +28,10 @@ import ( ) // NewMemoryManager returns a new MemoryManager with no mappings and 1 user. -func NewMemoryManager(p platform.Platform, mfp pgalloc.MemoryFileProvider, sleepForActivation bool) *MemoryManager { +func NewMemoryManager(p platform.Platform, mf *pgalloc.MemoryFile, sleepForActivation bool) *MemoryManager { return &MemoryManager{ p: p, - mfp: mfp, - mf: mfp.MemoryFile(), + mf: mf, haveASIO: p.SupportsAddressSpaceIO(), users: atomicbitops.FromInt32(1), auxv: arch.Auxv{}, @@ -74,7 +73,6 @@ func (mm *MemoryManager) Fork(ctx context.Context) (*MemoryManager, error) { defer mm.mappingMu.RUnlock() mm2 := &MemoryManager{ p: mm.p, - mfp: mm.mfp, mf: mm.mf, haveASIO: mm.haveASIO, layout: mm.layout, diff --git a/pkg/sentry/mm/mm.go b/pkg/sentry/mm/mm.go index 033430ad6..dc1431889 100644 --- a/pkg/sentry/mm/mm.go +++ b/pkg/sentry/mm/mm.go @@ -57,8 +57,7 @@ type MapsCallbackFunc func(start, end hostarch.Addr, permissions hostarch.Access // +stateify savable type MemoryManager struct { // p and mfp are immutable. - p platform.Platform - mfp pgalloc.MemoryFileProvider + p platform.Platform // mf is the cached result of mfp.MemoryFile(). // diff --git a/pkg/sentry/mm/mm_test.go b/pkg/sentry/mm/mm_test.go index 4fced2730..aafae4454 100644 --- a/pkg/sentry/mm/mm_test.go +++ b/pkg/sentry/mm/mm_test.go @@ -31,8 +31,7 @@ import ( func testMemoryManager(ctx context.Context) *MemoryManager { p := platform.FromContext(ctx) - mfp := pgalloc.MemoryFileProviderFromContext(ctx) - mm := NewMemoryManager(p, mfp, false) + mm := NewMemoryManager(p, pgalloc.MemoryFileFromContext(ctx), false) mm.layout = arch.MmapLayout{ MinAddr: p.MinUserAddress(), MaxAddr: p.MaxUserAddress(), diff --git a/pkg/sentry/mm/save_restore.go b/pkg/sentry/mm/save_restore.go index 8ca3e3c33..6f2e0ceb7 100644 --- a/pkg/sentry/mm/save_restore.go +++ b/pkg/sentry/mm/save_restore.go @@ -38,11 +38,16 @@ func (mm *MemoryManager) InvalidateUnsavable(ctx context.Context) error { } // afterLoad is invoked by stateify. -func (mm *MemoryManager) afterLoad(goContext.Context) { - mm.mf = mm.mfp.MemoryFile() +func (mm *MemoryManager) afterLoad(ctx goContext.Context) { + mm.mf = pgalloc.MemoryFileFromContext(ctx) mm.haveASIO = mm.p.SupportsAddressSpaceIO() } +// afterLoad is invoked by stateify. +func (m *SpecialMappable) afterLoad(ctx goContext.Context) { + m.mf = pgalloc.MemoryFileFromContext(ctx) +} + const ( vmaRealPermsRead = 1 << iota vmaRealPermsWrite diff --git a/pkg/sentry/mm/special_mappable.go b/pkg/sentry/mm/special_mappable.go index d0270aba2..fe1148ab9 100644 --- a/pkg/sentry/mm/special_mappable.go +++ b/pkg/sentry/mm/special_mappable.go @@ -31,7 +31,7 @@ import ( type SpecialMappable struct { SpecialMappableRefs - mfp pgalloc.MemoryFileProvider + mf *pgalloc.MemoryFile `state:"nosave"` fr memmap.FileRange name string } @@ -41,8 +41,8 @@ type SpecialMappable struct { // SpecialMappable will use the given name in /proc/[pid]/maps. // // Preconditions: fr.Length() != 0. -func NewSpecialMappable(name string, mfp pgalloc.MemoryFileProvider, fr memmap.FileRange) *SpecialMappable { - m := SpecialMappable{mfp: mfp, fr: fr, name: name} +func NewSpecialMappable(name string, mf *pgalloc.MemoryFile, fr memmap.FileRange) *SpecialMappable { + m := SpecialMappable{mf: mf, fr: fr, name: name} m.InitRefs() return &m } @@ -50,7 +50,7 @@ func NewSpecialMappable(name string, mfp pgalloc.MemoryFileProvider, fr memmap.F // DecRef implements refs.RefCounter.DecRef. func (m *SpecialMappable) DecRef(ctx context.Context) { m.SpecialMappableRefs.DecRef(func() { - m.mfp.MemoryFile().DecRef(m.fr) + m.mf.DecRef(m.fr) }) } @@ -99,7 +99,7 @@ func (m *SpecialMappable) Translate(ctx context.Context, required, optional memm return []memmap.Translation{ { Source: source, - File: m.mfp.MemoryFile(), + File: m.mf, Offset: m.fr.Start + source.Start, Perms: hostarch.AnyAccess, }, @@ -115,14 +115,8 @@ func (m *SpecialMappable) InvalidateUnsavable(ctx context.Context) error { return nil } -// MemoryFileProvider returns the MemoryFileProvider whose MemoryFile stores -// the SpecialMappable's contents. -func (m *SpecialMappable) MemoryFileProvider() pgalloc.MemoryFileProvider { - return m.mfp -} - -// FileRange returns the offsets into MemoryFileProvider().MemoryFile() that -// store the SpecialMappable's contents. +// FileRange returns the offsets into m.mf that stores the SpecialMappable's +// contents. func (m *SpecialMappable) FileRange() memmap.FileRange { return m.fr } diff --git a/pkg/sentry/pgalloc/context.go b/pkg/sentry/pgalloc/context.go index 94759f521..88c897f2f 100644 --- a/pkg/sentry/pgalloc/context.go +++ b/pkg/sentry/pgalloc/context.go @@ -25,9 +25,6 @@ const ( // CtxMemoryFile is a Context.Value key for a MemoryFile. CtxMemoryFile contextID = iota - // CtxMemoryFileProvider is a Context.Value key for a MemoryFileProvider. - CtxMemoryFileProvider - // CtxMemoryCgroupID is the memory cgroup id which the task belongs to. CtxMemoryCgroupID @@ -45,15 +42,6 @@ func MemoryFileFromContext(ctx context.Context) *MemoryFile { return nil } -// MemoryFileProviderFromContext returns the MemoryFileProvider used by ctx, or -// nil if no such MemoryFileProvider exists. -func MemoryFileProviderFromContext(ctx context.Context) MemoryFileProvider { - if v := ctx.Value(CtxMemoryFileProvider); v != nil { - return v.(MemoryFileProvider) - } - return nil -} - // MemoryCgroupIDFromContext returns the memory cgroup id of the ctx, or // zero if the ctx does not belong to any memory cgroup. func MemoryCgroupIDFromContext(ctx context.Context) uint32 { diff --git a/pkg/sentry/pgalloc/save_restore.go b/pkg/sentry/pgalloc/save_restore.go index c8cf27367..c9b3878b6 100644 --- a/pkg/sentry/pgalloc/save_restore.go +++ b/pkg/sentry/pgalloc/save_restore.go @@ -213,19 +213,3 @@ func (f *MemoryFile) LoadFrom(ctx context.Context, r wire.Reader) error { return nil } - -// MemoryFileProvider provides the MemoryFile method. -// -// This type exists to work around a save/restore defect. The only object in a -// saved object graph that S/R allows to be replaced at time of restore is the -// starting point of the restore, kernel.Kernel. However, the MemoryFile -// changes between save and restore as well, so objects that need persistent -// access to the MemoryFile must instead store a pointer to the Kernel and call -// Kernel.MemoryFile() as required. In most cases, depending on the kernel -// package directly would create a package dependency loop, so the stored -// pointer must instead be a MemoryProvider interface object. Correspondingly, -// kernel.Kernel is the only implementation of this interface. -type MemoryFileProvider interface { - // MemoryFile returns the Kernel MemoryFile. - MemoryFile() *MemoryFile -} diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go index b4b2c7948..b029a0dc3 100644 --- a/runsc/boot/loader.go +++ b/runsc/boot/loader.go @@ -424,13 +424,13 @@ func New(args Args) (*Loader, error) { // Create VDSO. // // Pass k as the platform since it is savable, unlike the actual platform. - vdso, err := loader.PrepareVDSO(k) + vdso, err := loader.PrepareVDSO(k.MemoryFile()) if err != nil { return nil, fmt.Errorf("creating vdso: %w", err) } // Create timekeeper. - tk := kernel.NewTimekeeper(k, vdso.ParamPage.FileRange()) + tk := kernel.NewTimekeeper(k.MemoryFile(), vdso.ParamPage.FileRange()) tk.SetClocks(time.NewCalibratedClocks()) if err := enableStrace(args.Conf); err != nil {