From: Andrew Deason <adeason@sinenomine.net>
Date: Fri, 12 Jan 2018 03:27:28 +0000 (-0600)
Subject: LINUX: Avoid locking inode in check_dentry_race
X-Git-Tag: openafs-devel-1_9_0~632
X-Git-Url: http://git.openafs.org/?p=openafs.git;a=commitdiff_plain;h=ef1d4c8d328e9b9affc9864fd084257e9fa08445

LINUX: Avoid locking inode in check_dentry_race

Currently, check_dentry_race locks the parent inode in order to ensure
it is not running in parallel with d_splice_alias for the same inode.
(For old Linux kernel versions; see commit b0461f2d: "LINUX:
Workaround d_splice_alias/d_lookup race".)

However, it is possible to hit this area of code when the parent inode
is already locked. When someone tries to create a file, directory, or
symlink, Linux tries to lookup the dentry for the target path, to see
if it already exists. While looking up the last component of the path,
Linux locks the directory, and if it finds a dentry for the target
name, it calls d_invalidate on it while the parent directory is
locked.

For a dentry with a NULL inode, we'll then try to lock the parent
inode in check_dentry_race. But since the inode is already locked, we
will deadlock.

From a user's point of view, the hang can be reproduced by doing
something similar to:

    $ mkdir dir # succeeds
    $ rmdir dir
    $ ls -l dir
    ls: cannot access dir: No such file or directory
    $ mkdir dir # hangs

To avoid this, we can just change which lock we're using to avoid
check_dentry_race/d_splice_alias from running in parallel. Instead of
locking the parent inode, introduce a new global lock (called
dentry_race_sem), and lock that in check_dentry_race and around our
d_splice_alias call. We know that those are the only two users of this
new lock, so this should avoid any such deadlocks.

This does potentially reduce performance, since all tasks that hit
check_dentry_race or d_splice_alias will take the same global lock.
However, this at least still allows us to make use of negative
dentries, and this entire code path only applies to older Linux
kernels. It could be possible to add a new lock into struct vcache
instead, but using a global lock like this commit does is much
simpler.

Change-Id: Ide0f21145c83d6fbb34c637d8a36c8cd21549940
Reviewed-on: https://gerrit.openafs.org/12868
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
---

diff --git a/src/afs/LINUX/osi_vnodeops.c b/src/afs/LINUX/osi_vnodeops.c
index d8dfd93..c1acca9 100644
--- a/src/afs/LINUX/osi_vnodeops.c
+++ b/src/afs/LINUX/osi_vnodeops.c
@@ -1125,7 +1125,30 @@ parent_vcache_dv(struct inode *inode, cred_t *credp)
     return hgetlo(pvcp->f.m.DataVersion);
 }
 
-#ifdef D_SPLICE_ALIAS_RACE
+#ifndef D_SPLICE_ALIAS_RACE
+
+static inline void dentry_race_lock(void) {}
+static inline void dentry_race_unlock(void) {}
+
+#else
+
+# if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,16)
+static DEFINE_MUTEX(dentry_race_sem);
+# else
+static DECLARE_MUTEX(dentry_race_sem);
+# endif
+
+static inline void
+dentry_race_lock(void)
+{
+    mutex_lock(&dentry_race_sem);
+}
+static inline void
+dentry_race_unlock(void)
+{
+    mutex_unlock(&dentry_race_sem);
+}
+
 /* Leave some trace that this code is enabled; otherwise it's pretty hard to
  * tell. */
 static __attribute__((used)) const char dentry_race_marker[] = "d_splice_alias race workaround enabled";
@@ -1135,8 +1158,6 @@ check_dentry_race(struct dentry *dp)
 {
     int raced = 0;
     if (!dp->d_inode) {
-        struct dentry *parent = dget_parent(dp);
-
         /* In Linux, before commit 4919c5e45a91b5db5a41695fe0357fbdff0d5767,
          * d_splice_alias can momentarily hash a dentry before it's fully
          * populated. This only happens for a moment, since it's unhashed again
@@ -1144,20 +1165,17 @@ check_dentry_race(struct dentry *dp)
          * __d_lookup, and then given to us.
          *
          * So check if the dentry is unhashed; if it is, then the dentry is not
-         * valid. We lock the parent inode to ensure that d_splice_alias is no
-         * longer running (the inode mutex will be held during
-         * afs_linux_lookup). Locking d_lock is required to check the dentry's
+         * valid. We lock dentry_race_lock() to ensure that d_splice_alias is
+         * no longer running. Locking d_lock is required to check the dentry's
          * flags, so lock that, too.
          */
-        afs_linux_lock_inode(parent->d_inode);
+        dentry_race_lock();
         spin_lock(&dp->d_lock);
         if (d_unhashed(dp)) {
             raced = 1;
         }
         spin_unlock(&dp->d_lock);
-        afs_linux_unlock_inode(parent->d_inode);
-
-        dput(parent);
+        dentry_race_unlock();
     }
     return raced;
 }
@@ -1636,7 +1654,9 @@ afs_linux_lookup(struct inode *dip, struct dentry *dp)
 	igrab(ip);
 #endif
 
+    dentry_race_lock();
     newdp = d_splice_alias(ip, dp);
+    dentry_race_unlock();
 
  done:
     crfree(credp);