Fixed some races and misleading comments in ufs_rename().

1. When a directory is renamed to an existing (empty) directory,
it is possible for the target vnode to become the source vnode
underneath you (because another process may complete the same
rename).  It was assumed that this can't happen, and the bogus
errno EINVAL was returned.  This was fairly harmless.

Fix: return ENOENT instead, as if the source directory was renamed
a little earlier.

2. The same metamorphosis is possible for non-directories.  It was
assumed that this can't happen, and the code for handling "just
removing a link name" happened to be used.  This would have worked
except for fatal bugs in the link name removal - the link name was
assumed to still be there, and a null pointer was followed.

Fix: check the result of relookup().  This fixes PR 1930.

Notes:

(a) POSIX seems to say that removing link names shall have no effect.
BSD (4.4Lite2 at least) does something reasonable instead.

(b) The relookup() may find a file unrelated to the original.
Removing this isn't correct.  Consider 3 existing files A, B and
C, and concurrent renames: AB = rename(A, B), another AB, and
CA = rename("c", "a").  If rename() is atomic, then only the
following results are possible:

	AB, AB (fails), CA: A = original C, B = original A, C = gone
	AB, CA, AB:         A = gone,       B = original C, C = gone
	CA, AB, AB (fails): A = gone,       B = original C, C = gone

but ufs_rename() can give:

	A,AB,CA,B (sorta):  A = gone,       B = original A, C = gone

This usually doesn't matter, since getting into a race is usually
an error.
---

These fixes should be in 2.1.6 and 2.2.

1 files changed, 30 insertions, 6 deletions

diff --git a/sys/ufs/ufs/ufs_vnops.c b/sys/ufs/ufs/ufs_vnops.c
index 7712a8e..91d7631 100644
--- a/sys/ufs/ufs/ufs_vnops.c
+++ b/sys/ufs/ufs/ufs_vnops.c
@@ -36,7 +36,7 @@
  * SUCH DAMAGE.
  *
  *	@(#)ufs_vnops.c	8.10 (Berkeley) 4/1/94
- * $Id: ufs_vnops.c,v 1.40 1996/09/03 14:25:27 bde Exp $
+ * $Id: ufs_vnops.c,v 1.41 1996/09/19 18:21:32 nate Exp $
  */
 
 #include "opt_quota.h"
@@ -842,17 +842,30 @@ abortit:
 		return (error);
 	}
 
-	/*
-	 * Check if just deleting a link name.
-	 */
 	if (tvp && ((VTOI(tvp)->i_flags & (IMMUTABLE | APPEND)) ||
 	    (VTOI(tdvp)->i_flags & APPEND))) {
 		error = EPERM;
 		goto abortit;
 	}
+
+	/*
+	 * Check if just deleting a link name or if we've lost a race.
+	 * If another process completes the same rename after we've looked
+	 * up the source and have blocked looking up the target, then the
+	 * source and target inodes may be identical now although the
+	 * names were never linked.
+	 */
 	if (fvp == tvp) {
 		if (fvp->v_type == VDIR) {
-			error = EINVAL;
+			/*
+			 * Linked directories are impossible, so we must
+			 * have lost the race.  Pretend that the rename
+			 * completed before the lookup.
+			 */
+#ifdef UFS_RENAME_DEBUG
+			printf("ufs_rename: fvp == tvp for directories\n");
+#endif
+			error = ENOENT;
 			goto abortit;
 		}
 
@@ -861,7 +874,12 @@ abortit:
 		vput(tdvp);
 		vput(tvp);
 
-		/* Delete source. */
+		/*
+		 * Delete source.  There is another race now that everything
+		 * is unlocked, but this doesn't cause any new complications.
+		 * Relookup() may find a file that is unrelated to the
+		 * original one, or it may fail.  Too bad.
+		 */
 		vrele(fdvp);
 		vrele(fvp);
 		fcnp->cn_flags &= ~MODMASK;
@@ -873,6 +891,12 @@ abortit:
 		error = relookup(fdvp, &fvp, fcnp);
 		if (error == 0)
 			vrele(fdvp);
+		if (fvp == NULL) {
+#ifdef UFS_RENAME_DEBUG
+			printf("ufs_rename: from name disappeared\n");
+#endif
+			return (ENOENT);
+		}
 		return (VOP_REMOVE(fdvp, fvp, fcnp));
 	}
 	error = VOP_LOCK(fvp);