From 4bc7daf91a0d9102fa477b199964e7db45066da1 Mon Sep 17 00:00:00 2001
From: Jamie Liu <jamieliu@google.com>
Date: Wed, 17 Feb 2021 15:32:02 -0800
Subject: Check for directory emptiness in VFS1 overlay rmdir().

Note that this CL reorders overlayEntry.copyMu before overlayEntry.dirCacheMu
in the overlayFileOperations.IterateDir() => readdirEntries() path - but this
lock ordering is already required by overlayRemove/Bind() =>
overlayEntry.markDirectoryDirty(), so this actually just fixes an
inconsistency.

PiperOrigin-RevId: 358047121
---
 pkg/sentry/fs/file_overlay.go  | 28 ++++++++++++----------------
 pkg/sentry/fs/inode_overlay.go | 13 +++++++++++++
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/pkg/sentry/fs/file_overlay.go b/pkg/sentry/fs/file_overlay.go
index 9dc58d5ff..696613f3a 100644
--- a/pkg/sentry/fs/file_overlay.go
+++ b/pkg/sentry/fs/file_overlay.go
@@ -177,10 +177,16 @@ func (f *overlayFileOperations) Readdir(ctx context.Context, file *File, seriali
 // IterateDir implements DirIterator.IterateDir.
 func (f *overlayFileOperations) IterateDir(ctx context.Context, d *Dirent, dirCtx *DirCtx, offset int) (int, error) {
 	o := d.Inode.overlay
+	o.copyMu.RLock()
+	defer o.copyMu.RUnlock()
+	return overlayIterateDirLocked(ctx, o, d, dirCtx, offset)
+}
 
+// Preconditions: o.copyMu must be locked.
+func overlayIterateDirLocked(ctx context.Context, o *overlayEntry, d *Dirent, dirCtx *DirCtx, offset int) (int, error) {
 	if !d.Inode.MountSource.CacheReaddir() {
 		// Can't use the dirCache. Simply read the entries.
-		entries, err := readdirEntries(ctx, o)
+		entries, err := readdirEntriesLocked(ctx, o)
 		if err != nil {
 			return offset, err
 		}
@@ -198,15 +204,6 @@ func (f *overlayFileOperations) IterateDir(ctx context.Context, d *Dirent, dirCt
 	}
 	o.dirCacheMu.RUnlock()
 
-	// readdirEntries holds o.copyUpMu to ensure that copy-up does not
-	// occur while calculating the readdir results.
-	//
-	// However, it is possible for a copy-up to occur after the call to
-	// readdirEntries, but before setting o.dirCache. This is OK, since
-	// copy-up does not change the children in a way that would affect the
-	// children returned in dirCache. Copy-up only moves files/directories
-	// between layers in the overlay.
-	//
 	// We must hold dirCacheMu around both readdirEntries and setting
 	// o.dirCache to synchronize with dirCache invalidations done by
 	// Create, Remove, Rename.
@@ -216,7 +213,7 @@ func (f *overlayFileOperations) IterateDir(ctx context.Context, d *Dirent, dirCt
 	// chance that a racing call managed to just set it, in which case we
 	// can use that new value.
 	if o.dirCache == nil {
-		dirCache, err := readdirEntries(ctx, o)
+		dirCache, err := readdirEntriesLocked(ctx, o)
 		if err != nil {
 			o.dirCacheMu.Unlock()
 			return offset, err
@@ -444,12 +441,11 @@ func (f *overlayFileOperations) SetFifoSize(size int64) (rv int64, err error) {
 	return sz.SetFifoSize(size)
 }
 
-// readdirEntries returns a sorted map of directory entries from the
+// readdirEntriesLocked returns a sorted map of directory entries from the
 // upper and/or lower filesystem.
-func readdirEntries(ctx context.Context, o *overlayEntry) (*SortedDentryMap, error) {
-	o.copyMu.RLock()
-	defer o.copyMu.RUnlock()
-
+//
+// Preconditions: o.copyMu must be locked.
+func readdirEntriesLocked(ctx context.Context, o *overlayEntry) (*SortedDentryMap, error) {
 	// Assert that there is at least one upper or lower entry.
 	if o.upper == nil && o.lower == nil {
 		panic("invalid overlayEntry, needs at least one Inode")
diff --git a/pkg/sentry/fs/inode_overlay.go b/pkg/sentry/fs/inode_overlay.go
index b16ab08ba..e97afc626 100644
--- a/pkg/sentry/fs/inode_overlay.go
+++ b/pkg/sentry/fs/inode_overlay.go
@@ -333,6 +333,19 @@ func overlayRemove(ctx context.Context, o *overlayEntry, parent *Dirent, child *
 	}
 	child.Inode.overlay.copyMu.RLock()
 	defer child.Inode.overlay.copyMu.RUnlock()
+	if child.Inode.StableAttr.Type == Directory {
+		// RemoveDirectory requires that the directory is empty.
+		ser := &CollectEntriesSerializer{}
+		dirCtx := &DirCtx{
+			Serializer: ser,
+		}
+		if _, err := overlayIterateDirLocked(ctx, child.Inode.overlay, child, dirCtx, 0); err != nil {
+			return err
+		}
+		if ser.Written() != 0 {
+			return syserror.ENOTEMPTY
+		}
+	}
 	if child.Inode.overlay.upper != nil {
 		if child.Inode.StableAttr.Type == Directory {
 			if err := o.upper.InodeOperations.RemoveDirectory(ctx, o.upper, child.name); err != nil {
-- 
cgit v1.2.3