fs: Allow overlays to revalidate files from the upper fs.

Previously, an overlay would panic if either the upper or lower fs required
revalidation for a given Dirent. Now, we allow revalidation from the upper
file, but not the lower.

If a cached overlay inode does need revalidation (because the upper needs
revalidation), then the entire overlay Inode will be discarded and a new
overlay Inode will be built with a fresh copy of the upper file.

As a side effect of this change, Revalidate must take an Inode instead of a
Dirent, since an overlay needs to revalidate individual Inodes.

PiperOrigin-RevId: 208293638
Change-Id: Ic8f8d1ffdc09114721745661a09522b54420c5f1

11 files changed, 279 insertions, 56 deletions

diff --git a/pkg/sentry/fs/README.md b/pkg/sentry/fs/README.md
index 76638cdae..7680187f4 100644
--- a/pkg/sentry/fs/README.md
+++ b/pkg/sentry/fs/README.md
@@ -193,6 +193,18 @@ interface. It multiplexes between upper and lower directory memory mappings and
 stores a copy of memory references so they can be transferred to the upper
 directory `fs.Mappable` when the file is copied up.
 
+The lower filesystem in an overlay may contain another (nested) overlay, but the
+upper filesystem may not contain another overlay. In other words, nested
+overlays form a tree structure that only allows branching in the lower
+filesystem.
+
+Caching decisions in the overlay are delegated to the upper filesystem, meaning
+that the Keep and Revalidate methods on the overlay return the same values as
+the upper filesystem. A small wrinkle is that the lower filesystem is not
+allowed to return `true` from Revalidate, as the overlay can not reload inodes
+from the lower filesystem. A lower filesystem that does return `true` from
+Revalidate will trigger a panic.
+
 The `fs.Inode` also holds a reference to a `fs.MountedFilesystem` that
 normalizes across the mounted filesystem state of the upper and lower
 directories.
diff --git a/pkg/sentry/fs/dirent.go b/pkg/sentry/fs/dirent.go
index 821cc5789..f81ad5792 100644
--- a/pkg/sentry/fs/dirent.go
+++ b/pkg/sentry/fs/dirent.go
@@ -503,7 +503,7 @@ func (d *Dirent) walk(ctx context.Context, root *Dirent, name string, walkMayUnl
 			//
 			// We never allow the file system to revalidate mounts, that could cause them
 			// to unexpectedly drop out before umount.
-			if cd.mounted || !cd.Inode.MountSource.Revalidate(ctx, cd) {
+			if cd.mounted || !cd.Inode.MountSource.Revalidate(ctx, cd.Inode) {
 				// Good to go. This is the fast-path.
 				return cd, nil
 			}
diff --git a/pkg/sentry/fs/file_overlay_test.go b/pkg/sentry/fs/file_overlay_test.go
index 407ba8562..38762d8a1 100644
--- a/pkg/sentry/fs/file_overlay_test.go
+++ b/pkg/sentry/fs/file_overlay_test.go
@@ -21,6 +21,7 @@ import (
 	"gvisor.googlesource.com/gvisor/pkg/sentry/context"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/context/contexttest"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/fs"
+	ramfstest "gvisor.googlesource.com/gvisor/pkg/sentry/fs/ramfs/test"
 )
 
 func TestReaddir(t *testing.T) {
@@ -48,7 +49,7 @@ func TestReaddir(t *testing.T) {
 					{name: "a"},
 					{name: "b"},
 				}, nil), /* lower */
-			),
+				false /* revalidate */),
 			names: []string{".", "..", "a", "b"},
 		},
 		{
@@ -59,7 +60,7 @@ func TestReaddir(t *testing.T) {
 					{name: "b"},
 				}, nil), /* upper */
 				nil, /* lower */
-			),
+				false /* revalidate */),
 			names: []string{".", "..", "a", "b"},
 		},
 		{
@@ -67,11 +68,11 @@ func TestReaddir(t *testing.T) {
 			dir: fs.NewTestOverlayDir(ctx,
 				newTestRamfsDir(ctx, []dirContent{
 					{name: "a"},
-				}, nil), /* lower */
+				}, nil), /* upper */
 				newTestRamfsDir(ctx, []dirContent{
 					{name: "b"},
 				}, nil), /* lower */
-			),
+				false /* revalidate */),
 			names: []string{".", "..", "a", "b"},
 		},
 		{
@@ -79,11 +80,11 @@ func TestReaddir(t *testing.T) {
 			dir: fs.NewTestOverlayDir(ctx,
 				newTestRamfsDir(ctx, []dirContent{
 					{name: "a"},
-				}, []string{"b"}), /* lower */
+				}, []string{"b"}), /* upper */
 				newTestRamfsDir(ctx, []dirContent{
 					{name: "c"},
 				}, nil), /* lower */
-			),
+				false /* revalidate */),
 			names: []string{".", "..", "a", "c"},
 		},
 		{
@@ -91,12 +92,12 @@ func TestReaddir(t *testing.T) {
 			dir: fs.NewTestOverlayDir(ctx,
 				newTestRamfsDir(ctx, []dirContent{
 					{name: "a"},
-				}, []string{"b"}), /* lower */
+				}, []string{"b"}), /* upper */
 				newTestRamfsDir(ctx, []dirContent{
 					{name: "b"}, /* will be masked */
 					{name: "c"},
 				}, nil), /* lower */
-			),
+				false /* revalidate */),
 			names: []string{".", "..", "a", "c"},
 		},
 	} {
@@ -120,6 +121,59 @@ func TestReaddir(t *testing.T) {
 	}
 }
 
+func TestReaddirRevalidation(t *testing.T) {
+	ctx := contexttest.Context(t)
+	ctx = &rootContext{
+		Context: ctx,
+		root:    fs.NewDirent(newTestRamfsDir(ctx, nil, nil), "root"),
+	}
+
+	// Create an overlay with two directories, each with one file.
+	upper := newTestRamfsDir(ctx, []dirContent{{name: "a"}}, nil)
+	lower := newTestRamfsDir(ctx, []dirContent{{name: "b"}}, nil)
+	overlay := fs.NewTestOverlayDir(ctx, upper, lower, true /* revalidate */)
+
+	// Get a handle to the dirent in the upper filesystem so that we can
+	// modify it without going through the dirent.
+	upperDir := upper.InodeOperations.(*dir).InodeOperations.(*ramfstest.Dir)
+
+	// Check that overlay returns the files from both upper and lower.
+	openDir, err := overlay.GetFile(ctx, fs.NewDirent(overlay, "stub"), fs.FileFlags{Read: true})
+	if err != nil {
+		t.Fatalf("GetFile got error %v, want nil", err)
+	}
+	ser := &fs.CollectEntriesSerializer{}
+	if err := openDir.Readdir(ctx, ser); err != nil {
+		t.Fatalf("Readdir got error %v, want nil", err)
+	}
+	got, want := ser.Order, []string{".", "..", "a", "b"}
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("Readdir got names %v, want %v", got, want)
+	}
+
+	// Remove "a" from the upper and add "c".
+	if err := upperDir.Remove(ctx, upper, "a"); err != nil {
+		t.Fatalf("error removing child: %v", err)
+	}
+	upperDir.AddChild(ctx, "c", fs.NewInode(ramfstest.NewFile(ctx, fs.FilePermissions{}),
+		upper.MountSource, fs.StableAttr{Type: fs.RegularFile}))
+
+	// Seek to beginning of the directory and do the readdir again.
+	if _, err := openDir.Seek(ctx, fs.SeekSet, 0); err != nil {
+		t.Fatalf("error seeking to beginning of dir: %v", err)
+	}
+	ser = &fs.CollectEntriesSerializer{}
+	if err := openDir.Readdir(ctx, ser); err != nil {
+		t.Fatalf("Readdir got error %v, want nil", err)
+	}
+
+	// Readdir should return the updated children.
+	got, want = ser.Order, []string{".", "..", "b", "c"}
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("Readdir got names %v, want %v", got, want)
+	}
+}
+
 type rootContext struct {
 	context.Context
 	root *fs.Dirent
diff --git a/pkg/sentry/fs/gofer/session.go b/pkg/sentry/fs/gofer/session.go
index bfb1154dc..eeb9087e9 100644
--- a/pkg/sentry/fs/gofer/session.go
+++ b/pkg/sentry/fs/gofer/session.go
@@ -145,12 +145,12 @@ func (s *session) Destroy() {
 	s.conn.Close()
 }
 
-// Revalidate returns true if the cache policy is does not allow for VFS caching.
-func (s *session) Revalidate(ctx context.Context, d *fs.Dirent) bool {
+// Revalidate implements MountSource.Revalidate.
+func (s *session) Revalidate(ctx context.Context, i *fs.Inode) bool {
 	return s.cachePolicy.revalidateDirent()
 }
 
-// TakeRefs takes an extra reference on dirent if possible.
+// Keep implements MountSource.Keep.
 func (s *session) Keep(d *fs.Dirent) bool {
 	return s.cachePolicy.keepDirent(d.Inode)
 }
diff --git a/pkg/sentry/fs/inode_overlay.go b/pkg/sentry/fs/inode_overlay.go
index 543db9ac7..34e62a4a2 100644
--- a/pkg/sentry/fs/inode_overlay.go
+++ b/pkg/sentry/fs/inode_overlay.go
@@ -57,6 +57,10 @@ func overlayLookup(ctx context.Context, parent *overlayEntry, inode *Inode, name
 	var upperInode *Inode
 	var lowerInode *Inode
 
+	// We must remember whether the upper fs returned a negative dirent,
+	// because it is only safe to return one if the upper did.
+	var negativeUpperChild bool
+
 	// Does the parent directory exist in the upper file system?
 	if parent.upper != nil {
 		// First check if a file object exists in the upper file system.
@@ -70,7 +74,9 @@ func overlayLookup(ctx context.Context, parent *overlayEntry, inode *Inode, name
 			return nil, err
 		}
 		if child != nil {
-			if !child.IsNegative() {
+			if child.IsNegative() {
+				negativeUpperChild = true
+			} else {
 				upperInode = child.Inode
 				upperInode.IncRef()
 			}
@@ -81,7 +87,18 @@ func overlayLookup(ctx context.Context, parent *overlayEntry, inode *Inode, name
 		if overlayHasWhiteout(parent.upper, name) {
 			if upperInode == nil {
 				parent.copyMu.RUnlock()
-				return NewNegativeDirent(name), nil
+				if negativeUpperChild {
+					// If the upper fs returnd a negative
+					// Dirent, then the upper is OK with
+					// that negative Dirent being cached in
+					// the Dirent tree, so we can return
+					// one from the overlay.
+					return NewNegativeDirent(name), nil
+				}
+				// Upper fs is not OK with a negative Dirent
+				// being cached in the Dirent tree, so don't
+				// return one.
+				return nil, syserror.ENOENT
 			}
 			entry, err := newOverlayEntry(ctx, upperInode, nil, false)
 			if err != nil {
@@ -127,9 +144,14 @@ func overlayLookup(ctx context.Context, parent *overlayEntry, inode *Inode, name
 
 	// Was all of this for naught?
 	if upperInode == nil && lowerInode == nil {
-		// Return a negative Dirent indicating that nothing was found.
 		parent.copyMu.RUnlock()
-		return NewNegativeDirent(name), nil
+		// We can only return a negative dirent if the upper returned
+		// one as well. See comments above regarding negativeUpperChild
+		// for more info.
+		if negativeUpperChild {
+			return NewNegativeDirent(name), nil
+		}
+		return nil, syserror.ENOENT
 	}
 
 	// Did we find a lower Inode? Remember this because we may decide we don't
@@ -568,10 +590,19 @@ func overlayHandleOps(o *overlayEntry) HandleOperations {
 }
 
 // NewTestOverlayDir returns an overlay Inode for tests.
-func NewTestOverlayDir(ctx context.Context, upper *Inode, lower *Inode) *Inode {
+//
+// If `revalidate` is true, then the upper filesystem will require
+// revalidation.
+func NewTestOverlayDir(ctx context.Context, upper, lower *Inode, revalidate bool) *Inode {
 	fs := &overlayFilesystem{}
+	var upperMsrc *MountSource
+	if revalidate {
+		upperMsrc = NewRevalidatingMountSource(fs, MountSourceFlags{})
+	} else {
+		upperMsrc = NewNonCachingMountSource(fs, MountSourceFlags{})
+	}
 	msrc := NewMountSource(&overlayMountSourceOperations{
-		upper: NewNonCachingMountSource(fs, MountSourceFlags{}),
+		upper: upperMsrc,
 		lower: NewNonCachingMountSource(fs, MountSourceFlags{}),
 	}, fs, MountSourceFlags{})
 	overlay := &overlayEntry{
diff --git a/pkg/sentry/fs/inode_overlay_test.go b/pkg/sentry/fs/inode_overlay_test.go
index 684d54bd2..a7be9d040 100644
--- a/pkg/sentry/fs/inode_overlay_test.go
+++ b/pkg/sentry/fs/inode_overlay_test.go
@@ -35,7 +35,6 @@ func TestLookup(t *testing.T) {
 		name string
 
 		// Want from lookup.
-		err      error
 		found    bool
 		hasUpper bool
 		hasLower bool
@@ -50,7 +49,7 @@ func TestLookup(t *testing.T) {
 						dir:  false,
 					},
 				}, nil), /* lower */
-			),
+				false /* revalidate */),
 			name:     "a",
 			found:    true,
 			hasUpper: false,
@@ -66,7 +65,7 @@ func TestLookup(t *testing.T) {
 					},
 				}, nil), /* upper */
 				nil, /* lower */
-			),
+				false /* revalidate */),
 			name:     "a",
 			found:    true,
 			hasUpper: true,
@@ -87,7 +86,7 @@ func TestLookup(t *testing.T) {
 						dir:  false,
 					},
 				}, nil), /* lower */
-			),
+				false /* revalidate */),
 			name:     "a",
 			found:    true,
 			hasUpper: false,
@@ -108,7 +107,7 @@ func TestLookup(t *testing.T) {
 						dir:  false,
 					},
 				}, nil), /* lower */
-			),
+				false /* revalidate */),
 			name:     "a",
 			found:    true,
 			hasUpper: true,
@@ -129,7 +128,7 @@ func TestLookup(t *testing.T) {
 						dir:  false,
 					},
 				}, nil), /* lower */
-			),
+				false /* revalidate */),
 			name:     "a",
 			found:    true,
 			hasUpper: true,
@@ -150,7 +149,7 @@ func TestLookup(t *testing.T) {
 						dir:  true,
 					},
 				}, nil), /* lower */
-			),
+				false /* revalidate */),
 			name:     "a",
 			found:    true,
 			hasUpper: true,
@@ -166,7 +165,7 @@ func TestLookup(t *testing.T) {
 						dir:  false,
 					},
 				}, nil), /* lower */
-			),
+				false /* revalidate */),
 			name:     "a",
 			found:    false,
 			hasUpper: false,
@@ -182,7 +181,7 @@ func TestLookup(t *testing.T) {
 						dir:  false,
 					},
 				}, nil), /* lower */
-			),
+				false /* revalidate */),
 			name:     "a",
 			found:    true,
 			hasUpper: false,
@@ -191,13 +190,14 @@ func TestLookup(t *testing.T) {
 	} {
 		t.Run(test.desc, func(t *testing.T) {
 			dirent, err := test.dir.Lookup(ctx, test.name)
-			if err != test.err {
-				t.Fatalf("lookup got error %v, want %v", err, test.err)
-			}
-			if test.found && dirent.IsNegative() {
-				t.Fatalf("lookup expected to find %q, got negative dirent", test.name)
+			if test.found && (err == syserror.ENOENT || dirent.IsNegative()) {
+				t.Fatalf("lookup %q expected to find positive dirent, got dirent %v err %v", test.name, dirent, err)
 			}
 			if !test.found {
+				if err != syserror.ENOENT && !dirent.IsNegative() {
+					t.Errorf("lookup %q expected to return ENOENT or negative dirent, got dirent %v err %v", test.name, dirent, err)
+				}
+				// Nothing more to check.
 				return
 			}
 			if hasUpper := dirent.Inode.TestHasUpperFS(); hasUpper != test.hasUpper {
@@ -210,6 +210,95 @@ func TestLookup(t *testing.T) {
 	}
 }
 
+func TestLookupRevalidation(t *testing.T) {
+	// File name used in the tests.
+	fileName := "foofile"
+	ctx := contexttest.Context(t)
+	for _, tc := range []struct {
+		// Test description.
+		desc string
+
+		// Upper and lower fs for the overlay.
+		upper *fs.Inode
+		lower *fs.Inode
+
+		// Whether the upper requires revalidation.
+		revalidate bool
+
+		// Whether we should get the same dirent on second lookup.
+		wantSame bool
+	}{
+		{
+			desc:       "file from upper with no revalidation",
+			upper:      newTestRamfsDir(ctx, []dirContent{{name: fileName}}, nil),
+			lower:      newTestRamfsDir(ctx, nil, nil),
+			revalidate: false,
+			wantSame:   true,
+		},
+		{
+			desc:       "file from upper with revalidation",
+			upper:      newTestRamfsDir(ctx, []dirContent{{name: fileName}}, nil),
+			lower:      newTestRamfsDir(ctx, nil, nil),
+			revalidate: true,
+			wantSame:   false,
+		},
+		{
+			desc:       "file from lower with no revalidation",
+			upper:      newTestRamfsDir(ctx, nil, nil),
+			lower:      newTestRamfsDir(ctx, []dirContent{{name: fileName}}, nil),
+			revalidate: false,
+			wantSame:   true,
+		},
+		{
+			desc:       "file from lower with revalidation",
+			upper:      newTestRamfsDir(ctx, nil, nil),
+			lower:      newTestRamfsDir(ctx, []dirContent{{name: fileName}}, nil),
+			revalidate: true,
+			// The file does not exist in the upper, so we do not
+			// need to revalidate it.
+			wantSame: true,
+		},
+		{
+			desc:       "file from upper and lower with no revalidation",
+			upper:      newTestRamfsDir(ctx, []dirContent{{name: fileName}}, nil),
+			lower:      newTestRamfsDir(ctx, []dirContent{{name: fileName}}, nil),
+			revalidate: false,
+			wantSame:   true,
+		},
+		{
+			desc:       "file from upper and lower with revalidation",
+			upper:      newTestRamfsDir(ctx, []dirContent{{name: fileName}}, nil),
+			lower:      newTestRamfsDir(ctx, []dirContent{{name: fileName}}, nil),
+			revalidate: true,
+			wantSame:   false,
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			root := fs.NewDirent(newTestRamfsDir(ctx, nil, nil), "root")
+			ctx = &rootContext{
+				Context: ctx,
+				root:    root,
+			}
+			overlay := fs.NewDirent(fs.NewTestOverlayDir(ctx, tc.upper, tc.lower, tc.revalidate), "overlay")
+			// Lookup the file twice through the overlay.
+			first, err := overlay.Walk(ctx, root, fileName)
+			if err != nil {
+				t.Fatalf("overlay.Walk(%q) failed: %v", fileName, err)
+			}
+			second, err := overlay.Walk(ctx, root, fileName)
+			if err != nil {
+				t.Fatalf("overlay.Walk(%q) failed: %v", fileName, err)
+			}
+
+			if tc.wantSame && first != second {
+				t.Errorf("dirent lookup got different dirents, wanted same\nfirst=%+v\nsecond=%+v", first, second)
+			} else if !tc.wantSame && first == second {
+				t.Errorf("dirent lookup got the same dirent, wanted different: %+v", first)
+			}
+		})
+	}
+}
+
 type dir struct {
 	fs.InodeOperations
 
@@ -231,6 +320,10 @@ type dirContent struct {
 	dir  bool
 }
 
+func newTestRamfsInode(ctx context.Context, msrc *fs.MountSource) *fs.Inode {
+	return fs.NewInode(ramfstest.NewFile(ctx, fs.FilePermissions{}), msrc, fs.StableAttr{Type: fs.RegularFile})
+}
+
 func newTestRamfsDir(ctx context.Context, contains []dirContent, negative []string) *fs.Inode {
 	msrc := fs.NewCachingMountSource(nil, fs.MountSourceFlags{})
 	contents := make(map[string]*fs.Inode)
@@ -238,7 +331,7 @@ func newTestRamfsDir(ctx context.Context, contains []dirContent, negative []stri
 		if c.dir {
 			contents[c.name] = newTestRamfsDir(ctx, nil, nil)
 		} else {
-			contents[c.name] = fs.NewInode(ramfstest.NewFile(ctx, fs.FilePermissions{}), msrc, fs.StableAttr{Type: fs.RegularFile})
+			contents[c.name] = newTestRamfsInode(ctx, msrc)
 		}
 	}
 	dops := ramfstest.NewDir(ctx, contents, fs.FilePermissions{
diff --git a/pkg/sentry/fs/mock.go b/pkg/sentry/fs/mock.go
index dc82a2002..89a0103ba 100644
--- a/pkg/sentry/fs/mock.go
+++ b/pkg/sentry/fs/mock.go
@@ -68,7 +68,7 @@ func NewMockMountSource(cache *DirentCache) *MountSource {
 }
 
 // Revalidate implements fs.MountSourceOperations.Revalidate.
-func (n *MockMountSourceOps) Revalidate(context.Context, *Dirent) bool {
+func (n *MockMountSourceOps) Revalidate(context.Context, *Inode) bool {
 	return n.revalidate
 }
 
diff --git a/pkg/sentry/fs/mount.go b/pkg/sentry/fs/mount.go
index c72372929..455f5b35c 100644
--- a/pkg/sentry/fs/mount.go
+++ b/pkg/sentry/fs/mount.go
@@ -27,10 +27,10 @@ import (
 // DirentOperations provide file systems greater control over how long a Dirent stays pinned
 // in core. Implementations must not take Dirent.mu.
 type DirentOperations interface {
-	// Revalidate returns true if the Dirent is stale and its
+	// Revalidate returns true if the Inode is stale and its
 	// InodeOperations needs to be reloaded. Revalidate will never be
-	// called on a Dirent that is mounted.
-	Revalidate(ctx context.Context, dirent *Dirent) bool
+	// called on a Inode that is mounted.
+	Revalidate(ctx context.Context, inode *Inode) bool
 
 	// Keep returns true if the Dirent should be kept in memory for as long
 	// as possible beyond any active references.
@@ -249,7 +249,8 @@ func (msrc *MountSource) FlushDirentRefs() {
 // aggressively. Filesystem may be nil if there is no backing filesystem.
 func NewCachingMountSource(filesystem Filesystem, flags MountSourceFlags) *MountSource {
 	return NewMountSource(&SimpleMountSourceOperations{
-		keep: true,
+		keep:       true,
+		revalidate: false,
 	}, filesystem, flags)
 }
 
@@ -257,7 +258,17 @@ func NewCachingMountSource(filesystem Filesystem, flags MountSourceFlags) *Mount
 // Filesystem may be nil if there is no backing filesystem.
 func NewNonCachingMountSource(filesystem Filesystem, flags MountSourceFlags) *MountSource {
 	return NewMountSource(&SimpleMountSourceOperations{
-		keep: false,
+		keep:       false,
+		revalidate: false,
+	}, filesystem, flags)
+}
+
+// NewRevalidatingMountSource returns a generic mount that will cache dirents,
+// but will revalidate them on each lookup.
+func NewRevalidatingMountSource(filesystem Filesystem, flags MountSourceFlags) *MountSource {
+	return NewMountSource(&SimpleMountSourceOperations{
+		keep:       true,
+		revalidate: true,
 	}, filesystem, flags)
 }
 
@@ -265,12 +276,13 @@ func NewNonCachingMountSource(filesystem Filesystem, flags MountSourceFlags) *Mo
 //
 // +stateify savable
 type SimpleMountSourceOperations struct {
-	keep bool
+	keep       bool
+	revalidate bool
 }
 
 // Revalidate implements MountSourceOperations.Revalidate.
-func (*SimpleMountSourceOperations) Revalidate(context.Context, *Dirent) bool {
-	return false
+func (smo *SimpleMountSourceOperations) Revalidate(context.Context, *Inode) bool {
+	return smo.revalidate
 }
 
 // Keep implements MountSourceOperations.Keep.
diff --git a/pkg/sentry/fs/mount_overlay.go b/pkg/sentry/fs/mount_overlay.go
index d135e8a37..9fa87c10f 100644
--- a/pkg/sentry/fs/mount_overlay.go
+++ b/pkg/sentry/fs/mount_overlay.go
@@ -14,10 +14,13 @@
 
 package fs
 
-import "gvisor.googlesource.com/gvisor/pkg/sentry/context"
+import (
+	"gvisor.googlesource.com/gvisor/pkg/sentry/context"
+)
 
 // overlayMountSourceOperations implements MountSourceOperations for an overlay
-// mount point.
+// mount point. The upper filesystem determines the caching behavior of the
+// overlay.
 //
 // +stateify savable
 type overlayMountSourceOperations struct {
@@ -34,19 +37,33 @@ func newOverlayMountSource(upper, lower *MountSource, flags MountSourceFlags) *M
 	}, &overlayFilesystem{}, flags)
 }
 
-// Revalidate panics if the upper or lower MountSource require that dirent be
-// revalidated. Otherwise always returns false.
-func (o *overlayMountSourceOperations) Revalidate(ctx context.Context, dirent *Dirent) bool {
-	if o.upper.Revalidate(ctx, dirent) || o.lower.Revalidate(ctx, dirent) {
-		panic("an overlay cannot revalidate file objects")
+// Revalidate implements MountSourceOperations.Revalidate for an overlay by
+// delegating to the upper filesystem's Revalidate method. We cannot reload
+// files from the lower filesystem, so we panic if the lower filesystem's
+// Revalidate method returns true.
+func (o *overlayMountSourceOperations) Revalidate(ctx context.Context, inode *Inode) bool {
+	if inode.overlay == nil {
+		panic("overlay cannot revalidate inode that is not an overlay")
 	}
-	return false
+
+	// Should we bother checking this, or just ignore?
+	if inode.overlay.lower != nil && o.lower.Revalidate(ctx, inode.overlay.lower) {
+		panic("an overlay cannot revalidate file objects from the lower fs")
+	}
+
+	if inode.overlay.upper == nil {
+		// Nothing to revalidate.
+		return false
+	}
+
+	// Does the upper require revalidation?
+	return o.upper.Revalidate(ctx, inode.overlay.upper)
 }
 
-// Keep returns true if either upper or lower MountSource require that the
-// dirent be kept in memory.
+// Keep implements MountSourceOperations by delegating to the upper
+// filesystem's Keep method.
 func (o *overlayMountSourceOperations) Keep(dirent *Dirent) bool {
-	return o.upper.Keep(dirent) || o.lower.Keep(dirent)
+	return o.upper.Keep(dirent)
 }
 
 // ResetInodeMappings propagates the call to both upper and lower MountSource.
diff --git a/pkg/sentry/fs/overlay.go b/pkg/sentry/fs/overlay.go
index af13dc8c7..5a30af419 100644
--- a/pkg/sentry/fs/overlay.go
+++ b/pkg/sentry/fs/overlay.go
@@ -88,10 +88,11 @@ func isXattrOverlay(name string) bool {
 // Preconditions:
 //
 // - upper and lower must be non-nil.
+// - upper must not be an overlay.
 // - lower should not expose character devices, pipes, or sockets, because
 //   copying up these types of files is not supported.
-// - upper and lower must not require that file objects be revalidated.
-// - upper and lower must not have dynamic file/directory content.
+// - lower must not require that file objects be revalidated.
+// - lower must not have dynamic file/directory content.
 func NewOverlayRoot(ctx context.Context, upper *Inode, lower *Inode, flags MountSourceFlags) (*Inode, error) {
 	if !IsDir(upper.StableAttr) {
 		return nil, fmt.Errorf("upper Inode is not a directory")
@@ -99,6 +100,9 @@ func NewOverlayRoot(ctx context.Context, upper *Inode, lower *Inode, flags Mount
 	if !IsDir(lower.StableAttr) {
 		return nil, fmt.Errorf("lower Inode is not a directory")
 	}
+	if upper.overlay != nil {
+		return nil, fmt.Errorf("cannot nest overlay in upper file of another overlay")
+	}
 
 	msrc := newOverlayMountSource(upper.MountSource, lower.MountSource, flags)
 	overlay, err := newOverlayEntry(ctx, upper, lower, true)
diff --git a/pkg/sentry/fs/tty/fs.go b/pkg/sentry/fs/tty/fs.go
index e28635607..fe7da05b5 100644
--- a/pkg/sentry/fs/tty/fs.go
+++ b/pkg/sentry/fs/tty/fs.go
@@ -82,7 +82,7 @@ type superOperations struct{}
 // Slave entries are dropped from dir when their master is closed, so an
 // existing slave Dirent in the tree is not sufficient to guarantee that it
 // still exists on the filesystem.
-func (superOperations) Revalidate(context.Context, *fs.Dirent) bool {
+func (superOperations) Revalidate(context.Context, *fs.Inode) bool {
 	return true
 }