tmpfs - Fix rare deadlock
authorMatthew Dillon <dillon@apollo.backplane.com>
Sat, 25 Aug 2018 17:10:08 +0000 (10:10 -0700)
committerMatthew Dillon <dillon@apollo.backplane.com>
Wed, 29 Aug 2018 15:27:26 +0000 (08:27 -0700)
* Fix a deadlock which can occur between umount and tmpfs, and
  possibly in other very rare situations.

* tmpfs holds the directory node locked when resolving a directory
  entry.  This results in a lock order reversal between the
  directory's tmpfs_node lock and the vnode being locked.

  Fixed by using a NOWAIT/UNLOCK/SLEEPFAIL/RETRY sequence.

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

index 14499a6..fd5aa46 100644 (file)
@@ -398,8 +398,8 @@ 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 **);
 void   tmpfs_free_dirent(struct tmpfs_mount *, struct tmpfs_dirent *);
-int    tmpfs_alloc_vp(struct mount *, struct tmpfs_node *, int,
-           struct vnode **);
+int    tmpfs_alloc_vp(struct mount *, struct tmpfs_node *,
+           struct tmpfs_node *, int, struct vnode **);
 void   tmpfs_free_vp(struct vnode *);
 int    tmpfs_alloc_file(struct vnode *, struct vnode **, struct vattr *,
            struct namecache *, struct ucred *, char *);
index e6a26c3..8a7a69d 100644 (file)
@@ -360,9 +360,18 @@ tmpfs_free_dirent(struct tmpfs_mount *tmp, struct tmpfs_dirent *de)
  * resulting locked vnode is returned in *vpp.
  *
  * Returns zero on success or an appropriate error code on failure.
+ *
+ * The caller must ensure that node cannot go away (usually by holding
+ * the related directory entry).
+ *
+ * If dnode is non-NULL this routine avoids deadlocking against it but
+ * can return EAGAIN.  Caller must try again.  The dnode lock will cycle
+ * in this case, it remains locked on return in all cases.  dnode must
+ * be shared-locked.
  */
 int
-tmpfs_alloc_vp(struct mount *mp, struct tmpfs_node *node, int lkflag,
+tmpfs_alloc_vp(struct mount *mp,
+              struct tmpfs_node *dnode, struct tmpfs_node *node, int lkflag,
               struct vnode **vpp)
 {
        int error = 0;
@@ -380,9 +389,33 @@ loop:
                vhold(vp);
                TMPFS_NODE_UNLOCK(node);
 
-               if (vget(vp, lkflag | LK_EXCLUSIVE) != 0) {
-                       vdrop(vp);
-                       goto loop;
+               if (dnode) {
+                       /*
+                        * Special-case handling to avoid deadlocking against
+                        * dnode.
+                        */
+                       if (vget(vp, (lkflag & ~LK_RETRY) |
+                                    LK_NOWAIT |
+                                    LK_EXCLUSIVE) != 0) {
+                               TMPFS_NODE_UNLOCK(dnode);
+                               if (vget(vp, (lkflag & ~LK_RETRY) |
+                                            LK_SLEEPFAIL |
+                                            LK_EXCLUSIVE) == 0) {
+                                       vn_unlock(vp);
+                               }
+                               vdrop(vp);
+                               TMPFS_NODE_LOCK_SH(dnode);
+                               kprintf("tmpfs: deadlock avoided\n");
+                               return EAGAIN;
+                       }
+               } else {
+                       /*
+                        * Normal path
+                        */
+                       if (vget(vp, lkflag | LK_EXCLUSIVE) != 0) {
+                               vdrop(vp);
+                               goto loop;
+                       }
                }
                if (node->tn_vnode != vp) {
                        vput(vp);
@@ -478,13 +511,7 @@ unlock:
 
 out:
        *vpp = vp;
-
        KKASSERT(IFF(error == 0, *vpp != NULL && vn_islocked(*vpp)));
-#ifdef INVARIANTS
-       TMPFS_NODE_LOCK(node);
-       KKASSERT(*vpp == node->tn_vnode);
-       TMPFS_NODE_UNLOCK(node);
-#endif
 
        return error;
 }
@@ -567,7 +594,7 @@ tmpfs_alloc_file(struct vnode *dvp, struct vnode **vpp, struct vattr *vap,
        }
 
        /* Allocate a vnode for the new file. */
-       error = tmpfs_alloc_vp(dvp->v_mount, node, LK_EXCLUSIVE, vpp);
+       error = tmpfs_alloc_vp(dvp->v_mount, NULL, node, LK_EXCLUSIVE, vpp);
        if (error != 0) {
                tmpfs_free_dirent(tmp, de);
                tmpfs_free_node(tmp, node);
index 7ee682c..d3adcf6 100644 (file)
@@ -467,7 +467,8 @@ tmpfs_root(struct mount *mp, struct vnode **vpp)
                *vpp = NULL;
                error = EINVAL;
        } else {
-               error = tmpfs_alloc_vp(mp, tmp->tm_root, LK_EXCLUSIVE, vpp);
+               error = tmpfs_alloc_vp(mp, NULL, tmp->tm_root,
+                                      LK_EXCLUSIVE, vpp);
                (*vpp)->v_flag |= VROOT;
                (*vpp)->v_type = VDIR;
        }
@@ -505,7 +506,7 @@ tmpfs_fhtovp(struct mount *mp, struct vnode *rootvp, struct fid *fhp,
        }
 
        if (found)
-               rc = tmpfs_alloc_vp(mp, node, LK_EXCLUSIVE, vpp);
+               rc = tmpfs_alloc_vp(mp, NULL, node, LK_EXCLUSIVE, vpp);
 
        TMPFS_UNLOCK(tmp);
 
index ffcfc85..01452f1 100644 (file)
@@ -99,16 +99,20 @@ tmpfs_nresolve(struct vop_nresolve_args *ap)
        dnode = VP_TO_TMPFS_DIR(dvp);
 
        TMPFS_NODE_LOCK_SH(dnode);
+loop:
        de = tmpfs_dir_lookup(dnode, NULL, ncp);
        if (de == NULL) {
                error = ENOENT;
        } else {
                /*
-                * Allocate a vnode for the node we found.
+                * Allocate a vnode for the node we found.  Use
+                * tmpfs_alloc_vp()'s deadlock handling mode.
                 */
                tnode = de->td_node;
-               error = tmpfs_alloc_vp(dvp->v_mount, tnode,
+               error = tmpfs_alloc_vp(dvp->v_mount, dnode, tnode,
                                       LK_EXCLUSIVE | LK_RETRY, &vp);
+               if (error == EAGAIN)
+                       goto loop;
                if (error)
                        goto out;
                KKASSERT(vp);
@@ -156,7 +160,8 @@ tmpfs_nlookupdotdot(struct vop_nlookupdotdot_args *ap)
 
        if (dnode->tn_dir.tn_parent != NULL) {
                /* Allocate a new vnode on the matching entry. */
-               error = tmpfs_alloc_vp(dvp->v_mount, dnode->tn_dir.tn_parent,
+               error = tmpfs_alloc_vp(dvp->v_mount,
+                                      NULL, dnode->tn_dir.tn_parent,
                                       LK_EXCLUSIVE | LK_RETRY, vpp);
 
                if (*vpp)