Skip to content

Commit ae286a2

Browse files
gvisor-botgvisor-bot
committed
Merge pull request #16565 from ayushr2:mm-internal-mappings-rlock
PiperOrigin-RevId: 1009259611
2 parents efb9dff + 7a8074c commit ae286a2

3 files changed

Lines changed: 88 additions & 39 deletions

File tree

‎pkg/sentry/mm/io.go‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -626,10 +626,14 @@ func (mm *MemoryManager) withInternalMappings(ctx context.Context, ar hostarch.A
626626
// mm.mappingMu.
627627
mm.activeMu.RLock()
628628
if pseg := mm.existingPMAsLocked(ar, at, ignorePermissions, true /* needInternalMappings */); pseg.Ok() {
629-
defer mm.activeMu.RUnlock()
630-
n, err := f(mm.internalMappingsLocked(pseg, ar))
631-
// Do not convert errors returned by f to EFAULT.
632-
return int64(n), err
629+
// On failure, fall back to the slow path, which handles partial
630+
// success.
631+
if ims, err := mm.internalMappingsLocked(pseg, ar); err == nil {
632+
defer mm.activeMu.RUnlock()
633+
n, err := f(ims)
634+
// Do not convert errors returned by f to EFAULT.
635+
return int64(n), err
636+
}
633637
}
634638
mm.activeMu.RUnlock()
635639

@@ -708,10 +712,14 @@ func (mm *MemoryManager) withVecInternalMappings(ctx context.Context, ars hostar
708712
// mm.mappingMu.
709713
mm.activeMu.RLock()
710714
if mm.existingVecPMAsLocked(ars, at, ignorePermissions, true /* needInternalMappings */) {
711-
defer mm.activeMu.RUnlock()
712-
n, err := f(mm.vecInternalMappingsLocked(ars))
713-
// Do not convert errors returned by f to EFAULT.
714-
return int64(n), err
715+
// On failure, fall back to the slow path, which handles partial
716+
// success.
717+
if ims, err := mm.vecInternalMappingsLocked(ars); err == nil {
718+
defer mm.activeMu.RUnlock()
719+
n, err := f(ims)
720+
// Do not convert errors returned by f to EFAULT.
721+
return int64(n), err
722+
}
715723
}
716724
mm.activeMu.RUnlock()
717725

‎pkg/sentry/mm/mm_test.go‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1186,15 +1186,29 @@ func TestIODuringAsyncPageLoad(t *testing.T) {
11861186
mm.addRSSLocked(ar)
11871187
mm.activeMu.Unlock()
11881188

1189+
// I/O to pages past offset gated waits until gate is closed, but should
1190+
// not block concurrent I/O to other pages.
1191+
gatedErr := make(chan error, 1)
1192+
go func() {
1193+
_, err := mm.CopyIn(ctx, addr+gated+pageSizeAddr, make([]byte, 8), usermem.IOOpts{})
1194+
gatedErr <- err
1195+
}()
11891196
timer := time.AfterFunc(10*time.Second, func() {
11901197
t.Errorf("CopyIn waited for pages past offset %#x", gated)
11911198
close(gate)
11921199
})
11931200
b := make([]byte, 8)
1194-
_, err = mm.CopyIn(ctx, addr+pageSizeAddr, b, usermem.IOOpts{})
1201+
for end := time.Now().Add(100 * time.Millisecond); time.Now().Before(end); {
1202+
if _, err = mm.CopyIn(ctx, addr+pageSizeAddr, b, usermem.IOOpts{}); err != nil {
1203+
break
1204+
}
1205+
}
11951206
if timer.Stop() {
11961207
close(gate)
11971208
}
1209+
if err := <-gatedErr; err != nil {
1210+
t.Errorf("CopyIn past offset %#x got err %v want nil", gated, err)
1211+
}
11981212
if err := <-loadDone; err != nil {
11991213
t.Errorf("async page load got err %v want nil", err)
12001214
}

‎pkg/sentry/mm/pma.go‎

Lines changed: 57 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ func (mm *MemoryManager) existingPMAsLocked(ar hostarch.AddrRange, at hostarch.A
5656
if !perms.SupersetOf(at) {
5757
return pmaIterator{}
5858
}
59-
if needInternalMappings && pma.internalMappings.IsEmpty() {
59+
if needInternalMappings && pma.internalMappings.IsEmpty() && !pseg.internalMappingsUncached() {
6060
return pmaIterator{}
6161
}
6262

@@ -890,7 +890,7 @@ func (mm *MemoryManager) movePMAsLocked(oldAR, newAR hostarch.AddrRange) {
890890
mm.unmapASLocked(oldAR)
891891
}
892892

893-
// internalMappingsLocked returns cached internal mappings for addresses in ar.
893+
// internalMappingsLocked returns internal mappings for addresses in ar.
894894
//
895895
// Preconditions:
896896
// - While mm.activeMu was locked, a call to
@@ -900,7 +900,7 @@ func (mm *MemoryManager) movePMAsLocked(oldAR, newAR hostarch.AddrRange) {
900900
// - pseg.Range().Contains(ar.Start).
901901
//
902902
// +checklocksread:mm.activeMu
903-
func (mm *MemoryManager) internalMappingsLocked(pseg pmaIterator, ar hostarch.AddrRange) safemem.BlockSeq {
903+
func (mm *MemoryManager) internalMappingsLocked(pseg pmaIterator, ar hostarch.AddrRange) (safemem.BlockSeq, error) {
904904
if checkInvariants {
905905
if !ar.WellFormed() || ar.Length() == 0 {
906906
panic(fmt.Sprintf("invalid ar: %v", ar))
@@ -911,46 +911,51 @@ func (mm *MemoryManager) internalMappingsLocked(pseg pmaIterator, ar hostarch.Ad
911911
}
912912

913913
if ar.End <= pseg.End() {
914-
// Since only one pma is involved, we can use pma.internalMappings
914+
// Since only one pma is involved, we can return its internal mappings
915915
// directly, avoiding a slice allocation.
916-
offset := uint64(ar.Start - pseg.Start())
917-
return pseg.ValuePtr().internalMappings.DropFirst64(offset).TakeFirst64(uint64(ar.Length()))
916+
return pseg.internalMappingsOfLocked(ar)
918917
}
919918

920919
var ims []safemem.Block
921920
for {
922-
pr := pseg.Range().Intersect(ar)
923-
for pims := pseg.ValuePtr().internalMappings.DropFirst64(uint64(pr.Start - pseg.Start())).TakeFirst64(uint64(pr.Length())); !pims.IsEmpty(); pims = pims.Tail() {
921+
pims, err := pseg.internalMappingsOfLocked(pseg.Range().Intersect(ar))
922+
if err != nil {
923+
return safemem.BlockSeq{}, err
924+
}
925+
for ; !pims.IsEmpty(); pims = pims.Tail() {
924926
ims = append(ims, pims.Head())
925927
}
926928
if ar.End <= pseg.End() {
927929
break
928930
}
929931
pseg = pseg.NextSegment()
930932
}
931-
return safemem.BlockSeqFromSlice(ims)
933+
return safemem.BlockSeqFromSlice(ims), nil
932934
}
933935

934-
// vecInternalMappingsLocked returns cached internal mappings for addresses in
935-
// ars.
936+
// vecInternalMappingsLocked returns internal mappings for addresses in ars.
936937
//
937938
// Preconditions: While mm.activeMu was locked, a call to
938939
// existingVecPMAsLocked(needInternalMappings=true) succeeded for all addresses
939940
// in ars.
940941
//
941942
// +checklocksread:mm.activeMu
942-
func (mm *MemoryManager) vecInternalMappingsLocked(ars hostarch.AddrRangeSeq) safemem.BlockSeq {
943+
func (mm *MemoryManager) vecInternalMappingsLocked(ars hostarch.AddrRangeSeq) (safemem.BlockSeq, error) {
943944
var ims []safemem.Block
944945
for ; !ars.IsEmpty(); ars = ars.Tail() {
945946
ar := ars.Head()
946947
if ar.Length() == 0 {
947948
continue
948949
}
949-
for pims := mm.internalMappingsLocked(mm.pmas.FindSegment(ar.Start), ar); !pims.IsEmpty(); pims = pims.Tail() {
950+
pims, err := mm.internalMappingsLocked(mm.pmas.FindSegment(ar.Start), ar)
951+
if err != nil {
952+
return safemem.BlockSeq{}, err
953+
}
954+
for ; !pims.IsEmpty(); pims = pims.Tail() {
950955
ims = append(ims, pims.Head())
951956
}
952957
}
953-
return safemem.BlockSeqFromSlice(ims)
958+
return safemem.BlockSeqFromSlice(ims), nil
954959
}
955960

956961
// addRSSLocked updates the current and maximum resident set size of a
@@ -1043,9 +1048,8 @@ func (mm *MemoryManager) findOrSeekPrevUpperBoundPMA(addr hostarch.Addr, pgap pm
10431048
}
10441049

10451050
// getInternalMappingsLocked returns internal mappings for addresses in ar.
1046-
// Unless async page loading is in progress and the pma is larger than
1047-
// `asyncLoadMapUnit`, it caches internal mappings for the whole pma in
1048-
// pseg.ValuePtr().internalMappings. The generated iterator
1051+
// Unless pseg.internalMappingsUncached(), it caches internal mappings for the
1052+
// whole pma in pseg.ValuePtr().internalMappings. The generated iterator
10491053
// identifies a set node and index, not its MemoryManager, so checklocks cannot
10501054
// name the owning mutex.
10511055
//
@@ -1055,24 +1059,47 @@ func (mm *MemoryManager) findOrSeekPrevUpperBoundPMA(addr hostarch.Addr, pgap pm
10551059
// - ar.Length() != 0.
10561060
func (pseg pmaIterator) getInternalMappingsLocked(ar hostarch.AddrRange) (safemem.BlockSeq, error) {
10571061
pma := pseg.ValuePtr()
1058-
if pma.internalMappings.IsEmpty() {
1059-
// This must use maxPerms (instead of perms) because some permission
1060-
// constraints are only visible to vmas; for example, mappings of
1061-
// read-only files have vma.maxPerms.Write unset, but this may not be
1062-
// visible to the memmap.Mappable.
1063-
perms := pma.maxPerms
1064-
// We will never execute application code through an internal mapping.
1065-
perms.Execute = false
1066-
if mf, ok := pma.file.(*pgalloc.MemoryFile); ok && mf.IsAsyncLoading() && pseg.Range().Length() > asyncLoadMapUnit {
1067-
return mf.MapInternal(pseg.fileRangeOf(ar), perms)
1068-
}
1069-
ims, err := pma.file.MapInternal(pseg.fileRange(), perms)
1062+
if pma.internalMappings.IsEmpty() && !pseg.internalMappingsUncached() {
1063+
ims, err := pseg.internalMappingsOfLocked(pseg.Range())
10701064
if err != nil {
10711065
return safemem.BlockSeq{}, err
10721066
}
10731067
pma.internalMappings = ims
10741068
}
1075-
return pma.internalMappings.DropFirst64(uint64(ar.Start - pseg.Start())).TakeFirst64(uint64(ar.Length())), nil
1069+
return pseg.internalMappingsOfLocked(ar)
1070+
}
1071+
1072+
// internalMappingsOfLocked returns internal mappings for addresses in ar,
1073+
// from pseg.ValuePtr().internalMappings if it is non-empty and from
1074+
// memmap.File.MapInternal otherwise.
1075+
//
1076+
// Preconditions:
1077+
// - The owning MemoryManager's activeMu is locked.
1078+
// - pseg.Range().IsSupersetOf(ar).
1079+
// - ar.Length() != 0.
1080+
func (pseg pmaIterator) internalMappingsOfLocked(ar hostarch.AddrRange) (safemem.BlockSeq, error) {
1081+
pma := pseg.ValuePtr()
1082+
if !pma.internalMappings.IsEmpty() {
1083+
return pma.internalMappings.DropFirst64(uint64(ar.Start - pseg.Start())).TakeFirst64(uint64(ar.Length())), nil
1084+
}
1085+
// This must use maxPerms (instead of perms) because some permission
1086+
// constraints are only visible to vmas; for example, mappings of read-only
1087+
// files have vma.maxPerms.Write unset, but this may not be visible to the
1088+
// memmap.Mappable.
1089+
perms := pma.maxPerms
1090+
// We will never execute application code through an internal mapping.
1091+
perms.Execute = false
1092+
return pma.file.MapInternal(pseg.fileRangeOf(ar), perms)
1093+
}
1094+
1095+
// internalMappingsUncached returns true if internal mappings for pseg should
1096+
// not be cached, so that each access waits only for async page loading of the
1097+
// pages it accesses rather than of the whole pma. Uncached internal mappings
1098+
// do not mutate the pma, so they only require activeMu to be locked for
1099+
// reading.
1100+
func (pseg pmaIterator) internalMappingsUncached() bool {
1101+
mf, ok := pseg.ValuePtr().file.(*pgalloc.MemoryFile)
1102+
return ok && mf.IsAsyncLoading() && pseg.Range().Length() > asyncLoadMapUnit
10761103
}
10771104

10781105
func (pseg pmaIterator) fileRange() memmap.FileRange {

0 commit comments

Comments
 (0)