From 6e0c5aabac8d345557e78062b61bd3c1ed432332 Mon Sep 17 00:00:00 2001 From: Matthew Dillon Date: Fri, 24 Aug 2012 16:30:20 -0700 Subject: [PATCH] tmpfs - Fix numerous races and adjust to use cache_unlink() * 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 | 4 +- sys/vfs/tmpfs/tmpfs_subr.c | 122 ++++++++++++++++++----------------- sys/vfs/tmpfs/tmpfs_vfsops.c | 54 ++++++---------- sys/vfs/tmpfs/tmpfs_vnops.c | 59 +++++------------ 4 files changed, 102 insertions(+), 137 deletions(-) diff --git a/sys/vfs/tmpfs/tmpfs.h b/sys/vfs/tmpfs/tmpfs.h index 2e209238ab..c53d7c1ad4 100644 --- a/sys/vfs/tmpfs/tmpfs.h +++ b/sys/vfs/tmpfs/tmpfs.h @@ -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 **); diff --git a/sys/vfs/tmpfs/tmpfs_subr.c b/sys/vfs/tmpfs/tmpfs_subr.c index 0913eb9a78..cd18ca2497 100644 --- a/sys/vfs/tmpfs/tmpfs_subr.c +++ b/sys/vfs/tmpfs/tmpfs_subr.c @@ -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; diff --git a/sys/vfs/tmpfs/tmpfs_vfsops.c b/sys/vfs/tmpfs/tmpfs_vfsops.c index de23cb7d3b..0f67881041 100644 --- a/sys/vfs/tmpfs/tmpfs_vfsops.c +++ b/sys/vfs/tmpfs/tmpfs_vfsops.c @@ -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); diff --git a/sys/vfs/tmpfs/tmpfs_vnops.c b/sys/vfs/tmpfs/tmpfs_vnops.c index 70cb7cfc2f..febc692cac 100644 --- a/sys/vfs/tmpfs/tmpfs_vnops.c +++ b/sys/vfs/tmpfs/tmpfs_vnops.c @@ -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 */ -- 2.41.0