Merge release-20200810.0-237-g8d0f76dda (automated)

3 files changed, 3 insertions, 211 deletions

diff --git a/pkg/sentry/fsimpl/overlay/copy_up.go b/pkg/sentry/fsimpl/overlay/copy_up.go
index 360b77ef6..c589b4746 100644
--- a/pkg/sentry/fsimpl/overlay/copy_up.go
+++ b/pkg/sentry/fsimpl/overlay/copy_up.go
@@ -23,7 +23,6 @@ import (
 	"gvisor.dev/gvisor/pkg/context"
 	"gvisor.dev/gvisor/pkg/fspath"
 	"gvisor.dev/gvisor/pkg/sentry/kernel/auth"
-	"gvisor.dev/gvisor/pkg/sentry/memmap"
 	"gvisor.dev/gvisor/pkg/sentry/vfs"
 	"gvisor.dev/gvisor/pkg/syserror"
 	"gvisor.dev/gvisor/pkg/usermem"
@@ -82,8 +81,6 @@ func (d *dentry) copyUpLocked(ctx context.Context) error {
 		Start: d.parent.upperVD,
 		Path:  fspath.Parse(d.name),
 	}
-	// Used during copy-up of memory-mapped regular files.
-	var mmapOpts *memmap.MMapOpts
 	cleanupUndoCopyUp := func() {
 		var err error
 		if ftype == linux.S_IFDIR {
@@ -139,25 +136,6 @@ func (d *dentry) copyUpLocked(ctx context.Context) error {
 				break
 			}
 		}
-		d.mapsMu.Lock()
-		defer d.mapsMu.Unlock()
-		if d.wrappedMappable != nil {
-			// We may have memory mappings of the file on the lower layer.
-			// Switch to mapping the file on the upper layer instead.
-			mmapOpts = &memmap.MMapOpts{
-				Perms:    usermem.ReadWrite,
-				MaxPerms: usermem.ReadWrite,
-			}
-			if err := newFD.ConfigureMMap(ctx, mmapOpts); err != nil {
-				cleanupUndoCopyUp()
-				return err
-			}
-			if mmapOpts.MappingIdentity != nil {
-				mmapOpts.MappingIdentity.DecRef(ctx)
-			}
-			// Don't actually switch Mappables until the end of copy-up; see
-			// below for why.
-		}
 		if err := newFD.SetStat(ctx, vfs.SetStatOptions{
 			Stat: linux.Statx{
 				Mask: linux.STATX_UID | linux.STATX_GID,
@@ -287,62 +265,6 @@ func (d *dentry) copyUpLocked(ctx context.Context) error {
 		atomic.StoreUint64(&d.ino, upperStat.Ino)
 	}
 
-	if mmapOpts != nil && mmapOpts.Mappable != nil {
-		// Note that if mmapOpts != nil, then d.mapsMu is locked for writing
-		// (from the S_IFREG path above).
-
-		// Propagate mappings of d to the new Mappable. Remember which mappings
-		// we added so we can remove them on failure.
-		upperMappable := mmapOpts.Mappable
-		allAdded := make(map[memmap.MappableRange]memmap.MappingsOfRange)
-		for seg := d.lowerMappings.FirstSegment(); seg.Ok(); seg = seg.NextSegment() {
-			added := make(memmap.MappingsOfRange)
-			for m := range seg.Value() {
-				if err := upperMappable.AddMapping(ctx, m.MappingSpace, m.AddrRange, seg.Start(), m.Writable); err != nil {
-					for m := range added {
-						upperMappable.RemoveMapping(ctx, m.MappingSpace, m.AddrRange, seg.Start(), m.Writable)
-					}
-					for mr, mappings := range allAdded {
-						for m := range mappings {
-							upperMappable.RemoveMapping(ctx, m.MappingSpace, m.AddrRange, mr.Start, m.Writable)
-						}
-					}
-					return err
-				}
-				added[m] = struct{}{}
-			}
-			allAdded[seg.Range()] = added
-		}
-
-		// Switch to the new Mappable. We do this at the end of copy-up
-		// because:
-		//
-		// - We need to switch Mappables (by changing d.wrappedMappable) before
-		// invalidating Translations from the old Mappable (to pick up
-		// Translations from the new one).
-		//
-		// - We need to lock d.dataMu while changing d.wrappedMappable, but
-		// must invalidate Translations with d.dataMu unlocked (due to lock
-		// ordering).
-		//
-		// - Consequently, once we unlock d.dataMu, other threads may
-		// immediately observe the new (copied-up) Mappable, which we want to
-		// delay until copy-up is guaranteed to succeed.
-		d.dataMu.Lock()
-		lowerMappable := d.wrappedMappable
-		d.wrappedMappable = upperMappable
-		d.dataMu.Unlock()
-		d.lowerMappings.InvalidateAll(memmap.InvalidateOpts{})
-
-		// Remove mappings from the old Mappable.
-		for seg := d.lowerMappings.FirstSegment(); seg.Ok(); seg = seg.NextSegment() {
-			for m := range seg.Value() {
-				lowerMappable.RemoveMapping(ctx, m.MappingSpace, m.AddrRange, seg.Start(), m.Writable)
-			}
-		}
-		d.lowerMappings.RemoveAll()
-	}
-
 	atomic.StoreUint32(&d.copiedUp, 1)
 	return nil
 }
diff --git a/pkg/sentry/fsimpl/overlay/non_directory.go b/pkg/sentry/fsimpl/overlay/non_directory.go
index 74cfd3799..268b32537 100644
--- a/pkg/sentry/fsimpl/overlay/non_directory.go
+++ b/pkg/sentry/fsimpl/overlay/non_directory.go
@@ -23,7 +23,6 @@ import (
 	"gvisor.dev/gvisor/pkg/sentry/memmap"
 	"gvisor.dev/gvisor/pkg/sentry/vfs"
 	"gvisor.dev/gvisor/pkg/sync"
-	"gvisor.dev/gvisor/pkg/syserror"
 	"gvisor.dev/gvisor/pkg/usermem"
 )
 
@@ -257,105 +256,10 @@ func (fd *nonDirectoryFD) Sync(ctx context.Context) error {
 
 // ConfigureMMap implements vfs.FileDescriptionImpl.ConfigureMMap.
 func (fd *nonDirectoryFD) ConfigureMMap(ctx context.Context, opts *memmap.MMapOpts) error {
-	if err := fd.ensureMappable(ctx, opts); err != nil {
-		return err
-	}
-	return vfs.GenericConfigureMMap(&fd.vfsfd, fd.dentry(), opts)
-}
-
-// ensureMappable ensures that fd.dentry().wrappedMappable is not nil.
-func (fd *nonDirectoryFD) ensureMappable(ctx context.Context, opts *memmap.MMapOpts) error {
-	d := fd.dentry()
-
-	// Fast path if we already have a Mappable for the current top layer.
-	if atomic.LoadUint32(&d.isMappable) != 0 {
-		return nil
-	}
-
-	// Only permit mmap of regular files, since other file types may have
-	// unpredictable behavior when mmapped (e.g. /dev/zero).
-	if atomic.LoadUint32(&d.mode)&linux.S_IFMT != linux.S_IFREG {
-		return syserror.ENODEV
-	}
-
-	// Get a Mappable for the current top layer.
-	fd.mu.Lock()
-	defer fd.mu.Unlock()
-	d.copyMu.RLock()
-	defer d.copyMu.RUnlock()
-	if atomic.LoadUint32(&d.isMappable) != 0 {
-		return nil
-	}
-	wrappedFD, err := fd.currentFDLocked(ctx)
+	wrappedFD, err := fd.getCurrentFD(ctx)
 	if err != nil {
 		return err
 	}
-	if err := wrappedFD.ConfigureMMap(ctx, opts); err != nil {
-		return err
-	}
-	if opts.MappingIdentity != nil {
-		opts.MappingIdentity.DecRef(ctx)
-		opts.MappingIdentity = nil
-	}
-	// Use this Mappable for all mappings of this layer (unless we raced with
-	// another call to ensureMappable).
-	d.mapsMu.Lock()
-	defer d.mapsMu.Unlock()
-	d.dataMu.Lock()
-	defer d.dataMu.Unlock()
-	if d.wrappedMappable == nil {
-		d.wrappedMappable = opts.Mappable
-		atomic.StoreUint32(&d.isMappable, 1)
-	}
-	return nil
-}
-
-// AddMapping implements memmap.Mappable.AddMapping.
-func (d *dentry) AddMapping(ctx context.Context, ms memmap.MappingSpace, ar usermem.AddrRange, offset uint64, writable bool) error {
-	d.mapsMu.Lock()
-	defer d.mapsMu.Unlock()
-	if err := d.wrappedMappable.AddMapping(ctx, ms, ar, offset, writable); err != nil {
-		return err
-	}
-	if !d.isCopiedUp() {
-		d.lowerMappings.AddMapping(ms, ar, offset, writable)
-	}
-	return nil
-}
-
-// RemoveMapping implements memmap.Mappable.RemoveMapping.
-func (d *dentry) RemoveMapping(ctx context.Context, ms memmap.MappingSpace, ar usermem.AddrRange, offset uint64, writable bool) {
-	d.mapsMu.Lock()
-	defer d.mapsMu.Unlock()
-	d.wrappedMappable.RemoveMapping(ctx, ms, ar, offset, writable)
-	if !d.isCopiedUp() {
-		d.lowerMappings.RemoveMapping(ms, ar, offset, writable)
-	}
-}
-
-// CopyMapping implements memmap.Mappable.CopyMapping.
-func (d *dentry) CopyMapping(ctx context.Context, ms memmap.MappingSpace, srcAR, dstAR usermem.AddrRange, offset uint64, writable bool) error {
-	d.mapsMu.Lock()
-	defer d.mapsMu.Unlock()
-	if err := d.wrappedMappable.CopyMapping(ctx, ms, srcAR, dstAR, offset, writable); err != nil {
-		return err
-	}
-	if !d.isCopiedUp() {
-		d.lowerMappings.AddMapping(ms, dstAR, offset, writable)
-	}
-	return nil
-}
-
-// Translate implements memmap.Mappable.Translate.
-func (d *dentry) Translate(ctx context.Context, required, optional memmap.MappableRange, at usermem.AccessType) ([]memmap.Translation, error) {
-	d.dataMu.RLock()
-	defer d.dataMu.RUnlock()
-	return d.wrappedMappable.Translate(ctx, required, optional, at)
-}
-
-// InvalidateUnsavable implements memmap.Mappable.InvalidateUnsavable.
-func (d *dentry) InvalidateUnsavable(ctx context.Context) error {
-	d.mapsMu.Lock()
-	defer d.mapsMu.Unlock()
-	return d.wrappedMappable.InvalidateUnsavable(ctx)
+	defer wrappedFD.DecRef(ctx)
+	return wrappedFD.ConfigureMMap(ctx, opts)
 }
diff --git a/pkg/sentry/fsimpl/overlay/overlay.go b/pkg/sentry/fsimpl/overlay/overlay.go
index b2efe5f80..9a8f7010e 100644
--- a/pkg/sentry/fsimpl/overlay/overlay.go
+++ b/pkg/sentry/fsimpl/overlay/overlay.go
@@ -22,10 +22,6 @@
 //   filesystem.renameMu
 //     dentry.dirMu
 //       dentry.copyMu
-//         *** "memmap.Mappable locks" below this point
-//         dentry.mapsMu
-//           *** "memmap.Mappable locks taken by Translate" below this point
-//           dentry.dataMu
 //
 // Locking dentry.dirMu in multiple dentries requires that parent dentries are
 // locked before child dentries, and that filesystem.renameMu is locked to
@@ -41,7 +37,6 @@ import (
 	"gvisor.dev/gvisor/pkg/fspath"
 	fslock "gvisor.dev/gvisor/pkg/sentry/fs/lock"
 	"gvisor.dev/gvisor/pkg/sentry/kernel/auth"
-	"gvisor.dev/gvisor/pkg/sentry/memmap"
 	"gvisor.dev/gvisor/pkg/sentry/vfs"
 	"gvisor.dev/gvisor/pkg/sync"
 	"gvisor.dev/gvisor/pkg/syserror"
@@ -424,35 +419,6 @@ type dentry struct {
 	devMinor uint32
 	ino      uint64
 
-	// If this dentry represents a regular file, then:
-	//
-	// - mapsMu is used to synchronize between copy-up and memmap.Mappable
-	// methods on dentry preceding mm.MemoryManager.activeMu in the lock order.
-	//
-	// - dataMu is used to synchronize between copy-up and
-	// dentry.(memmap.Mappable).Translate.
-	//
-	// - lowerMappings tracks memory mappings of the file. lowerMappings is
-	// used to invalidate mappings of the lower layer when the file is copied
-	// up to ensure that they remain coherent with subsequent writes to the
-	// file. (Note that, as of this writing, Linux overlayfs does not do this;
-	// this feature is a gVisor extension.) lowerMappings is protected by
-	// mapsMu.
-	//
-	// - If this dentry is copied-up, then wrappedMappable is the Mappable
-	// obtained from a call to the current top layer's
-	// FileDescription.ConfigureMMap(). Once wrappedMappable becomes non-nil
-	// (from a call to nonDirectoryFD.ensureMappable()), it cannot become nil.
-	// wrappedMappable is protected by mapsMu and dataMu.
-	//
-	// - isMappable is non-zero iff wrappedMappable is non-nil. isMappable is
-	// accessed using atomic memory operations.
-	mapsMu          sync.Mutex
-	lowerMappings   memmap.MappingSet
-	dataMu          sync.RWMutex
-	wrappedMappable memmap.Mappable
-	isMappable      uint32
-
 	locks vfs.FileLocks
 }