tmpfs - Fix numerous races and adjust to use cache_unlink()
authorMatthew Dillon <dillon@apollo.backplane.com>
Fri, 24 Aug 2012 23:30:20 +0000 (16:30 -0700)
committerMatthew Dillon <dillon@apollo.backplane.com>
Fri, 24 Aug 2012 23:30:20 +0000 (16:30 -0700)
* Fix numerous issues when chdir'd into a directory that is then
  rmdir'd.

* Rewrite the link count handling code for directories, also removing
  two now unecessary recursions.

* Do not allow new files to be created in a directory which has been
  rmdir'd.

* Use the new cache_unlink facility.

sys/vfs/tmpfs/tmpfs.h
sys/vfs/tmpfs/tmpfs_subr.c
sys/vfs/tmpfs/tmpfs_vfsops.c
sys/vfs/tmpfs/tmpfs_vnops.c

index 2e20923..c53d7c1 100644 (file)
@@ -433,8 +433,8 @@ struct tmpfs_fid {
  */
 
 int    tmpfs_alloc_node(struct tmpfs_mount *, enum vtype,
-           uid_t uid, gid_t gid, mode_t mode, struct tmpfs_node *,
-           char *, int, int, struct tmpfs_node **);
+           uid_t uid, gid_t gid, mode_t mode, char *, int, int,
+           struct tmpfs_node **);
 void   tmpfs_free_node(struct tmpfs_mount *, struct tmpfs_node *);
 int    tmpfs_alloc_dirent(struct tmpfs_mount *, struct tmpfs_node *,
            const char *, uint16_t, struct tmpfs_dirent **);
index 0913eb9..cd18ca2 100644 (file)
@@ -87,17 +87,14 @@ RB_GENERATE(tmpfs_dirtree, tmpfs_dirent, rb_node, tmpfs_dirtree_compare);
  */
 int
 tmpfs_alloc_node(struct tmpfs_mount *tmp, enum vtype type,
-    uid_t uid, gid_t gid, mode_t mode, struct tmpfs_node *parent,
-    char *target, int rmajor, int rminor, struct tmpfs_node **node)
+                uid_t uid, gid_t gid, mode_t mode,
+                char *target, int rmajor, int rminor,
+                struct tmpfs_node **node)
 {
        struct tmpfs_node *nnode;
        struct timespec ts;
        udev_t rdev;
 
-       /* If the root directory of the 'tmp' file system is not yet
-        * allocated, this must be the request to do it. */
-       KKASSERT(IMPLIES(tmp->tm_root == NULL, parent == NULL && type == VDIR));
-
        KKASSERT(IFF(type == VLNK, target != NULL));
        KKASSERT(IFF(type == VBLK || type == VCHR, rmajor != VNOVAL));
 
@@ -120,6 +117,7 @@ tmpfs_alloc_node(struct tmpfs_mount *tmp, enum vtype type,
        nnode->tn_mode = mode;
        nnode->tn_id = tmpfs_fetch_ino(tmp);
        nnode->tn_advlock.init_done = 0;
+       KKASSERT(nnode->tn_links == 0);
 
        /* Type-specific initialization. */
        switch (nnode->tn_type) {
@@ -135,18 +133,9 @@ tmpfs_alloc_node(struct tmpfs_mount *tmp, enum vtype type,
 
        case VDIR:
                RB_INIT(&nnode->tn_dir.tn_dirtree);
-               KKASSERT(parent != nnode);
-               KKASSERT(IMPLIES(parent == NULL, tmp->tm_root == NULL));
-               nnode->tn_dir.tn_parent = parent;
                nnode->tn_dir.tn_readdir_lastn = 0;
                nnode->tn_dir.tn_readdir_lastp = NULL;
-               nnode->tn_links++;
                nnode->tn_size = 0;
-               if (parent) {
-                       TMPFS_NODE_LOCK(parent);
-                       parent->tn_links++;
-                       TMPFS_NODE_UNLOCK(parent);
-               }
                break;
 
        case VFIFO:
@@ -238,29 +227,13 @@ tmpfs_free_node(struct tmpfs_mount *tmp, struct tmpfs_node *node)
        case VDIR:
                /*
                 * The parent link can be NULL if this is the root
-                * node.
+                * node or if it is a directory node that was rmdir'd.
+                *
+                * XXX what if node is a directory which still contains
+                * directory entries (e.g. due to a forced umount) ?
                 */
-               node->tn_links--;
                node->tn_size = 0;
-               KKASSERT(node->tn_dir.tn_parent || node == tmp->tm_root);
-               if (node->tn_dir.tn_parent) {
-                       TMPFS_NODE_LOCK(node->tn_dir.tn_parent);
-                       node->tn_dir.tn_parent->tn_links--;
-
-                       /*
-                        * If the parent directory has no more links and
-                        * no vnode ref nothing is going to come along
-                        * and clean it up unless we do it here.
-                        */
-                       if (node->tn_dir.tn_parent->tn_links == 0 &&
-                           node->tn_dir.tn_parent->tn_vnode == NULL) {
-                               tmpfs_free_node(tmp, node->tn_dir.tn_parent);
-                               /* eats parent lock */
-                       } else {
-                               TMPFS_NODE_UNLOCK(node->tn_dir.tn_parent);
-                       }
-                       node->tn_dir.tn_parent = NULL;
-               }
+               KKASSERT(node->tn_dir.tn_parent == NULL);
 
                /*
                 * If the root node is being destroyed don't leave a
@@ -548,31 +521,30 @@ tmpfs_alloc_file(struct vnode *dvp, struct vnode **vpp, struct vattr *vap,
        struct tmpfs_mount *tmp;
        struct tmpfs_node *dnode;
        struct tmpfs_node *node;
-       struct tmpfs_node *parent;
 
        tmp = VFS_TO_TMPFS(dvp->v_mount);
        dnode = VP_TO_TMPFS_DIR(dvp);
        *vpp = NULL;
 
-       /* If the entry we are creating is a directory, we cannot overflow
-        * the number of links of its parent, because it will get a new
-        * link. */
-       if (vap->va_type == VDIR) {
-               /* Ensure that we do not overflow the maximum number of links
-                * imposed by the system. */
-               KKASSERT(dnode->tn_links <= LINK_MAX);
-               if (dnode->tn_links == LINK_MAX) {
-                       return EMLINK;
-               }
+       /*
+        * If the directory was removed but a process was CD'd into it,
+        * we do not allow any more file/dir creation within it.  Otherwise
+        * we will lose track of it.
+        */
+       KKASSERT(dnode->tn_type == VDIR);
+       if (dnode != tmp->tm_root && dnode->tn_dir.tn_parent == NULL)
+               return ENOENT;
 
-               parent = dnode;
-               KKASSERT(parent != NULL);
-       } else
-               parent = NULL;
+       /*
+        * Make sure the link count does not overflow.
+        */
+       if (vap->va_type == VDIR && dnode->tn_links >= LINK_MAX)
+               return EMLINK;
 
        /* Allocate a node that represents the new file. */
        error = tmpfs_alloc_node(tmp, vap->va_type, cred->cr_uid,
-           dnode->tn_gid, vap->va_mode, parent, target, vap->va_rmajor, vap->va_rminor, &node);
+                                dnode->tn_gid, vap->va_mode, target,
+                                vap->va_rmajor, vap->va_rminor, &node);
        if (error != 0)
                return error;
        TMPFS_NODE_LOCK(node);
@@ -594,9 +566,11 @@ tmpfs_alloc_file(struct vnode *dvp, struct vnode **vpp, struct vattr *vap,
                return error;
        }
 
-       /* Now that all required items are allocated, we can proceed to
+       /*
+        * Now that all required items are allocated, we can proceed to
         * insert the new node into the directory, an operation that
-        * cannot fail. */
+        * cannot fail.
+        */
        tmpfs_dir_attach(dnode, de);
        TMPFS_NODE_UNLOCK(node);
 
@@ -613,10 +587,18 @@ tmpfs_alloc_file(struct vnode *dvp, struct vnode **vpp, struct vattr *vap,
 void
 tmpfs_dir_attach(struct tmpfs_node *dnode, struct tmpfs_dirent *de)
 {
+       struct tmpfs_node *node = de->td_node;
+
        TMPFS_NODE_LOCK(dnode);
+       if (node && node->tn_type == VDIR) {
+               TMPFS_NODE_LOCK(node);
+               ++node->tn_links;
+               node->tn_status |= TMPFS_NODE_CHANGED;
+               node->tn_dir.tn_parent = dnode;
+               ++dnode->tn_links;
+               TMPFS_NODE_UNLOCK(node);
+       }
        RB_INSERT(tmpfs_dirtree, &dnode->tn_dir.tn_dirtree, de);
-
-       TMPFS_ASSERT_ELOCKED(dnode);
        dnode->tn_size += sizeof(struct tmpfs_dirent);
        dnode->tn_status |= TMPFS_NODE_ACCESSED | TMPFS_NODE_CHANGED |
                            TMPFS_NODE_MODIFIED;
@@ -633,18 +615,40 @@ tmpfs_dir_attach(struct tmpfs_node *dnode, struct tmpfs_dirent *de)
 void
 tmpfs_dir_detach(struct tmpfs_node *dnode, struct tmpfs_dirent *de)
 {
+       struct tmpfs_node *node = de->td_node;
+
        TMPFS_NODE_LOCK(dnode);
        if (dnode->tn_dir.tn_readdir_lastp == de) {
                dnode->tn_dir.tn_readdir_lastn = 0;
                dnode->tn_dir.tn_readdir_lastp = NULL;
        }
        RB_REMOVE(tmpfs_dirtree, &dnode->tn_dir.tn_dirtree, de);
-
-       TMPFS_ASSERT_ELOCKED(dnode);
        dnode->tn_size -= sizeof(struct tmpfs_dirent);
        dnode->tn_status |= TMPFS_NODE_ACCESSED | TMPFS_NODE_CHANGED |
                            TMPFS_NODE_MODIFIED;
        TMPFS_NODE_UNLOCK(dnode);
+
+       /*
+        * Clean out the tn_parent pointer immediately when removing a
+        * directory.
+        *
+        * Removal of the parent linkage also cleans out the extra tn_links
+        * count we had on both node and dnode.
+        *
+        * node can be NULL (typ during a forced umount), in which case
+        * the mount code is dealing with the linkages from a linked list
+        * scan.
+        */
+       if (node && node->tn_type == VDIR && node->tn_dir.tn_parent) {
+               TMPFS_NODE_LOCK(dnode);
+               TMPFS_NODE_LOCK(node);
+               KKASSERT(node->tn_dir.tn_parent == dnode);
+               dnode->tn_links--;
+               node->tn_links--;
+               node->tn_dir.tn_parent = NULL;
+               TMPFS_NODE_UNLOCK(node);
+               TMPFS_NODE_UNLOCK(dnode);
+       }
 }
 
 /* --------------------------------------------------------------------- */
@@ -659,7 +663,7 @@ tmpfs_dir_detach(struct tmpfs_node *dnode, struct tmpfs_dirent *de)
  */
 struct tmpfs_dirent *
 tmpfs_dir_lookup(struct tmpfs_node *node, struct tmpfs_node *f,
-    struct namecache *ncp)
+                struct namecache *ncp)
 {
        struct tmpfs_dirent *de;
        int len = ncp->nc_nlen;
index de23cb7..0f67881 100644 (file)
@@ -255,7 +255,7 @@ tmpfs_mount(struct mount *mp, char *path, caddr_t data, struct ucred *cred)
 
        /* Allocate the root node. */
        error = tmpfs_alloc_node(tmp, VDIR, root_uid, root_gid,
-                                root_mode & ALLPERMS, NULL, NULL,
+                                root_mode & ALLPERMS, NULL,
                                 VNOVAL, VNOVAL, &root);
 
        /*
@@ -271,6 +271,7 @@ tmpfs_mount(struct mount *mp, char *path, caddr_t data, struct ucred *cred)
            return error;
        }
        KASSERT(root->tn_id >= 0, ("tmpfs root with invalid ino: %d", (int)root->tn_id));
+       ++root->tn_links;       /* prevent destruction */
        tmp->tm_root = root;
 
        mp->mnt_flag |= MNT_LOCAL;
@@ -308,7 +309,6 @@ tmpfs_unmount(struct mount *mp, int mntflags)
 {
        int error;
        int flags = 0;
-       int found;
        struct tmpfs_mount *tmp;
        struct tmpfs_node *node;
 
@@ -340,8 +340,9 @@ tmpfs_unmount(struct mount *mp, int mntflags)
 
        /*
         * First pass get rid of all the directory entries and
-        * vnode associations.  The directory structure will
-        * remain via the extra link count representing tn_dir.tn_parent.
+        * vnode associations.  This will also destroy the
+        * directory topology and should drop all link counts
+        * to 0 except for the root.
         *
         * No vnodes should remain after the vflush above.
         */
@@ -351,11 +352,9 @@ tmpfs_unmount(struct mount *mp, int mntflags)
                if (node->tn_type == VDIR) {
                        struct tmpfs_dirent *de;
 
-                       while (!RB_EMPTY(&node->tn_dir.tn_dirtree)) {
-                               de = RB_FIRST(tmpfs_dirtree, &node->tn_dir.tn_dirtree);
+                       while ((de = RB_ROOT(&node->tn_dir.tn_dirtree)) != NULL) {
                                tmpfs_dir_detach(node, de);
                                tmpfs_free_dirent(tmp, de);
-                               node->tn_size -= sizeof(struct tmpfs_dirent);
                        }
                }
                KKASSERT(node->tn_vnode == NULL);
@@ -372,34 +371,23 @@ tmpfs_unmount(struct mount *mp, int mntflags)
        }
 
        /*
-        * Now get rid of all nodes.  We can remove any node with a
-        * link count of 0 or any directory node with a link count of
-        * 1.  The parents will not be destroyed until all their children
-        * have been destroyed.
-        *
-        * Recursion in tmpfs_free_node() can further modify the list so
-        * we cannot use a next pointer here.
-        *
-        * The root node will be destroyed by this loop (it will be last).
+        * Allow the root node to be destroyed by dropping the link count
+        * we bumped in the mount code.
         */
-       while (!LIST_EMPTY(&tmp->tm_nodes_used)) {
-               found = 0;
-               LIST_FOREACH(node, &tmp->tm_nodes_used, tn_entries) {
-                       if (node->tn_links == 0 ||
-                           (node->tn_links == 1 && node->tn_type == VDIR)) {
-                               TMPFS_NODE_LOCK(node);
-                               tmpfs_free_node(tmp, node);
-                               /* eats lock */
-                               found = 1;
-                               break;
-                       }
-               }
-               if (found == 0) {
-                       kprintf("tmpfs: Cannot free entire node tree!");
-                       break;
-               }
-       }
+       KKASSERT(tmp->tm_root);
+       --tmp->tm_root->tn_links;
 
+       /*
+        * At this point all nodes, including the root node, should have a
+        * link count of 0.  The root is not necessarily going to be last.
+        */
+       while ((node = LIST_FIRST(&tmp->tm_nodes_used)) != NULL) {
+               if (node->tn_links)
+                       panic("tmpfs: Dangling nodes during umount (%p)!\n", node);
+               TMPFS_NODE_LOCK(node);
+               tmpfs_free_node(tmp, node);
+               /* eats lock */
+       }
        KKASSERT(tmp->tm_root == NULL);
 
        objcache_destroy(tmp->tm_dirent_pool);
index 70cb7cf..febc692 100644 (file)
@@ -211,11 +211,13 @@ tmpfs_open(struct vop_open_args *v)
 
        node = VP_TO_TMPFS_NODE(vp);
 
+#if 0
        /* The file is still active but all its names have been removed
         * (e.g. by a "rmdir $(pwd)").  It cannot be opened any more as
         * it is about to die. */
        if (node->tn_links < 1)
                return (ENOENT);
+#endif
 
        /* If the file is marked append-only, deny write requests. */
        if ((node->tn_flags & APPEND) &&
@@ -238,8 +240,10 @@ tmpfs_close(struct vop_close_args *v)
        node = VP_TO_TMPFS_NODE(vp);
 
        if (node->tn_links > 0) {
-               /* Update node times.  No need to do it if the node has
-                * been deleted, because it will vanish after we return. */
+               /*
+                * Update node times.  No need to do it if the node has
+                * been deleted, because it will vanish after we return.
+                */
                tmpfs_update(vp);
        }
 
@@ -828,10 +832,8 @@ tmpfs_nremove(struct vop_nremove_args *v)
                TMPFS_NODE_UNLOCK(node);
        }
 
-       cache_setunresolved(v->a_nch);
-       cache_setvp(v->a_nch, NULL);
+       cache_unlink(v->a_nch);
        tmpfs_knote(vp, NOTE_DELETE);
-       /*cache_inval_vp(vp, CINV_DESTROY);*/
        tmpfs_knote(dvp, NOTE_WRITE);
        error = 0;
 
@@ -1037,9 +1039,9 @@ tmpfs_nrename(struct vop_nrename_args *v)
         * already checked for illegal recursion cases (renaming a directory
         * into a subdirectory of itself).
         */
-       if (fdnode != tdnode)
+       if (fdnode != tdnode) {
                tmpfs_dir_detach(fdnode, de);
-       else {
+       else {
                RB_REMOVE(tmpfs_dirtree, &fdnode->tn_dir.tn_dirtree, de);
        }
 
@@ -1086,21 +1088,6 @@ tmpfs_nrename(struct vop_nrename_args *v)
        if (fdnode != tdnode) {
                if (de->td_node->tn_type == VDIR) {
                        TMPFS_VALIDATE_DIR(fnode);
-
-                       TMPFS_NODE_LOCK(tdnode);
-                       tdnode->tn_links++;
-                       tdnode->tn_status |= TMPFS_NODE_MODIFIED;
-                       TMPFS_NODE_UNLOCK(tdnode);
-
-                       TMPFS_NODE_LOCK(fnode);
-                       fnode->tn_dir.tn_parent = tdnode;
-                       fnode->tn_status |= TMPFS_NODE_CHANGED;
-                       TMPFS_NODE_UNLOCK(fnode);
-
-                       TMPFS_NODE_LOCK(fdnode);
-                       fdnode->tn_links--;
-                       fdnode->tn_status |= TMPFS_NODE_MODIFIED;
-                       TMPFS_NODE_UNLOCK(fdnode);
                }
                tmpfs_dir_attach(tdnode, de);
        } else {
@@ -1238,20 +1225,13 @@ tmpfs_nrmdir(struct vop_nrmdir_args *v)
        TMPFS_NODE_LOCK(dnode);
        TMPFS_ASSERT_ELOCKED(dnode);
 
-#if 0
-       /* handled by tmpfs_free_node */
-       KKASSERT(node->tn_links > 0);
-       node->tn_links--;
-       node->tn_dir.tn_parent = NULL;
-#endif
+       /*
+        * Must set parent linkage to NULL (tested by ncreate to disallow
+        * the creation of new files/dirs in a deleted directory)
+        */
        node->tn_status |= TMPFS_NODE_ACCESSED | TMPFS_NODE_CHANGED | \
            TMPFS_NODE_MODIFIED;
 
-#if 0
-       /* handled by tmpfs_free_node */
-       KKASSERT(dnode->tn_links > 0);
-       dnode->tn_links--;
-#endif
        dnode->tn_status |= TMPFS_NODE_ACCESSED | \
            TMPFS_NODE_CHANGED | TMPFS_NODE_MODIFIED;
 
@@ -1271,9 +1251,7 @@ tmpfs_nrmdir(struct vop_nrmdir_args *v)
        TMPFS_NODE_UNLOCK(dnode);
        tmpfs_update(dvp);
 
-       cache_setunresolved(v->a_nch);
-       cache_setvp(v->a_nch, NULL);
-       /*cache_inval_vp(vp, CINV_DESTROY);*/
+       cache_unlink(v->a_nch);
        tmpfs_knote(dvp, NOTE_WRITE | NOTE_LINK);
        error = 0;
 
@@ -1447,9 +1425,7 @@ tmpfs_inactive(struct vop_inactive_args *v)
         */
        TMPFS_NODE_LOCK(node);
        if ((node->tn_vpstate & TMPFS_VNODE_ALLOCATING) == 0 &&
-           (node->tn_links == 0 ||
-            (node->tn_links == 1 && node->tn_type == VDIR &&
-             node->tn_dir.tn_parent)))
+           node->tn_links == 0)
        {
                node->tn_vpstate = TMPFS_VNODE_DOOMED;
                TMPFS_NODE_UNLOCK(node);
@@ -1486,10 +1462,7 @@ tmpfs_reclaim(struct vop_reclaim_args *v)
         */
        TMPFS_NODE_LOCK(node);
        if ((node->tn_vpstate & TMPFS_VNODE_ALLOCATING) == 0 &&
-           (node->tn_links == 0 ||
-            (node->tn_links == 1 && node->tn_type == VDIR &&
-             node->tn_dir.tn_parent)))
-       {
+           node->tn_links == 0) {
                node->tn_vpstate = TMPFS_VNODE_DOOMED;
                tmpfs_free_node(tmp, node);
                /* eats the lock */