From ecd94ea7a693d49a0edce8607241a8e2ac22bfe0 Mon Sep 17 00:00:00 2001
From: Nicolas Lacasse <nlacasse@google.com>
Date: Mon, 15 Oct 2018 17:41:34 -0700
Subject: Clean up Rename and Unlink checks for EBUSY.

- Change Dirent.Busy => Dirent.isMountPoint. The function body is unchanged,
  and it is no longer exported.

- fs.MayDelete now checks that the victim is not the process root. This aligns
  with Linux's namei.c:may_delete().

- Fix "is-ancestor" checks to actually compare all ancestors, not just the
  parents.

- Fix handling of paths that end in dots, which are handled differently in
  Rename vs. Unlink.

PiperOrigin-RevId: 217239274
Change-Id: I7a0eb768e70a1b2915017ce54f7f95cbf8edf1fb
---
 pkg/sentry/fs/dirent.go | 75 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 24 deletions(-)

(limited to 'pkg/sentry/fs')

diff --git a/pkg/sentry/fs/dirent.go b/pkg/sentry/fs/dirent.go
index 7191c5c30..a42c03e98 100644
--- a/pkg/sentry/fs/dirent.go
+++ b/pkg/sentry/fs/dirent.go
@@ -1027,11 +1027,14 @@ func (d *Dirent) flush() {
 	}
 }
 
-// Busy indicates whether this Dirent is a mount point or root dirent.
-func (d *Dirent) Busy() bool {
+// isMountPoint returns true if the dirent is a mount point or the root.
+func (d *Dirent) isMountPoint() bool {
 	d.mu.Lock()
 	defer d.mu.Unlock()
+	return d.isMountPointLocked()
+}
 
+func (d *Dirent) isMountPointLocked() bool {
 	return d.mounted || d.parent == nil
 }
 
@@ -1137,7 +1140,7 @@ func (d *Dirent) Remove(ctx context.Context, root *Dirent, name string) error {
 	}
 
 	// Remove cannot remove a mount point.
-	if child.Busy() {
+	if child.isMountPoint() {
 		return syscall.EBUSY
 	}
 
@@ -1211,7 +1214,7 @@ func (d *Dirent) RemoveDirectory(ctx context.Context, root *Dirent, name string)
 	}
 
 	// Remove cannot remove a mount point.
-	if child.Busy() {
+	if child.isMountPoint() {
 		return syscall.EBUSY
 	}
 
@@ -1457,12 +1460,20 @@ func MayDelete(ctx context.Context, root, dir *Dirent, name string) error {
 	return mayDelete(ctx, dir, victim)
 }
 
-func mayDelete(ctx context.Context, dir *Dirent, victim *Dirent) error {
+func mayDelete(ctx context.Context, dir, victim *Dirent) error {
 	if err := dir.Inode.CheckPermission(ctx, PermMask{Write: true, Execute: true}); err != nil {
 		return err
 	}
 
-	return checkSticky(ctx, dir, victim)
+	if err := checkSticky(ctx, dir, victim); err != nil {
+		return err
+	}
+
+	if victim.IsRoot() {
+		return syserror.EBUSY
+	}
+
+	return nil
 }
 
 // Rename atomically converts the child of oldParent named oldName to a
@@ -1491,33 +1502,28 @@ func Rename(ctx context.Context, root *Dirent, oldParent *Dirent, oldName string
 		return syscall.ENOENT
 	}
 
-	// Check constraints on the object being renamed.
+	// renamed is the dirent that will be renamed to something else.
 	renamed, err := oldParent.walk(ctx, root, oldName, false /* may unlock */)
 	if err != nil {
 		return err
 	}
 	defer renamed.DecRef()
 
-	// Make sure we have write permissions on old and new parent.
+	// Check that the renamed dirent is deletable.
 	if err := mayDelete(ctx, oldParent, renamed); err != nil {
 		return err
 	}
-	if newParent != oldParent {
-		if err := newParent.Inode.CheckPermission(ctx, PermMask{Write: true, Execute: true}); err != nil {
-			return err
-		}
+
+	// Check that the renamed dirent is not a mount point.
+	if renamed.isMountPointLocked() {
+		return syscall.EBUSY
 	}
 
 	// Source should not be an ancestor of the target.
-	if renamed == newParent {
+	if newParent.descendantOf(renamed) {
 		return syscall.EINVAL
 	}
 
-	// Is the thing we're trying to rename busy?
-	if renamed.Busy() {
-		return syscall.EBUSY
-	}
-
 	// Per rename(2): "... EACCES: ... or oldpath is a directory and does not
 	// allow write permission (needed to update the .. entry)."
 	if IsDir(renamed.Inode.StableAttr) {
@@ -1526,21 +1532,42 @@ func Rename(ctx context.Context, root *Dirent, oldParent *Dirent, oldName string
 		}
 	}
 
-	// Check constraints on the object being replaced, if any.
+	// replaced is the dirent that is being overwritten by rename.
 	replaced, err := newParent.walk(ctx, root, newName, false /* may unlock */)
-	if err == nil {
+	if err != nil {
+		if err != syserror.ENOENT {
+			return err
+		}
+
+		// Make sure we can create a new child in the new parent.
+		if err := newParent.Inode.CheckPermission(ctx, PermMask{Write: true, Execute: true}); err != nil {
+			return err
+		}
+	} else {
+		// Check constraints on the dirent being replaced.
+
 		// NOTE: We don't want to keep replaced alive
 		// across the Rename, so must call DecRef manually (no defer).
 
+		// Check that we can delete replaced.
+		if err := mayDelete(ctx, oldParent, renamed); err != nil {
+			replaced.DecRef()
+			return err
+		}
+
 		// Target should not be an ancestor of source.
-		if replaced == oldParent {
+		if oldParent.descendantOf(replaced) {
 			replaced.DecRef()
-			// Why is this not EINVAL? See fs/namei.c.
+
+			// Note that Linux returns EINVAL if the source is an
+			// ancestor of target, but ENOTEMPTY if the target is
+			// an ancestor of source (unless RENAME_EXCHANGE flag
+			// is present).  See fs/namei.c:renameat2.
 			return syscall.ENOTEMPTY
 		}
 
-		// Is the thing we're trying to replace busy?
-		if replaced.Busy() {
+		// Check that replaced is not a mount point.
+		if replaced.isMountPointLocked() {
 			replaced.DecRef()
 			return syscall.EBUSY
 		}
-- 
cgit v1.2.3