From dbf8cbbe8344bf1f7c4034bb1be92dec48bafabd Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Fri, 10 Nov 2023 14:59:43 -0800 Subject: [PATCH] Remove MM.privateRefs. MM.privateRefs counted pgalloc.MemoryFile references before platform.File refcounting was added in cl/191858388. Since that CL, MM.privateRefs is vestigial. PiperOrigin-RevId: 581379213 --- pkg/sentry/mm/BUILD | 28 +-------------- pkg/sentry/mm/lifecycle.go | 24 ++++++------- pkg/sentry/mm/mm.go | 58 ++++-------------------------- pkg/sentry/mm/pma.go | 67 ++--------------------------------- pkg/sentry/mm/syscalls.go | 5 +-- pkg/sentry/pgalloc/pgalloc.go | 21 +++++++++++ 6 files changed, 43 insertions(+), 160 deletions(-) diff --git a/pkg/sentry/mm/BUILD b/pkg/sentry/mm/BUILD index 40bb3d439..6fbf619df 100644 --- a/pkg/sentry/mm/BUILD +++ b/pkg/sentry/mm/BUILD @@ -1,6 +1,6 @@ +load("//pkg/sync/locking:locking.bzl", "declare_mutex", "declare_rwmutex") load("//tools:defs.bzl", "go_library", "go_test") load("//tools/go_generics:defs.bzl", "go_template_instance") -load("//pkg/sync/locking:locking.bzl", "declare_mutex", "declare_rwmutex") package( default_applicable_licenses = ["//:license"], @@ -43,30 +43,6 @@ declare_mutex( prefix = "metadata", ) -declare_mutex( - name = "private_refs_mutex", - out = "private_refs_mutex.go", - package = "mm", - prefix = "privateRefs", -) - -go_template_instance( - name = "file_refcount_set", - out = "file_refcount_set.go", - imports = { - "memmap": "gvisor.dev/gvisor/pkg/sentry/memmap", - }, - package = "mm", - prefix = "fileRefcount", - template = "//pkg/segment:generic_set", - types = { - "Key": "uint64", - "Range": "memmap.FileRange", - "Value": "int32", - "Functions": "fileRefcountSetFunctions", - }, -) - go_template_instance( name = "vma_set", out = "vma_set.go", @@ -153,7 +129,6 @@ go_library( "aio_manager_mutex.go", "aio_mappable_refs.go", "debug.go", - "file_refcount_set.go", "io.go", "io_list.go", "lifecycle.go", @@ -163,7 +138,6 @@ go_library( "mm.go", "pma.go", "pma_set.go", - "private_refs_mutex.go", "procfs.go", "save_restore.go", "shm.go", diff --git a/pkg/sentry/mm/lifecycle.go b/pkg/sentry/mm/lifecycle.go index bd91ed379..9ded62e01 100644 --- a/pkg/sentry/mm/lifecycle.go +++ b/pkg/sentry/mm/lifecycle.go @@ -34,7 +34,6 @@ func NewMemoryManager(p platform.Platform, mfp pgalloc.MemoryFileProvider, sleep mfp: mfp, mf: mfp.MemoryFile(), haveASIO: p.SupportsAddressSpaceIO(), - privateRefs: &privateRefs{}, users: atomicbitops.FromInt32(1), auxv: arch.Auxv{}, dumpability: atomicbitops.FromInt32(int32(UserDumpable)), @@ -74,16 +73,15 @@ func (mm *MemoryManager) Fork(ctx context.Context) (*MemoryManager, error) { mm.mappingMu.RLock() defer mm.mappingMu.RUnlock() mm2 := &MemoryManager{ - p: mm.p, - mfp: mm.mfp, - mf: mm.mf, - haveASIO: mm.haveASIO, - layout: mm.layout, - privateRefs: mm.privateRefs, - users: atomicbitops.FromInt32(1), - brk: mm.brk, - usageAS: mm.usageAS, - dataAS: mm.dataAS, + p: mm.p, + mfp: mm.mfp, + mf: mm.mf, + haveASIO: mm.haveASIO, + layout: mm.layout, + users: atomicbitops.FromInt32(1), + brk: mm.brk, + usageAS: mm.usageAS, + dataAS: mm.dataAS, // "The child does not inherit its parent's memory locks (mlock(2), // mlockall(2))." - fork(2). So lockedAS is 0 and defMLockMode is // MLockNone, both of which are zero values. vma.mlockMode is reset @@ -200,8 +198,8 @@ func (mm *MemoryManager) Fork(ctx context.Context) (*MemoryManager, error) { pma.maxPerms.Write = false } fr := srcpseg.fileRange() - mm2.incPrivateRef(fr) - srcpseg.ValuePtr().file.IncRef(fr, memCgID) + // srcpseg.ValuePtr().file == mm.mf since pma.private == true. + mm.mf.IncRef(fr, memCgID) addrRange := srcpseg.Range() mm2.addRSSLocked(addrRange) dstpgap = mm2.pmas.Insert(dstpgap, addrRange, *pma).NextGap() diff --git a/pkg/sentry/mm/mm.go b/pkg/sentry/mm/mm.go index 3fb02e225..b6281e4ef 100644 --- a/pkg/sentry/mm/mm.go +++ b/pkg/sentry/mm/mm.go @@ -17,7 +17,7 @@ // // Lock order: // -// fs locks, except for memmap.Mappable locks +// fs locks, except for memmap.Mappable locks // mm.MemoryManager.metadataMu // mm.MemoryManager.mappingMu // Locks taken by memmap.MappingIdentity and memmap.Mappable methods other @@ -25,9 +25,8 @@ // kernel.TaskSet.mu // mm.MemoryManager.activeMu // Locks taken by memmap.Mappable.Translate -// mm.privateRefs.mu -// platform.AddressSpace locks -// memmap.File locks +// platform.AddressSpace locks +// memmap.File locks // mm.aioManager.mu // mm.AIOContext.mu // @@ -78,13 +77,6 @@ type MemoryManager struct { // layout is set by the binary loader before the MemoryManager can be used. layout arch.MmapLayout - // privateRefs stores reference counts for private memory (memory whose - // ownership is shared by one or more pmas instead of being owned by a - // memmap.Mappable). - // - // privateRefs is immutable. - privateRefs *privateRefs - // users is the number of dependencies on the mappings in the MemoryManager. // When the number of references in users reaches zero, all mappings are // unmapped. @@ -359,11 +351,6 @@ type pma struct { file memmap.File `state:"nosave"` // off is the offset into file at which this pma begins. - // - // Note that pmas do *not* hold references on offsets in file! If private - // is true, MemoryManager.privateRefs holds the reference instead. If - // private is false, the corresponding memmap.Mappable holds the reference - // instead (per memmap.Mappable.Translate requirement). off uint64 // translatePerms is the permissions returned by memmap.Mappable.Translate. @@ -386,10 +373,9 @@ type pma struct { // private is true if this pma represents private memory. // - // If private is true, file must be MemoryManager.mfp.MemoryFile(), the pma - // holds a reference on the mapped memory that is tracked in privateRefs, - // and calls to Invalidate for which - // memmap.InvalidateOpts.InvalidatePrivate is false should ignore the pma. + // If private is true, file must be MemoryManager.mfp.MemoryFile(), and + // calls to Invalidate for which memmap.InvalidateOpts.InvalidatePrivate is + // false should ignore the pma. // // If private is false, this pma caches a translation from the // corresponding vma's memmap.Mappable.Translate. @@ -400,39 +386,7 @@ type pma struct { internalMappings safemem.BlockSeq `state:"nosave"` } -// +stateify savable -type privateRefs struct { - mu privateRefsMutex `state:"nosave"` - - // refs maps offsets into MemoryManager.mfp.MemoryFile() to the number of - // pmas (or, equivalently, MemoryManagers) that share ownership of the - // memory at that offset. - refs fileRefcountSet -} - type invalidateArgs struct { ar hostarch.AddrRange opts memmap.InvalidateOpts } - -// fileRefcountSetFunctions implements segment.Functions for fileRefcountSet. -type fileRefcountSetFunctions struct{} - -func (fileRefcountSetFunctions) MinKey() uint64 { - return 0 -} - -func (fileRefcountSetFunctions) MaxKey() uint64 { - return ^uint64(0) -} - -func (fileRefcountSetFunctions) ClearValue(_ *int32) { -} - -func (fileRefcountSetFunctions) Merge(_ memmap.FileRange, rc1 int32, _ memmap.FileRange, rc2 int32) (int32, bool) { - return rc1, rc1 == rc2 -} - -func (fileRefcountSetFunctions) Split(_ memmap.FileRange, rc int32, _ uint64) (int32, int32) { - return rc, rc -} diff --git a/pkg/sentry/mm/pma.go b/pkg/sentry/mm/pma.go index 3dcd38017..81d06a01e 100644 --- a/pkg/sentry/mm/pma.go +++ b/pkg/sentry/mm/pma.go @@ -250,8 +250,6 @@ func (mm *MemoryManager) getPMAsInternalLocked(ctx context.Context, vseg vmaIter } } mm.addRSSLocked(allocAR) - mm.incPrivateRef(fr) - mm.mf.IncRef(fr, memCgID) pseg, pgap = mm.pmas.Insert(pgap, allocAR, pma{ file: mm.mf, off: fr.Start, @@ -390,7 +388,7 @@ func (mm *MemoryManager) getPMAsInternalLocked(ctx context.Context, vseg vmaIter } // Unmap all of maskAR, not just copyAR, to minimize host // syscalls. AddressSpace mappings must be removed before - // mm.decPrivateRef(). + // oldpma.file.DecRef(). if !didUnmapAS { mm.unmapASLocked(maskAR) didUnmapAS = true @@ -404,12 +402,7 @@ func (mm *MemoryManager) getPMAsInternalLocked(ctx context.Context, vseg vmaIter pstart = pmaIterator{} // iterators invalidated } oldpma = pseg.ValuePtr() - if oldpma.private { - mm.decPrivateRef(pseg.fileRange()) - } oldpma.file.DecRef(pseg.fileRange()) - mm.incPrivateRef(fr) - mm.mf.IncRef(fr, memCgID) oldpma.file = mm.mf oldpma.off = fr.Start oldpma.translatePerms = hostarch.AnyAccess @@ -573,12 +566,7 @@ func (mm *MemoryManager) isPMACopyOnWriteLocked(vseg vmaIterator, pseg pmaIterat // ownership of it instead of copying. If we do hold the only reference, // additional references can only be taken by mm.Fork(), which is excluded // by mm.activeMu, so this isn't racy. - mm.privateRefs.mu.Lock() - defer mm.privateRefs.mu.Unlock() - fr := pseg.fileRange() - // This check relies on mm.privateRefs.refs being kept fully merged. - rseg := mm.privateRefs.refs.FindSegment(fr.Start) - if rseg.Ok() && rseg.Value() == 1 && fr.End <= rseg.End() { + if mm.mf.HasUniqueRef(pseg.fileRange()) { pma.needCOW = false // pma.private => pma.translatePerms == hostarch.AnyAccess vma := vseg.ValuePtr() @@ -630,7 +618,7 @@ func (mm *MemoryManager) invalidateLocked(ar hostarch.AddrRange, invalidatePriva if !didUnmapAS { // Unmap all of ar, not just pseg.Range(), to minimize host // syscalls. AddressSpace mappings must be removed before - // mm.decPrivateRef(). + // pma.file.DecRef(). // // Note that we do more than just ar here, and extrapolate // to the end of any previous region that we may have mapped. @@ -654,9 +642,6 @@ func (mm *MemoryManager) invalidateLocked(ar hostarch.AddrRange, invalidatePriva mm.unmapASLocked(unmapAR) didUnmapAS = true } - if pma.private { - mm.decPrivateRef(pseg.fileRange()) - } mm.removeRSSLocked(pseg.Range()) pma.file.DecRef(pseg.fileRange()) pseg = mm.pmas.Remove(pseg).NextSegment() @@ -933,52 +918,6 @@ func (mm *MemoryManager) vecInternalMappingsLocked(ars hostarch.AddrRangeSeq) sa return safemem.BlockSeqFromSlice(ims) } -// incPrivateRef acquires a reference on private pages in fr. -func (mm *MemoryManager) incPrivateRef(fr memmap.FileRange) { - mm.privateRefs.mu.Lock() - defer mm.privateRefs.mu.Unlock() - refSet := &mm.privateRefs.refs - seg, gap := refSet.Find(fr.Start) - for { - switch { - case seg.Ok() && seg.Start() < fr.End: - seg = refSet.Isolate(seg, fr) - seg.SetValue(seg.Value() + 1) - seg, gap = seg.NextNonEmpty() - case gap.Ok() && gap.Start() < fr.End: - seg, gap = refSet.InsertWithoutMerging(gap, gap.Range().Intersect(fr), 1).NextNonEmpty() - default: - refSet.MergeAdjacent(fr) - return - } - } -} - -// decPrivateRef releases a reference on private pages in fr. -func (mm *MemoryManager) decPrivateRef(fr memmap.FileRange) { - var freed []memmap.FileRange - - mm.privateRefs.mu.Lock() - refSet := &mm.privateRefs.refs - seg := refSet.LowerBoundSegment(fr.Start) - for seg.Ok() && seg.Start() < fr.End { - seg = refSet.Isolate(seg, fr) - if old := seg.Value(); old == 1 { - freed = append(freed, seg.Range()) - seg = refSet.Remove(seg).NextSegment() - } else { - seg.SetValue(old - 1) - seg = seg.NextSegment() - } - } - refSet.MergeAdjacent(fr) - mm.privateRefs.mu.Unlock() - - for _, fr := range freed { - mm.mf.DecRef(fr) - } -} - // addRSSLocked updates the current and maximum resident set size of a // MemoryManager to reflect the insertion of a pma at ar. // diff --git a/pkg/sentry/mm/syscalls.go b/pkg/sentry/mm/syscalls.go index dceae9c17..232dc0203 100644 --- a/pkg/sentry/mm/syscalls.go +++ b/pkg/sentry/mm/syscalls.go @@ -1119,13 +1119,10 @@ func (mm *MemoryManager) Decommit(addr hostarch.Addr, length uint64) error { if !didUnmapAS { // Unmap all of ar, not just pseg.Range(), to minimize host // syscalls. AddressSpace mappings must be removed before - // mm.decPrivateRef(). + // pma.file.DecRef(). mm.unmapASLocked(ar) didUnmapAS = true } - if pma.private { - mm.decPrivateRef(pseg.fileRange()) - } pma.file.DecRef(pseg.fileRange()) mm.removeRSSLocked(pseg.Range()) pseg = mm.pmas.Remove(pseg).NextSegment() diff --git a/pkg/sentry/pgalloc/pgalloc.go b/pkg/sentry/pgalloc/pgalloc.go index 79e2fd719..bb67fe788 100644 --- a/pkg/sentry/pgalloc/pgalloc.go +++ b/pkg/sentry/pgalloc/pgalloc.go @@ -866,6 +866,27 @@ func (f *MemoryFile) markDecommitted(fr memmap.FileRange) { f.usage.MergeRange(fr) } +// HasUniqueRef returns true if all pages in the given range have exactly one +// reference. A return value of false is inherently racy, but if the caller +// holds a reference on the given range and is preventing other goroutines from +// copying it, then a return value of true is not racy. +// +// Preconditions: At least one reference must be held on all pages in fr. +func (f *MemoryFile) HasUniqueRef(fr memmap.FileRange) bool { + f.mu.Lock() + defer f.mu.Unlock() + seg := f.usage.FindSegment(fr.Start) + for { + if seg.ValuePtr().refs != 1 { + return false + } + seg = seg.NextSegment() + if !seg.Ok() || fr.End <= seg.Start() { + return true + } + } +} + // IncRef implements memmap.File.IncRef. func (f *MemoryFile) IncRef(fr memmap.FileRange, memCgID uint32) { if !fr.WellFormed() || fr.Length() == 0 || fr.Start%hostarch.PageSize != 0 || fr.End%hostarch.PageSize != 0 {