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

@@ -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",

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()

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
-}

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.
 //

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()

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 {