From f4e537843f49fadaa364ba7f210e68e267f59c3d Mon Sep 17 00:00:00 2001 From: Rahat Mahmood Date: Tue, 19 Apr 2022 14:53:25 -0700 Subject: [PATCH] Drop MappingIdentity ref outside mm.mappingMu critical section. Otherwise we get circular locking with various filesystem locks if the dropped ref is the final ref. Mapping identities are typically file descriptions on various filesystems, which in turn call mm functions under the respective filesystem locks. PiperOrigin-RevId: 442919016 --- pkg/sentry/mm/lifecycle.go | 20 ++++++++-- pkg/sentry/mm/shm.go | 17 ++++++++- pkg/sentry/mm/syscalls.go | 76 ++++++++++++++++++++++++++++++-------- pkg/sentry/mm/vma.go | 53 +++++++++++++++++--------- 4 files changed, 128 insertions(+), 38 deletions(-) diff --git a/pkg/sentry/mm/lifecycle.go b/pkg/sentry/mm/lifecycle.go index b2aa930cb..2e59e415d 100644 --- a/pkg/sentry/mm/lifecycle.go +++ b/pkg/sentry/mm/lifecycle.go @@ -61,6 +61,15 @@ func (mm *MemoryManager) Fork(ctx context.Context) (*MemoryManager, error) { defer mm.AddressSpace().PostFork() mm.metadataMu.Lock() defer mm.metadataMu.Unlock() + + var droppedIDs []memmap.MappingIdentity + // This must run after {mm,mm2}.mappingMu.Unlock(). + defer func() { + for _, id := range droppedIDs { + id.DecRef(ctx) + } + }() + mm.mappingMu.RLock() defer mm.mappingMu.RUnlock() mm2 := &MemoryManager{ @@ -109,7 +118,7 @@ func (mm *MemoryManager) Fork(ctx context.Context) (*MemoryManager, error) { // Inform the Mappable, if any, of the new mapping. if vma.mappable != nil { if err := vma.mappable.AddMapping(ctx, mm2, vmaAR, vma.off, vma.canWriteMappableLocked()); err != nil { - mm2.removeVMAsLocked(ctx, mm2.applicationAddrRange()) + _, droppedIDs = mm2.removeVMAsLocked(ctx, mm2.applicationAddrRange(), droppedIDs) return nil, err } } @@ -275,11 +284,16 @@ func (mm *MemoryManager) DecUsers(ctx context.Context) { } mm.activeMu.Unlock() + var droppedIDs []memmap.MappingIdentity mm.mappingMu.Lock() - defer mm.mappingMu.Unlock() // If mm is being dropped before mm.SetMmapLayout was called, // mm.applicationAddrRange() will be empty. if ar := mm.applicationAddrRange(); ar.Length() != 0 { - mm.unmapLocked(ctx, ar) + _, droppedIDs = mm.unmapLocked(ctx, ar, droppedIDs) + } + mm.mappingMu.Unlock() + + for _, id := range droppedIDs { + id.DecRef(ctx) } } diff --git a/pkg/sentry/mm/shm.go b/pkg/sentry/mm/shm.go index 94d5112a1..2a7e472ff 100644 --- a/pkg/sentry/mm/shm.go +++ b/pkg/sentry/mm/shm.go @@ -19,6 +19,7 @@ import ( "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/sentry/kernel/shm" + "gvisor.dev/gvisor/pkg/sentry/memmap" ) // DetachShm unmaps a sysv shared memory segment. @@ -29,6 +30,16 @@ func (mm *MemoryManager) DetachShm(ctx context.Context, addr hostarch.Addr) erro } var detached *shm.Shm + var vgap vmaGapIterator + + var droppedIDs []memmap.MappingIdentity + // This must run after mm.mappingMu.Unlock(). + defer func() { + for _, id := range droppedIDs { + id.DecRef(ctx) + } + }() + mm.mappingMu.Lock() defer mm.mappingMu.Unlock() @@ -39,7 +50,8 @@ func (mm *MemoryManager) DetachShm(ctx context.Context, addr hostarch.Addr) erro vma := vseg.ValuePtr() if shm, ok := vma.mappable.(*shm.Shm); ok && vseg.Start() >= addr && uint64(vseg.Start()-addr) == vma.off { detached = shm - vseg = mm.unmapLocked(ctx, vseg.Range()).NextSegment() + vgap, droppedIDs = mm.unmapLocked(ctx, vseg.Range(), droppedIDs) + vseg = vgap.NextSegment() break } else { vseg = vseg.NextSegment() @@ -56,7 +68,8 @@ func (mm *MemoryManager) DetachShm(ctx context.Context, addr hostarch.Addr) erro for vseg.Ok() && vseg.End() <= end { vma := vseg.ValuePtr() if vma.mappable == detached && uint64(vseg.Start()-addr) == vma.off { - vseg = mm.unmapLocked(ctx, vseg.Range()).NextSegment() + vgap, droppedIDs = mm.unmapLocked(ctx, vseg.Range(), droppedIDs) + vseg = vgap.NextSegment() } else { vseg = vseg.NextSegment() } diff --git a/pkg/sentry/mm/syscalls.go b/pkg/sentry/mm/syscalls.go index 7b1865567..e4c889081 100644 --- a/pkg/sentry/mm/syscalls.go +++ b/pkg/sentry/mm/syscalls.go @@ -115,11 +115,12 @@ func (mm *MemoryManager) MMap(ctx context.Context, opts memmap.MMapOpts) (hostar } // Get the new vma. + var droppedIDs []memmap.MappingIdentity mm.mappingMu.Lock() if opts.MLockMode < mm.defMLockMode { opts.MLockMode = mm.defMLockMode } - vseg, ar, err := mm.createVMALocked(ctx, opts) + vseg, ar, droppedIDs, err := mm.createVMALocked(ctx, opts, droppedIDs) if err != nil { mm.mappingMu.Unlock() return 0, err @@ -148,6 +149,10 @@ func (mm *MemoryManager) MMap(ctx context.Context, opts memmap.MMapOpts) (hostar mm.mappingMu.Unlock() } + for _, id := range droppedIDs { + id.DecRef(ctx) + } + return ar.Start, nil } @@ -264,9 +269,11 @@ func (mm *MemoryManager) MapStack(ctx context.Context) (hostarch.AddrRange, erro return hostarch.AddrRange{}, linuxerr.ENOMEM } stackStart := stackEnd - szaddr + var droppedIDs []memmap.MappingIdentity + var ar hostarch.AddrRange + var err error mm.mappingMu.Lock() - defer mm.mappingMu.Unlock() - _, ar, err := mm.createVMALocked(ctx, memmap.MMapOpts{ + _, ar, droppedIDs, err = mm.createVMALocked(ctx, memmap.MMapOpts{ Length: sz, Addr: stackStart, Perms: hostarch.ReadWrite, @@ -275,7 +282,11 @@ func (mm *MemoryManager) MapStack(ctx context.Context) (hostarch.AddrRange, erro GrowsDown: true, MLockMode: mm.defMLockMode, Hint: "[stack]", - }) + }, droppedIDs) + mm.mappingMu.Unlock() + for _, id := range droppedIDs { + id.DecRef(ctx) + } return ar, err } @@ -296,9 +307,15 @@ func (mm *MemoryManager) MUnmap(ctx context.Context, addr hostarch.Addr, length return linuxerr.EINVAL } + var droppedIDs []memmap.MappingIdentity mm.mappingMu.Lock() - defer mm.mappingMu.Unlock() - mm.unmapLocked(ctx, ar) + _, droppedIDs = mm.unmapLocked(ctx, ar, droppedIDs) + mm.mappingMu.Unlock() + + for _, id := range droppedIDs { + id.DecRef(ctx) + } + return nil } @@ -350,6 +367,14 @@ func (mm *MemoryManager) MRemap(ctx context.Context, oldAddr hostarch.Addr, oldS return 0, linuxerr.EINVAL } + var droppedIDs []memmap.MappingIdentity + // This must run after mm.mappingMu.Unlock(). + defer func() { + for _, id := range droppedIDs { + id.DecRef(ctx) + } + }() + mm.mappingMu.Lock() defer mm.mappingMu.Unlock() @@ -394,7 +419,7 @@ func (mm *MemoryManager) MRemap(ctx context.Context, oldAddr hostarch.Addr, oldS // If oldAddr+oldSize didn't overflow, oldAddr+newSize can't // either. newEnd := oldAddr + hostarch.Addr(newSize) - mm.unmapLocked(ctx, hostarch.AddrRange{newEnd, oldEnd}) + _, droppedIDs = mm.unmapLocked(ctx, hostarch.AddrRange{newEnd, oldEnd}, droppedIDs) } return oldAddr, nil } @@ -411,7 +436,10 @@ func (mm *MemoryManager) MRemap(ctx context.Context, oldAddr hostarch.Addr, oldS if vma.mappable != nil { newOffset = vseg.mappableRange().End } - vseg, ar, err := mm.createVMALocked(ctx, memmap.MMapOpts{ + var vseg vmaIterator + var ar hostarch.AddrRange + var err error + vseg, ar, droppedIDs, err = mm.createVMALocked(ctx, memmap.MMapOpts{ Length: newSize - oldSize, MappingIdentity: vma.id, Mappable: vma.mappable, @@ -424,7 +452,7 @@ func (mm *MemoryManager) MRemap(ctx context.Context, oldAddr hostarch.Addr, oldS GrowsDown: vma.growsDown, MLockMode: vma.mlockMode, Hint: vma.hint, - }) + }, droppedIDs) if err == nil { if vma.mlockMode == memmap.MLockEager { mm.populateVMA(ctx, vseg, ar, true) @@ -473,7 +501,7 @@ func (mm *MemoryManager) MRemap(ctx context.Context, oldAddr hostarch.Addr, oldS } // Unmap any mappings at the destination. - mm.unmapLocked(ctx, newAR) + _, droppedIDs = mm.unmapLocked(ctx, newAR, droppedIDs) // If the sizes specify shrinking, unmap everything between the new and // old sizes at the source. Unmapping before the following checks is @@ -481,7 +509,7 @@ func (mm *MemoryManager) MRemap(ctx context.Context, oldAddr hostarch.Addr, oldS // vma_to_resize(). if newSize < oldSize { oldNewEnd := oldAddr + hostarch.Addr(newSize) - mm.unmapLocked(ctx, hostarch.AddrRange{oldNewEnd, oldEnd}) + _, droppedIDs = mm.unmapLocked(ctx, hostarch.AddrRange{oldNewEnd, oldEnd}, droppedIDs) oldEnd = oldNewEnd } @@ -690,13 +718,17 @@ func (mm *MemoryManager) MProtect(addr hostarch.Addr, length uint64, realPerms h // BrkSetup sets mm's brk address to addr and its brk size to 0. func (mm *MemoryManager) BrkSetup(ctx context.Context, addr hostarch.Addr) { + var droppedIDs []memmap.MappingIdentity mm.mappingMu.Lock() - defer mm.mappingMu.Unlock() // Unmap the existing brk. if mm.brk.Length() != 0 { - mm.unmapLocked(ctx, mm.brk) + _, droppedIDs = mm.unmapLocked(ctx, mm.brk, droppedIDs) } mm.brk = hostarch.AddrRange{addr, addr} + mm.mappingMu.Unlock() + for _, id := range droppedIDs { + id.DecRef(ctx) + } } // Brk implements the semantics of Linux's brk(2), except that it returns an @@ -730,9 +762,21 @@ func (mm *MemoryManager) Brk(ctx context.Context, addr hostarch.Addr) (hostarch. return addr, linuxerr.EFAULT } + var vseg vmaIterator + var ar hostarch.AddrRange + var err error + + var droppedIDs []memmap.MappingIdentity + // This must run after mm.mappingMu.Unlock(). + defer func() { + for _, id := range droppedIDs { + id.DecRef(ctx) + } + }() + switch { case oldbrkpg < newbrkpg: - vseg, ar, err := mm.createVMALocked(ctx, memmap.MMapOpts{ + vseg, ar, droppedIDs, err = mm.createVMALocked(ctx, memmap.MMapOpts{ Length: uint64(newbrkpg - oldbrkpg), Addr: oldbrkpg, Fixed: true, @@ -745,7 +789,7 @@ func (mm *MemoryManager) Brk(ctx context.Context, addr hostarch.Addr) (hostarch. // mm->def_flags. MLockMode: mm.defMLockMode, Hint: "[heap]", - }) + }, droppedIDs) if err != nil { addr = mm.brk.End mm.mappingMu.Unlock() @@ -759,7 +803,7 @@ func (mm *MemoryManager) Brk(ctx context.Context, addr hostarch.Addr) (hostarch. } case newbrkpg < oldbrkpg: - mm.unmapLocked(ctx, hostarch.AddrRange{newbrkpg, oldbrkpg}) + _, droppedIDs = mm.unmapLocked(ctx, hostarch.AddrRange{newbrkpg, oldbrkpg}, droppedIDs) fallthrough default: diff --git a/pkg/sentry/mm/vma.go b/pkg/sentry/mm/vma.go index e0e80a8c7..ad1b964a3 100644 --- a/pkg/sentry/mm/vma.go +++ b/pkg/sentry/mm/vma.go @@ -28,10 +28,16 @@ import ( "gvisor.dev/gvisor/pkg/sentry/memmap" ) +// Caller provides the droppedIDs slice to collect dropped mapping +// identities. The caller must drop the references on these identities outside a +// mm.mappingMu critical section. droppedIDs has append-like semantics, multiple +// calls to functions that drop mapping identities within a scope should reuse +// the same slice. +// // Preconditions: // * mm.mappingMu must be locked for writing. // * opts must be valid as defined by the checks in MMap. -func (mm *MemoryManager) createVMALocked(ctx context.Context, opts memmap.MMapOpts) (vmaIterator, hostarch.AddrRange, error) { +func (mm *MemoryManager) createVMALocked(ctx context.Context, opts memmap.MMapOpts, droppedIDs []memmap.MappingIdentity) (vmaIterator, hostarch.AddrRange, []memmap.MappingIdentity, error) { if opts.MaxPerms != opts.MaxPerms.Effective() { panic(fmt.Sprintf("Non-effective MaxPerms %s cannot be enforced", opts.MaxPerms)) } @@ -48,7 +54,7 @@ func (mm *MemoryManager) createVMALocked(ctx context.Context, opts memmap.MMapOp if opts.Force && opts.Unmap && opts.Fixed { addr = opts.Addr } else { - return vmaIterator{}, hostarch.AddrRange{}, err + return vmaIterator{}, hostarch.AddrRange{}, droppedIDs, err } } ar, _ := addr.ToRange(opts.Length) @@ -59,7 +65,7 @@ func (mm *MemoryManager) createVMALocked(ctx context.Context, opts memmap.MMapOp newUsageAS -= uint64(mm.vmas.SpanRange(ar)) } if limitAS := limits.FromContext(ctx).Get(limits.AS).Cur; newUsageAS > limitAS { - return vmaIterator{}, hostarch.AddrRange{}, linuxerr.ENOMEM + return vmaIterator{}, hostarch.AddrRange{}, droppedIDs, linuxerr.ENOMEM } if opts.MLockMode != memmap.MLockNone { @@ -67,14 +73,14 @@ func (mm *MemoryManager) createVMALocked(ctx context.Context, opts memmap.MMapOp if creds := auth.CredentialsFromContext(ctx); !creds.HasCapabilityIn(linux.CAP_IPC_LOCK, creds.UserNamespace.Root()) { mlockLimit := limits.FromContext(ctx).Get(limits.MemoryLocked).Cur if mlockLimit == 0 { - return vmaIterator{}, hostarch.AddrRange{}, linuxerr.EPERM + return vmaIterator{}, hostarch.AddrRange{}, droppedIDs, linuxerr.EPERM } newLockedAS := mm.lockedAS + opts.Length if opts.Unmap { newLockedAS -= mm.mlockedBytesRangeLocked(ar) } if newLockedAS > mlockLimit { - return vmaIterator{}, hostarch.AddrRange{}, linuxerr.EAGAIN + return vmaIterator{}, hostarch.AddrRange{}, droppedIDs, linuxerr.EAGAIN } } } @@ -84,7 +90,7 @@ func (mm *MemoryManager) createVMALocked(ctx context.Context, opts memmap.MMapOp // file->f_op->mmap(). var vgap vmaGapIterator if opts.Unmap { - vgap = mm.unmapLocked(ctx, ar) + vgap, droppedIDs = mm.unmapLocked(ctx, ar, droppedIDs) } else { vgap = mm.vmas.FindGap(ar.Start) } @@ -94,7 +100,7 @@ func (mm *MemoryManager) createVMALocked(ctx context.Context, opts memmap.MMapOp // The expression for writable is vma.canWriteMappableLocked(), but we // don't yet have a vma. if err := opts.Mappable.AddMapping(ctx, mm, ar, opts.Offset, !opts.Private && opts.MaxPerms.Write); err != nil { - return vmaIterator{}, hostarch.AddrRange{}, err + return vmaIterator{}, hostarch.AddrRange{}, droppedIDs, err } } @@ -128,7 +134,7 @@ func (mm *MemoryManager) createVMALocked(ctx context.Context, opts memmap.MMapOp mm.lockedAS += opts.Length } - return vseg, ar, nil + return vseg, ar, droppedIDs, nil } type findAvailableOpts struct { @@ -345,11 +351,17 @@ const guardBytes = 256 * hostarch.PageSize // unmapLocked unmaps all addresses in ar and returns the resulting gap in // mm.vmas. // +// Caller provides the droppedIDs slice to collect dropped mapping +// identities. The caller must drop the references on these identities outside a +// mm.mappingMu critical section. droppedIDs has append-like semantics, multiple +// calls to functions that drop mapping identities within a scope should reuse +// the same slice. +// // Preconditions: // * mm.mappingMu must be locked for writing. // * ar.Length() != 0. // * ar must be page-aligned. -func (mm *MemoryManager) unmapLocked(ctx context.Context, ar hostarch.AddrRange) vmaGapIterator { +func (mm *MemoryManager) unmapLocked(ctx context.Context, ar hostarch.AddrRange, droppedIDs []memmap.MappingIdentity) (vmaGapIterator, []memmap.MappingIdentity) { if checkInvariants { if !ar.WellFormed() || ar.Length() == 0 || !ar.IsPageAligned() { panic(fmt.Sprintf("invalid ar: %v", ar)) @@ -359,24 +371,28 @@ func (mm *MemoryManager) unmapLocked(ctx context.Context, ar hostarch.AddrRange) // AddressSpace mappings and pmas must be invalidated before // mm.removeVMAsLocked() => memmap.Mappable.RemoveMapping(). mm.Invalidate(ar, memmap.InvalidateOpts{InvalidatePrivate: true}) - return mm.removeVMAsLocked(ctx, ar) + return mm.removeVMAsLocked(ctx, ar, droppedIDs) } -// removeVMAsLocked removes vmas for addresses in ar and returns the resulting -// gap in mm.vmas. It does not remove pmas or AddressSpace mappings; clients -// must do so before calling removeVMAsLocked. +// removeVMAsLocked removes vmas for addresses in ar and returns the +// resulting gap in mm.vmas. +// +// Caller provides the droppedIDs slice to collect dropped mapping +// identities. The caller must drop the references on these identities outside a +// mm.mappingMu critical section. droppedIDs has append-like semantics, multiple +// calls to functions that drop mapping identities within a scope should reuse +// the same slice. // // Preconditions: // * mm.mappingMu must be locked for writing. // * ar.Length() != 0. // * ar must be page-aligned. -func (mm *MemoryManager) removeVMAsLocked(ctx context.Context, ar hostarch.AddrRange) vmaGapIterator { +func (mm *MemoryManager) removeVMAsLocked(ctx context.Context, ar hostarch.AddrRange, droppedIDs []memmap.MappingIdentity) (vmaGapIterator, []memmap.MappingIdentity) { if checkInvariants { if !ar.WellFormed() || ar.Length() == 0 || !ar.IsPageAligned() { panic(fmt.Sprintf("invalid ar: %v", ar)) } } - vseg, vgap := mm.vmas.Find(ar.Start) if vgap.Ok() { vseg = vgap.NextSegment() @@ -389,7 +405,7 @@ func (mm *MemoryManager) removeVMAsLocked(ctx context.Context, ar hostarch.AddrR vma.mappable.RemoveMapping(ctx, mm, vmaAR, vma.off, vma.canWriteMappableLocked()) } if vma.id != nil { - vma.id.DecRef(ctx) + droppedIDs = append(droppedIDs, vma.id) } mm.usageAS -= uint64(vmaAR.Length()) if vma.isPrivateDataLocked() { @@ -401,7 +417,7 @@ func (mm *MemoryManager) removeVMAsLocked(ctx context.Context, ar hostarch.AddrR vgap = mm.vmas.Remove(vseg) vseg = vgap.NextSegment() } - return vgap + return vgap, droppedIDs } // canWriteMappableLocked returns true if it is possible for vma.mappable to be @@ -459,6 +475,9 @@ func (vmaSetFunctions) Merge(ar1 hostarch.AddrRange, vma1 vma, ar2 hostarch.Addr } if vma2.id != nil { + // This DecRef() will never be the final ref, since the vma1 is + // currently holding a ref to the same mapping identity. Thus, we don't + // need to worry about whether we're in a mm.mappingMu critical section. vma2.id.DecRef(context.Background()) } return vma1, true