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

2 files changed, 75 insertions, 43 deletions

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
 		}
diff --git a/pkg/sentry/syscalls/linux/sys_file.go b/pkg/sentry/syscalls/linux/sys_file.go
index 015afda9b..64704bb88 100644
--- a/pkg/sentry/syscalls/linux/sys_file.go
+++ b/pkg/sentry/syscalls/linux/sys_file.go
@@ -1042,11 +1042,9 @@ func rmdirAt(t *kernel.Task, dirFD kdefs.FD, addr usermem.Addr) error {
 		return err
 	}
 
-	// Special case: rmdir rejects anything with '.' as last component.
-	// This would be handled by the busy check for the current working
-	// directory, but this is how it's done.
-	if (len(path) == 1 && path == ".") || (len(path) > 1 && path[len(path)-2:] == "/.") {
-		return syserror.EINVAL
+	// Special case: removing the root always returns EBUSY.
+	if path == "/" {
+		return syserror.EBUSY
 	}
 
 	return fileOpAt(t, dirFD, path, func(root *fs.Dirent, d *fs.Dirent, name string) error {
@@ -1054,6 +1052,15 @@ func rmdirAt(t *kernel.Task, dirFD kdefs.FD, addr usermem.Addr) error {
 			return syserror.ENOTDIR
 		}
 
+		// Linux returns different ernos when the path ends in single
+		// dot vs. double dots.
+		switch name {
+		case ".":
+			return syserror.EINVAL
+		case "..":
+			return syserror.ENOTEMPTY
+		}
+
 		if err := fs.MayDelete(t, root, d, name); err != nil {
 			return err
 		}
@@ -1829,27 +1836,25 @@ func renameAt(t *kernel.Task, oldDirFD kdefs.FD, oldAddr usermem.Addr, newDirFD
 			return syserror.ENOTDIR
 		}
 
-		// Root cannot be renamed to anything.
-		//
-		// TODO: This catches the case when the rename
-		// argument is exactly "/", but we should return EBUSY when
-		// renaming any mount point, or when the argument is not
-		// exactly "/" but still resolves to the root, like "/.." or
-		// "/bin/..".
-		if oldParent == root && oldName == "." {
-			return syscall.EBUSY
+		// Rename rejects paths that end in ".", "..", or empty (i.e.
+		// the root) with EBUSY.
+		switch oldName {
+		case "", ".", "..":
+			return syserror.EBUSY
 		}
+
 		return fileOpAt(t, newDirFD, newPath, func(root *fs.Dirent, newParent *fs.Dirent, newName string) error {
 			if !fs.IsDir(newParent.Inode.StableAttr) {
 				return syserror.ENOTDIR
 			}
 
-			// Nothing can be renamed to root.
-			//
-			// TODO: Same as above.
-			if newParent == root && newName == "." {
-				return syscall.EBUSY
+			// Rename rejects paths that end in ".", "..", or empty
+			// (i.e.  the root) with EBUSY.
+			switch newName {
+			case "", ".", "..":
+				return syserror.EBUSY
 			}
+
 			return fs.Rename(t, root, oldParent, oldName, newParent, newName)
 		})
 	})