nfs_inactive() has a lock order reversal. When it removes the silly
file, it locks the directory vnode while it already holds the lock
of the argument file vnode. This clashes for example with name lookups
where directory vnodes are locked before file vnodes.

The reversal can cause a deadlock when an NFS client has multiple
processes that create, modify and remove files in the same
NFS directory.

The following patch makes the silly file removal happen after
nfs_inactive() has released the file vnode lock. This should be safe
because the silly file removal is independent of nfs_inactive()'s
argument vnode.

Could some NFS users test this?

Index: nfs/nfs_node.c
===================================================================
RCS file: src/sys/nfs/nfs_node.c,v
retrieving revision 1.74
diff -u -p -r1.74 nfs_node.c
--- nfs/nfs_node.c      20 Oct 2021 06:35:39 -0000      1.74
+++ nfs/nfs_node.c      16 Jun 2022 14:46:36 -0000
@@ -183,20 +183,23 @@ nfs_inactive(void *v)
                np->n_sillyrename = NULL;
        } else
                sp = NULL;
-       if (sp) {
+       if (sp != NULL)
+               nfs_vinvalbuf(ap->a_vp, 0, sp->s_cred, curproc);
+       np->n_flag &= (NMODIFIED | NFLUSHINPROG | NFLUSHWANT);
+
+       VOP_UNLOCK(ap->a_vp);
+
+       if (sp != NULL) {
                /*
                 * Remove the silly file that was rename'd earlier
                 */
-               nfs_vinvalbuf(ap->a_vp, 0, sp->s_cred, curproc);
                vn_lock(sp->s_dvp, LK_EXCLUSIVE | LK_RETRY);
                nfs_removeit(sp);
                crfree(sp->s_cred);
                vput(sp->s_dvp);
                free(sp, M_NFSREQ, sizeof(*sp));
        }
-       np->n_flag &= (NMODIFIED | NFLUSHINPROG | NFLUSHWANT);
 
-       VOP_UNLOCK(ap->a_vp);
        return (0);
 }
 

Reply via email to