[src/trunk]: src/sys/fs/tmpfs Fix tmpfs_rename locking.

[riastradh <riastradh%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/0d5ddeb0db2b
branches:  trunk
changeset: 768542:0d5ddeb0db2b
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Thu Aug 18 21:42:18 2011 +0000

description:
Fix tmpfs_rename locking.

Fixes PR kern/36681.  tmpfs now survives dirconc, all our vfs/tmpfs
tests and rename races in atf, and a bunch of hand-written tests
that I'd commit if atf didn't find them highly indigestible.

ok dholland

diffstat:

 sys/fs/tmpfs/tmpfs_vnops.c |  1384 ++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 1207 insertions(+), 177 deletions(-)

diffs (truncated from 1457 to 300 lines):

diff -r 45dbbcc4e910 -r 0d5ddeb0db2b sys/fs/tmpfs/tmpfs_vnops.c
--- a/sys/fs/tmpfs/tmpfs_vnops.c        Thu Aug 18 21:04:23 2011 +0000
+++ b/sys/fs/tmpfs/tmpfs_vnops.c        Thu Aug 18 21:42:18 2011 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: tmpfs_vnops.c,v 1.88 2011/07/13 03:28:41 riastradh Exp $       */
+/*     $NetBSD: tmpfs_vnops.c,v 1.89 2011/08/18 21:42:18 riastradh Exp $       */
 
 /*
  * Copyright (c) 2005, 2006, 2007 The NetBSD Foundation, Inc.
@@ -35,7 +35,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: tmpfs_vnops.c,v 1.88 2011/07/13 03:28:41 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: tmpfs_vnops.c,v 1.89 2011/08/18 21:42:18 riastradh Exp $");
 
 #include <sys/param.h>
 #include <sys/dirent.h>
@@ -798,27 +798,8 @@
 }
 
 /*
- * tmpfs_parentcheck_p: check if 'lower' is a descendent of 'upper'.
- *
- * => Returns 'true' if parent, and 'false' otherwise.
- */
-static inline bool
-tmpfs_parentcheck_p(tmpfs_node_t *lower, tmpfs_node_t *upper)
-{
-       tmpfs_node_t *un = lower;
-
-       while (un != un->tn_spec.tn_dir.tn_parent) {
-               KASSERT(un->tn_type == VDIR);
-               if (un == upper) {
-                       return true;
-               }
-               un = un->tn_spec.tn_dir.tn_parent;
-       }
-       return false;
-}
-
-/*
- * tmpfs_rename: rename routine.
+ * tmpfs_rename: rename routine, the hairiest system call, with the
+ * insane API.
  *
  * Arguments: fdvp (from-parent vnode), fvp (from-leaf), tdvp (to-parent)
  * and tvp (to-leaf), if exists (NULL if not).
@@ -829,6 +810,57 @@
  * => Both tdvp and tvp are referenced and locked.  It is our responsibility
  *    to release the references and unlock them (or destroy).
  */
+
+/*
+ * First, some forward declarations of subroutines.
+ */
+
+static int tmpfs_sane_rename(struct vnode *, struct componentname *,
+    struct vnode *, struct componentname *, kauth_cred_t, bool);
+static int tmpfs_rename_enter(struct mount *, struct tmpfs_mount *,
+    kauth_cred_t,
+    struct vnode *, struct tmpfs_node *, struct componentname *,
+    struct tmpfs_dirent **, struct vnode **,
+    struct vnode *, struct tmpfs_node *, struct componentname *,
+    struct tmpfs_dirent **, struct vnode **);
+static int tmpfs_rename_enter_common(struct mount *, struct tmpfs_mount *,
+    kauth_cred_t,
+    struct vnode *, struct tmpfs_node *,
+    struct componentname *, struct tmpfs_dirent **, struct vnode **,
+    struct componentname *, struct tmpfs_dirent **, struct vnode **);
+static int tmpfs_rename_enter_separate(struct mount *, struct tmpfs_mount *,
+    kauth_cred_t,
+    struct vnode *, struct tmpfs_node *, struct componentname *,
+    struct tmpfs_dirent **, struct vnode **,
+    struct vnode *, struct tmpfs_node *, struct componentname *,
+    struct tmpfs_dirent **, struct vnode **);
+static void tmpfs_rename_exit(struct tmpfs_mount *,
+    struct vnode *, struct vnode *, struct vnode *, struct vnode *);
+static int tmpfs_rename_lock_directory(struct vnode *, struct tmpfs_node *);
+static int tmpfs_rename_genealogy(struct tmpfs_node *, struct tmpfs_node *,
+    struct tmpfs_node **);
+static int tmpfs_rename_lock(struct mount *, kauth_cred_t, int,
+    struct vnode *, struct tmpfs_node *, struct componentname *, bool,
+    struct tmpfs_dirent **, struct vnode **,
+    struct vnode *, struct tmpfs_node *, struct componentname *, bool,
+    struct tmpfs_dirent **, struct vnode **);
+static void tmpfs_rename_attachdetach(struct tmpfs_mount *,
+    struct vnode *, struct tmpfs_dirent *, struct vnode *,
+    struct vnode *, struct tmpfs_dirent *, struct vnode *);
+static int tmpfs_do_remove(struct tmpfs_mount *, struct vnode *,
+    struct tmpfs_node *, struct tmpfs_dirent *, struct vnode *, kauth_cred_t);
+static int tmpfs_rename_check_possible(struct tmpfs_node *,
+    struct tmpfs_node *, struct tmpfs_node *, struct tmpfs_node *);
+static int tmpfs_rename_check_permitted(kauth_cred_t,
+    struct tmpfs_node *, struct tmpfs_node *,
+    struct tmpfs_node *, struct tmpfs_node *);
+static int tmpfs_remove_check_possible(struct tmpfs_node *,
+    struct tmpfs_node *);
+static int tmpfs_remove_check_permitted(kauth_cred_t,
+    struct tmpfs_node *, struct tmpfs_node *);
+static int tmpfs_check_sticky(kauth_cred_t,
+    struct tmpfs_node *, struct tmpfs_node *);
+
 int
 tmpfs_rename(void *v)
 {
@@ -840,198 +872,1196 @@
                struct vnode            *a_tvp;
                struct componentname    *a_tcnp;
        } */ *ap = v;
-       vnode_t *fdvp = ap->a_fdvp;
-       vnode_t *fvp = ap->a_fvp;
+       struct vnode *fdvp = ap->a_fdvp;
+       struct vnode *fvp = ap->a_fvp;
        struct componentname *fcnp = ap->a_fcnp;
-       vnode_t *tdvp = ap->a_tdvp;
-       vnode_t *tvp = ap->a_tvp;
+       struct vnode *tdvp = ap->a_tdvp;
+       struct vnode *tvp = ap->a_tvp;
        struct componentname *tcnp = ap->a_tcnp;
-       tmpfs_node_t *fdnode, *fnode, *tnode, *tdnode;
-       tmpfs_dirent_t *de;
-       tmpfs_mount_t *tmp;
-       size_t namelen;
+       kauth_cred_t cred;
+       int error;
+
+       KASSERT(fdvp != NULL);
+       KASSERT(fvp != NULL);
+       KASSERT(fcnp != NULL);
+       KASSERT(fcnp->cn_nameptr != NULL);
+       KASSERT(tdvp != NULL);
+       KASSERT(tcnp != NULL);
+       KASSERT(fcnp->cn_nameptr != NULL);
+       /* KASSERT(VOP_ISLOCKED(fdvp) != LK_EXCLUSIVE); */
+       /* KASSERT(VOP_ISLOCKED(fvp) != LK_EXCLUSIVE); */
+       KASSERT(VOP_ISLOCKED(tdvp) == LK_EXCLUSIVE);
+       KASSERT((tvp == NULL) || (VOP_ISLOCKED(tvp) == LK_EXCLUSIVE));
+       KASSERT(fdvp->v_type == VDIR);
+       KASSERT(tdvp->v_type == VDIR);
+
+       cred = fcnp->cn_cred;
+       KASSERT(tcnp->cn_cred == cred);
+
+       /*
+        * Sanitize our world from the VFS insanity.  Unlock the target
+        * directory and node, which are locked.  Release the children,
+        * which are referenced.  Check for rename("x", "y/."), which
+        * it is our responsibility to reject, not the caller's.  (But
+        * the caller does reject rename("x/.", "y").  Go figure.)
+        */
+
+       VOP_UNLOCK(tdvp);
+       if ((tvp != NULL) && (tvp != tdvp))
+               VOP_UNLOCK(tvp);
+
+       vrele(fvp);
+       if (tvp != NULL)
+               vrele(tvp);
+
+       if (tvp == tdvp) {
+               error = EINVAL;
+               goto out;
+       }
+
+       error = tmpfs_sane_rename(fdvp, fcnp, tdvp, tcnp, cred, false);
+
+out:   /*
+        * All done, whether with success or failure.  Release the
+        * directory nodes now, as the caller expects from the VFS
+        * protocol.
+        */
+       vrele(fdvp);
+       vrele(tdvp);
+
+       return error;
+}
+
+/*
+ * tmpfs_sane_rename: rename routine, the hairiest system call, with
+ * the sane API.
+ *
+ * Arguments:
+ *
+ * . fdvp (from directory vnode),
+ * . fcnp (from component name),
+ * . tdvp (to directory vnode), and
+ * . tcnp (to component name).
+ *
+ * fdvp and tdvp must be referenced and unlocked.
+ */
+static int
+tmpfs_sane_rename(struct vnode *fdvp, struct componentname *fcnp,
+    struct vnode *tdvp, struct componentname *tcnp, kauth_cred_t cred,
+    bool posixly_correct)
+{
+       struct mount *mount;
+       struct tmpfs_mount *tmpfs;
+       struct tmpfs_node *fdnode, *tdnode;
+       struct tmpfs_dirent *fde, *tde;
+       struct vnode *fvp, *tvp;
        char *newname;
        int error;
 
-       KASSERT(VOP_ISLOCKED(tdvp));
-       KASSERT(tvp == NULL || VOP_ISLOCKED(tvp) == LK_EXCLUSIVE);
+       KASSERT(fdvp != NULL);
+       KASSERT(fcnp != NULL);
+       KASSERT(tdvp != NULL);
+       KASSERT(tcnp != NULL);
+       /* KASSERT(VOP_ISLOCKED(fdvp) != LK_EXCLUSIVE); */
+       /* KASSERT(VOP_ISLOCKED(tdvp) != LK_EXCLUSIVE); */
+       KASSERT(fdvp->v_type == VDIR);
+       KASSERT(tdvp->v_type == VDIR);
+       KASSERT(fdvp->v_mount == tdvp->v_mount);
        KASSERT((fcnp->cn_flags & ISDOTDOT) == 0);
        KASSERT((tcnp->cn_flags & ISDOTDOT) == 0);
-
-       newname = NULL;
-       namelen = 0;
-       tmp = NULL;
+       KASSERT((fcnp->cn_namelen != 1) || (fcnp->cn_nameptr[0] != '.'));
+       KASSERT((tcnp->cn_namelen != 1) || (tcnp->cn_nameptr[0] != '.'));
+       KASSERT((fcnp->cn_namelen != 2) || (fcnp->cn_nameptr[0] != '.') ||
+           (fcnp->cn_nameptr[1] != '.'));
+       KASSERT((tcnp->cn_namelen != 2) || (tcnp->cn_nameptr[0] != '.') ||
+           (tcnp->cn_nameptr[1] != '.'));
 
-       /* Disallow cross-device renames. */
-       if (fvp->v_mount != tdvp->v_mount ||
-           (tvp != NULL && fvp->v_mount != tvp->v_mount)) {
-               error = EXDEV;
-               goto out_unlocked;
-       }
+       /*
+        * Pull out the tmpfs data structures.
+        */
+       fdnode = VP_TO_TMPFS_NODE(fdvp);
+       tdnode = VP_TO_TMPFS_NODE(tdvp);
+       KASSERT(fdnode != NULL);
+       KASSERT(tdnode != NULL);
+       KASSERT(fdnode->tn_vnode == fdvp);
+       KASSERT(tdnode->tn_vnode == tdvp);
+       KASSERT(fdnode->tn_type == VDIR);
+       KASSERT(tdnode->tn_type == VDIR);
 
-       fnode = VP_TO_TMPFS_NODE(fvp);
-       fdnode = VP_TO_TMPFS_DIR(fdvp);
-       tnode = (tvp == NULL) ? NULL : VP_TO_TMPFS_NODE(tvp);
-       tdnode = VP_TO_TMPFS_DIR(tdvp);
-       tmp = VFS_TO_TMPFS(tdvp->v_mount);
+       mount = fdvp->v_mount;
+       KASSERT(mount != NULL);
+       KASSERT(mount == tdvp->v_mount);
+       /* XXX How can we be sure this stays true?  (Not that you're
+        * likely to mount a tmpfs read-only...)  */
+       KASSERT((mount->mnt_flag & MNT_RDONLY) == 0);
+       tmpfs = VFS_TO_TMPFS(mount);
+       KASSERT(tmpfs != NULL);
 
-       if (fdvp == tvp) {
-               error = 0;
-               goto out_unlocked;
-       }
-
-       /* Allocate memory, if necessary, for a new name. */
-       namelen = tcnp->cn_namelen;
+       /*
+        * Decide whether we need a new name, and allocate memory for
+        * it if so.  Do this before locking anything or taking
+        * destructive actions so that we can back out safely and sleep
+        * safely.  XXX Is sleeping an issue here?  Can this just be
+        * moved into tmpfs_rename_attachdetach?
+        */
        if (tmpfs_strname_neqlen(fcnp, tcnp)) {
-               newname = tmpfs_strname_alloc(tmp, namelen);
+               newname = tmpfs_strname_alloc(tmpfs, tcnp->cn_namelen);
                if (newname == NULL) {
                        error = ENOSPC;
                        goto out_unlocked;
                }
+       } else {
+               newname = NULL;
        }
 
-       /* XXX: Lock order violation! */
-       if (fdvp != tdvp) {
-               vn_lock(fdvp, LK_EXCLUSIVE | LK_RETRY);
-       }
-       if (fvp != tvp) {
-               vn_lock(fvp, LK_EXCLUSIVE | LK_RETRY);
+       /*
+        * Lock and look up everything.  GCC is not very clever.
+        */
+       fde = tde = NULL;
+       fvp = tvp = NULL;
+       error = tmpfs_rename_enter(mount, tmpfs, cred,
+           fdvp, fdnode, fcnp, &fde, &fvp,
+           tdvp, tdnode, tcnp, &tde, &tvp);
+       if (error)