Clean up gofer handle caching.

- Document fsutil.CachedFileObject.FD() requirements on access
permissions, and change gofer.inodeFileState.FD() to honor them.
Fixes #147.

- Combine gofer.inodeFileState.readonly and
gofer.inodeFileState.readthrough, and simplify handle caching logic.

- Inline gofer.cachePolicy.cacheHandles into
gofer.inodeFileState.setSharedHandles, because users with access to
gofer.inodeFileState don't necessarily have access to the fs.Inode
(predictably, this is a save/restore problem).

Before this CL:

$ docker run --runtime=runsc-d -v $(pwd)/gvisor/repro:/root/repro -it ubuntu bash
root@34d51017ed67:/# /root/repro/runsc-b147
mmap: 0x7f3c01e45000
Segmentation fault

After this CL:

$ docker run --runtime=runsc-d -v $(pwd)/gvisor/repro:/root/repro -it ubuntu bash
root@d3c3cb56bbf9:/# /root/repro/runsc-b147
mmap: 0x7f78987ec000
o
PiperOrigin-RevId: 240818413
Change-Id: I49e1d4a81a0cb9177832b0a9f31a10da722a896b

pkg/sentry/fs/fsutil/inode_cached.go

@@ -138,8 +138,15 @@ type CachedFileObject interface {
 	// Sync instructs the remote filesystem to sync the file to stable storage.
 	Sync(ctx context.Context) error
 
-	// FD returns a host file descriptor. Return value must be -1 or not -1
-	// for the lifetime of the CachedFileObject.
+	// FD returns a host file descriptor. If it is possible for
+	// CachingInodeOperations.AddMapping to have ever been called with writable
+	// = true, the FD must have been opened O_RDWR; otherwise, it may have been
+	// opened O_RDONLY or O_RDWR. (mmap unconditionally requires that mapped
+	// files are readable.) If no host file descriptor is available, FD returns
+	// a negative number.
+	//
+	// For any given CachedFileObject, if FD() ever succeeds (returns a
+	// non-negative number), it must always succeed.
 	//
 	// FD is called iff the file has been memory mapped. This implies that
 	// the file was opened (see fs.InodeOperations.GetFile).

pkg/sentry/fs/gofer/cache_policy.go

@@ -103,18 +103,6 @@ func (cp cachePolicy) useCachingInodeOps(inode *fs.Inode) bool {
 	return cp == cacheAll || cp == cacheAllWritethrough
 }
 
-// cacheHandles determine whether handles need to be cached with the given
-// inode. Handles must be cached when inode can be mapped into memory to
-// implement InodeOperations.Mappable with stable handles.
-func (cp cachePolicy) cacheHandles(inode *fs.Inode) bool {
-	// Do cached IO for regular files only. Some "character devices" expect
-	// no caching.
-	if !fs.IsFile(inode.StableAttr) {
-		return false
-	}
-	return cp.useCachingInodeOps(inode) || cp == cacheRemoteRevalidating
-}
-
 // writeThough indicates whether writes to the file should be synced to the
 // gofer immediately.
 func (cp cachePolicy) writeThrough(inode *fs.Inode) bool {

pkg/sentry/fs/gofer/file_state.go

@@ -29,11 +29,10 @@ func (f *fileOperations) afterLoad() {
 		// Manually load the open handles.
 		var err error
 		// TODO: Context is not plumbed to save/restore.
-		f.handles, err = newHandles(context.Background(), f.inodeOperations.fileState.file, f.flags)
+		f.handles, err = f.inodeOperations.fileState.getHandles(context.Background(), f.flags)
 		if err != nil {
 			return fmt.Errorf("failed to re-open handle: %v", err)
 		}
-		f.inodeOperations.fileState.setHandlesForCachedIO(f.flags, f.handles)
 		return nil
 	}
 	fs.Async(fs.CatchError(load))

pkg/sentry/fs/gofer/inode.go

@@ -94,21 +94,23 @@ type inodeFileState struct {
 	// handlesMu protects the below fields.
 	handlesMu sync.RWMutex `state:"nosave"`
 
-	// Do minimal open handle caching: only for read only filesystems.
-	readonly *handles `state:"nosave"`
-
-	// Maintain readthrough handles for populating page caches.
-	readthrough *handles `state:"nosave"`
-
-	// Maintain writeback handles for syncing from page caches.
-	writeback *handles `state:"nosave"`
-
-	// writebackRW indicates whether writeback is opened read-write. If
-	// it is not and a read-write handle could replace writeback (above),
-	// then writeback is replaced with the read-write handle. This
-	// ensures that files that were first opened write-only and then
-	// later are opened read-write to be mapped can in fact be mapped.
-	writebackRW bool
+	// If readHandles is non-nil, it holds handles that are either read-only or
+	// read/write. If writeHandles is non-nil, it holds write-only handles if
+	// writeHandlesRW is false, and read/write handles if writeHandlesRW is
+	// true.
+	//
+	// Once readHandles becomes non-nil, it can't be changed until
+	// inodeFileState.Release(), because of a defect in the
+	// fsutil.CachedFileObject interface: there's no way for the caller of
+	// fsutil.CachedFileObject.FD() to keep the returned FD open, so if we
+	// racily replace readHandles after inodeFileState.FD() has returned
+	// readHandles.Host.FD(), fsutil.CachingInodeOperations may use a closed
+	// FD. writeHandles can be changed if writeHandlesRW is false, since
+	// inodeFileState.FD() can't return a write-only FD, but can't be changed
+	// if writeHandlesRW is true for the same reason.
+	readHandles    *handles `state:"nosave"`
+	writeHandles   *handles `state:"nosave"`
+	writeHandlesRW bool     `state:"nosave"`
 
 	// loading is acquired when the inodeFileState begins an asynchronous
 	// load. It releases when the load is complete. Callers that require all
@@ -134,81 +136,82 @@ type inodeFileState struct {
 // Release releases file handles.
 func (i *inodeFileState) Release(ctx context.Context) {
 	i.file.close(ctx)
-	if i.readonly != nil {
-		i.readonly.DecRef()
+	if i.readHandles != nil {
+		i.readHandles.DecRef()
 	}
-	if i.readthrough != nil {
-		i.readthrough.DecRef()
-	}
-	if i.writeback != nil {
-		i.writeback.DecRef()
+	if i.writeHandles != nil {
+		i.writeHandles.DecRef()
 	}
 }
 
-// setHandlesForCachedIO installs file handles for reading and writing
-// through fs.CachingInodeOperations.
-func (i *inodeFileState) setHandlesForCachedIO(flags fs.FileFlags, h *handles) {
-	i.handlesMu.Lock()
-	defer i.handlesMu.Unlock()
+func (i *inodeFileState) canShareHandles() bool {
+	// Only share handles for regular files, since for other file types,
+	// distinct handles may have special semantics even if they represent the
+	// same file. Disable handle sharing for cache policy cacheNone, since this
+	// is legacy behavior.
+	return fs.IsFile(i.sattr) && i.s.cachePolicy != cacheNone
+}
 
-	if flags.Read {
-		if i.readthrough == nil {
-			h.IncRef()
-			i.readthrough = h
-		}
+// Preconditions: i.handlesMu must be locked for writing.
+func (i *inodeFileState) setSharedHandlesLocked(flags fs.FileFlags, h *handles) {
+	if flags.Read && i.readHandles == nil {
+		h.IncRef()
+		i.readHandles = h
 	}
 	if flags.Write {
-		if i.writeback == nil {
+		if i.writeHandles == nil {
 			h.IncRef()
-			i.writeback = h
-		} else if !i.writebackRW && flags.Read {
-			i.writeback.DecRef()
+			i.writeHandles = h
+			i.writeHandlesRW = flags.Read
+		} else if !i.writeHandlesRW && flags.Read {
+			// Upgrade i.writeHandles.
+			i.writeHandles.DecRef()
 			h.IncRef()
-			i.writeback = h
-		}
-		if flags.Read {
-			i.writebackRW = true
+			i.writeHandles = h
+			i.writeHandlesRW = flags.Read
 		}
 	}
 }
 
-// getCachedHandles returns any cached handles which would accelerate
-// performance generally. These handles should only be used if the mount
-// supports caching. This is distinct from fs.CachingInodeOperations
-// which is used for a limited set of file types (those that can be mapped).
-func (i *inodeFileState) getCachedHandles(ctx context.Context, flags fs.FileFlags, msrc *fs.MountSource) (*handles, bool) {
+// getHandles returns a set of handles for a new file using i opened with the
+// given flags.
+func (i *inodeFileState) getHandles(ctx context.Context, flags fs.FileFlags) (*handles, error) {
+	if !i.canShareHandles() {
+		return newHandles(ctx, i.file, flags)
+	}
 	i.handlesMu.Lock()
 	defer i.handlesMu.Unlock()
-
-	if flags.Read && !flags.Write && msrc.Flags.ReadOnly {
-		if i.readonly != nil {
-			i.readonly.IncRef()
-			return i.readonly, true
+	// Do we already have usable shared handles?
+	if flags.Write {
+		if i.writeHandles != nil && (i.writeHandlesRW || !flags.Read) {
+			i.writeHandles.IncRef()
+			return i.writeHandles, nil
 		}
-		h, err := newHandles(ctx, i.file, flags)
-		if err != nil {
-			return nil, false
-		}
-		i.readonly = h
-		i.readonly.IncRef()
-		return i.readonly, true
+	} else if i.readHandles != nil {
+		i.readHandles.IncRef()
+		return i.readHandles, nil
 	}
-
-	return nil, false
+	// No; get new handles and cache them for future sharing.
+	h, err := newHandles(ctx, i.file, flags)
+	if err != nil {
+		return nil, err
+	}
+	i.setSharedHandlesLocked(flags, h)
+	return h, nil
 }
 
 // ReadToBlocksAt implements fsutil.CachedFileObject.ReadToBlocksAt.
 func (i *inodeFileState) ReadToBlocksAt(ctx context.Context, dsts safemem.BlockSeq, offset uint64) (uint64, error) {
 	i.handlesMu.RLock()
 	defer i.handlesMu.RUnlock()
-	return i.readthrough.readWriterAt(ctx, int64(offset)).ReadToBlocks(dsts)
+	return i.readHandles.readWriterAt(ctx, int64(offset)).ReadToBlocks(dsts)
 }
 
 // WriteFromBlocksAt implements fsutil.CachedFileObject.WriteFromBlocksAt.
 func (i *inodeFileState) WriteFromBlocksAt(ctx context.Context, srcs safemem.BlockSeq, offset uint64) (uint64, error) {
 	i.handlesMu.RLock()
 	defer i.handlesMu.RUnlock()
-	return i.writeback.readWriterAt(ctx, int64(offset)).WriteFromBlocks(srcs)
+	return i.writeHandles.readWriterAt(ctx, int64(offset)).WriteFromBlocks(srcs)
 }
 
 // SetMaskedAttributes implements fsutil.CachedFileObject.SetMaskedAttributes.
@@ -276,52 +279,31 @@ func (i *inodeFileState) skipSetAttr(mask fs.AttrMask) bool {
 
 	i.handlesMu.RLock()
 	defer i.handlesMu.RUnlock()
-	return (i.readonly != nil && i.readonly.Host != nil) ||
-		(i.readthrough != nil && i.readthrough.Host != nil) ||
-		(i.writeback != nil && i.writeback.Host != nil)
+	return (i.readHandles != nil && i.readHandles.Host != nil) ||
+		(i.writeHandles != nil && i.writeHandles.Host != nil)
 }
 
 // Sync implements fsutil.CachedFileObject.Sync.
 func (i *inodeFileState) Sync(ctx context.Context) error {
 	i.handlesMu.RLock()
 	defer i.handlesMu.RUnlock()
-	if i.writeback == nil {
+	if i.writeHandles == nil {
 		return nil
 	}
-	return i.writeback.File.fsync(ctx)
+	return i.writeHandles.File.fsync(ctx)
 }
 
 // FD implements fsutil.CachedFileObject.FD.
-//
-// FD meets the requirements of fsutil.CachedFileObject.FD because p9.File.Open
-// returns a host file descriptor to back _both_ readthrough and writeback or
-// not at all (e.g. both are nil).
 func (i *inodeFileState) FD() int {
 	i.handlesMu.RLock()
 	defer i.handlesMu.RUnlock()
-	return i.fdLocked()
-}
-
-func (i *inodeFileState) fdLocked() int {
-	// Assert that the file was actually opened.
-	if i.writeback == nil && i.readthrough == nil {
-		panic("cannot get host FD for a file that was never opened")
+	if i.writeHandlesRW && i.writeHandles != nil && i.writeHandles.Host != nil {
+		return int(i.writeHandles.Host.FD())
 	}
-	// If this file is mapped, then it must have been opened
-	// read-write and i.writeback was upgraded to a read-write
-	// handle. Prefer that to map.
-	if i.writeback != nil {
-		if i.writeback.Host == nil {
-			return -1
-		}
-		return int(i.writeback.Host.FD())
+	if i.readHandles != nil && i.readHandles.Host != nil {
+		return int(i.readHandles.Host.FD())
 	}
-	// Otherwise the file may only have been opened readable
-	// so far. That's the only way it can be accessed.
-	if i.readthrough.Host == nil {
-		return -1
-	}
-	return int(i.readthrough.Host.FD())
+	return -1
 }
 
 // waitForLoad makes sure any restore-issued loading is done.
@@ -409,18 +391,15 @@ func (i *inodeOperations) getFileSocket(ctx context.Context, d *fs.Dirent, flags
 }
 
 func (i *inodeOperations) getFilePipe(ctx context.Context, d *fs.Dirent, flags fs.FileFlags) (*fs.File, error) {
-	// Try to open as a host pipe.
-	if pipeOps, err := fdpipe.Open(ctx, i, flags); err != errNotHostFile {
-		return fs.NewFile(ctx, d, flags, pipeOps), err
+	// Try to open as a host pipe; if that doesn't work, handle it normally.
+	pipeOps, err := fdpipe.Open(ctx, i, flags)
+	if err == errNotHostFile {
+		return i.getFileDefault(ctx, d, flags)
 	}
-
-	// If the error is due to the fact that this was never a host pipe, then back
-	// this file with its dirent.
-	h, err := newHandles(ctx, i.fileState.file, flags)
 	if err != nil {
 		return nil, err
 	}
-	return NewFile(ctx, d, d.BaseName(), flags, i, h), nil
+	return fs.NewFile(ctx, d, flags, pipeOps), nil
 }
 
 // errNotHostFile indicates that the file is not a host file.
@@ -454,24 +433,10 @@ func (i *inodeOperations) NonBlockingOpen(ctx context.Context, p fs.PermMask) (*
 }
 
 func (i *inodeOperations) getFileDefault(ctx context.Context, d *fs.Dirent, flags fs.FileFlags) (*fs.File, error) {
-	if !i.session().cachePolicy.cacheHandles(d.Inode) {
-		h, err := newHandles(ctx, i.fileState.file, flags)
-		if err != nil {
-			return nil, err
-		}
-		return NewFile(ctx, d, d.BaseName(), flags, i, h), nil
+	h, err := i.fileState.getHandles(ctx, flags)
+	if err != nil {
+		return nil, err
 	}
-
-	h, ok := i.fileState.getCachedHandles(ctx, flags, d.Inode.MountSource)
-	if !ok {
-		var err error
-		h, err = newHandles(ctx, i.fileState.file, flags)
-		if err != nil {
-			return nil, err
-		}
-	}
-	i.fileState.setHandlesForCachedIO(flags, h)
-
 	return NewFile(ctx, d, d.BaseName(), flags, i, h), nil
 }
 

pkg/sentry/fs/gofer/path.go

@@ -129,8 +129,10 @@ func (i *inodeOperations) Create(ctx context.Context, dir *fs.Inode, name string
 		File: newFile,
 		Host: hostFile,
 	}
-	if iops.session().cachePolicy.cacheHandles(d.Inode) {
-		iops.fileState.setHandlesForCachedIO(flags, h)
+	if iops.fileState.canShareHandles() {
+		iops.fileState.handlesMu.Lock()
+		iops.fileState.setSharedHandlesLocked(flags, h)
+		iops.fileState.handlesMu.Unlock()
 	}
 	return NewFile(ctx, d, name, flags, iops, h), nil
 }

test/syscalls/linux/mmap.cc

@@ -1696,6 +1696,29 @@ TEST(MMapDeathTest, TruncateAfterCOWBreak) {
               ::testing::KilledBySignal(SIGBUS), "");
 }
 
+// Regression test for #147.
+TEST(MMapNoFixtureTest, MapReadOnlyAfterCreateWriteOnly) {
+  std::string filename = NewTempAbsPath();
+
+  // We have to create the file O_RDONLY to reproduce the bug because
+  // fsgofer.localFile.Create() silently upgrades O_WRONLY to O_RDWR, causing
+  // the cached "write-only" FD to be read/write and therefore usable by mmap().
+  auto const ro_fd = ASSERT_NO_ERRNO_AND_VALUE(
+      Open(filename, O_RDONLY | O_CREAT | O_EXCL, 0666));
+
+  // Get a write-only FD for the same file, which should be ignored by mmap()
+  // (but isn't in #147).
+  auto const wo_fd = ASSERT_NO_ERRNO_AND_VALUE(Open(filename, O_WRONLY));
+  ASSERT_THAT(ftruncate(wo_fd.get(), kPageSize), SyscallSucceeds());
+
+  auto const mapping = ASSERT_NO_ERRNO_AND_VALUE(
+      Mmap(nullptr, kPageSize, PROT_READ, MAP_SHARED, ro_fd.get(), 0));
+  std::vector<char> buf(kPageSize);
+  // The test passes if this survives.
+  std::copy(static_cast<char*>(mapping.ptr()),
+            static_cast<char*>(mapping.endptr()), buf.data());
+}
+
 // Conditional on MAP_32BIT.
 #ifdef __x86_64__