afs: write-lock vcache->lock in afs_InactiveVCache 84/14584/3
authorMark Vitale <mvitale@sinenomine.net>
Sat, 27 Mar 2021 04:00:17 +0000 (00:00 -0400)
committerBenjamin Kaduk <kaduk@mit.edu>
Thu, 13 May 2021 16:54:28 +0000 (12:54 -0400)
Since the original IBM code import, the comments for
afs_InvalidateAllSegments indicate that vcache->lock W should be held at
entry.  However, even back then, only LINUX and IRIX honored this
requirement when the 'inactive' vnode operation reached
afs_InvalidateAllSegments.

Over the years, a number of commits have changed the operation and
locking for the LINUX inactive vnode op:

5293aa35617a6ad35980ce16fdf492ea960cc18a linux-iput-and-glock-changes-20010130
e8591334602e5e8dad78dc6426d3c44d564572c1 linux-osi-clear-inode-locking-fix-20010816
652f3bd9cb7a5d7833a760ba50ef7c2c67214bba linux-dynamic-inodes-20050710
e0d9e434bb778a2507c1cd6d96c1faa2071f2b2c put-inode-speedup-20050815
b21c13dc3ab751118220dc31276995050841a1ae linux-dentry-iput-20060813

Eventually, ac52e2f3c0bec9298d020de963036409165f380e
linux-dont-lock-around-inactivevcache-20061010 removed the vcache->lock
from afs_dentry_iput (the current OpenAFS handler for inactive vcaches).
The commit message states:

    "iafs_InactiveVCache() [sic] calls afs_InvalidateAllSegments() which says
    it should be called with the vnode locked. so the lock should
    probably be moved to afs_InactiveVCache() so it can be droppped
    before calling afs_remunlink()."

Unfortunately, the vcache->lock was never moved to afs_InactiveVCache.

Finally, 3be5880d1d2a0aef6600047ed43d602949cd5f4d 'afs: Avoid panics in
afs_InvalidateAllSegments' introduced a background operation
BInvalidateSegments that contains an assert for vcache->lock.  This
assert has exposed the existing lack of proper locking for some paths to
afs_InvalidateAllSegments by causing a kernel panic:

  d_iput -> afs_dentry_iput -> afs_InactiveVCache ->
  afs_InvalidateAllSegments -> afs_BQueue(BOP_INVALIDATE_SEGMENTS..) ->
  BInvalidateSegments -> osi_Assert(WriteLocked(&vcache->lock))

Prevent the panic by modifying afs_InactiveVCache to obtain vcache->lock
W before calling afs_InvalidateAllSegments, and dropping it before
calling afs_remunlink.

Thanks to Chaskiel Grundman for reporting and diagnosing the problem.

Change-Id: Ic309e4006bf47bcb38fa2b53bf103e0c645a856d
Reviewed-on: https://gerrit.openafs.org/14584
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>

src/afs/afs_vcache.c

index 465aa98..7a003d8 100644 (file)
@@ -312,6 +312,9 @@ void
 afs_InactiveVCache(struct vcache *avc, afs_ucred_t *acred)
 {
     AFS_STATCNT(afs_inactive);
+
+    ObtainWriteLock(&avc->lock, 68);
+
     if (avc->f.states & CDirty) {
        /* we can't keep trying to push back dirty data forever.  Give up. */
        afs_InvalidateAllSegments(avc); /* turns off dirty bit */
@@ -321,11 +324,14 @@ afs_InactiveVCache(struct vcache *avc, afs_ucred_t *acred)
     if (avc->f.states & CUnlinked) {
        if (CheckLock(&afs_xvcache) || CheckLock(&afs_xdcache)) {
            avc->f.states |= CUnlinkedDel;
+           ReleaseWriteLock(&avc->lock);
            return;
        }
+       ReleaseWriteLock(&avc->lock);
        afs_remunlink(avc, 1);  /* ignore any return code */
+       return;
     }
-
+    ReleaseWriteLock(&avc->lock);
 }
 #endif