Windows: Avoid deadlock on VolumeCB->VolumeLock

AFSPrimaryVolumeWorkerThread() holds VolumeCB->VolumeLock SHARED
across the call to AFSCleanupFcb() -> CcPurgeCacheSection().
If a filter driver such as Sophos (savonaccessfilter.sys ->
savonaccesscontrol.sys) triggers an AFSCreate() in response to
the cache section being purged that will force the evaluation of
the file path by AFSLocateNameEntry().  If the path contains a
mount point that requires validation, AFSBuildMountPointTarget()
is called which in turn required the VolumeCB->VolumeLock EXCL.

AFSBuildMountPointTarget() only requires the VolumeCB->VolumeLock
if the VolumeCB->RootFcb == NULL.  That should only be true if
the VolumeCB was allocated by AFSInitVolume() or under very rare
race conditions.

This patchset refactors AFSInitVolume() to ensure that it holds
an extra VolumeCB->VolumeReferenceCount reference.  This reference
is used to assist in the refactoring of AFSBuildRootVolume() and
AFSBuildMountPointTarget() to avoid races with volume root object
invalidation as well as permitting the VolumeCB->VolumeLock to
be ignored in the common case.

Avoiding the acquisition of VolumeCB->VolumeLock during mount
point target evaluation has the additional benefit of reducing
lock contention during path evaluation.

FIXES 130812

Change-Id: Id9b0dcc2bfd91277d522f3724893b60ce4d947f5
Reviewed-on: http://gerrit.openafs.org/7474
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Peter Scott <pscott@kerneldrivers.com>
Reviewed-by: Jeffrey Altman <jaltman@secure-endpoints.com>
Tested-by: Jeffrey Altman <jaltman@secure-endpoints.com>

diff --git a/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp b/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp
index 9f1a979..71f90c4 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp
+++ b/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp
@@ -430,8 +430,6 @@ AFSInitVolume( IN GUID *AuthGroup,
                 AFSAcquireExcl( pVolumeCB->VolumeLock,
                                 TRUE);
 
-                lCount = InterlockedDecrement( &pVolumeCB->VolumeReferenceCount);
-
                 *VolumeCB = pVolumeCB;
 
                 try_return( ntStatus);
@@ -526,10 +524,10 @@ AFSInitVolume( IN GUID *AuthGroup,
 
         AFSDbgLogMsg( AFS_SUBSYSTEM_VOLUME_REF_COUNTING,
                       AFS_TRACE_LEVEL_VERBOSE,
-                      "AFSInitVolume Initializing count (1) on volume %08lX\n",
+                      "AFSInitVolume Initializing count (2) on volume %08lX\n",
                       pVolumeCB);
 
-        pVolumeCB->VolumeReferenceCount = 1;
+        pVolumeCB->VolumeReferenceCount = 2;
 
         AFSAcquireExcl( pVolumeCB->VolumeLock,
                         TRUE);

diff --git a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp
index 0209a3e..a8804f6 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp
+++ b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp
@@ -7731,6 +7731,7 @@ AFSInitializeLibrary( IN AFSLibraryInitCB *LibraryInit)
     NTSTATUS ntStatus = STATUS_SUCCESS;
     AFSDeviceExt *pControlDevExt = NULL;
     ULONG ulTimeIncrement = 0;
+    LONG lCount;
 
     __Enter
     {
@@ -7817,6 +7818,8 @@ AFSInitializeLibrary( IN AFSLibraryInitCB *LibraryInit)
                           "AFSInitializeLibrary AFSInitRootFcb failure %08lX\n",
                           ntStatus);
 
+            lCount = InterlockedDecrement( &AFSGlobalRoot->VolumeReferenceCount);
+
             AFSReleaseResource( AFSGlobalRoot->VolumeLock);
 
             try_return( ntStatus);
@@ -7843,6 +7846,8 @@ AFSInitializeLibrary( IN AFSLibraryInitCB *LibraryInit)
 
         AFSInitVolumeWorker( AFSGlobalRoot);
 
+        lCount = InterlockedDecrement( &AFSGlobalRoot->VolumeReferenceCount);
+
         AFSReleaseResource( AFSGlobalRoot->VolumeLock);
 
         AFSReleaseResource( AFSGlobalRoot->ObjectInformation.Fcb->Header.Resource);

diff --git a/src/WINNT/afsrdr/kernel/lib/AFSNameSupport.cpp b/src/WINNT/afsrdr/kernel/lib/AFSNameSupport.cpp
index 743c3c5..43c0637 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/AFSNameSupport.cpp
+++ b/src/WINNT/afsrdr/kernel/lib/AFSNameSupport.cpp
@@ -4067,6 +4067,7 @@ AFSBuildMountPointTarget( IN GUID *AuthGroup,
     AFSVolumeCB *pVolumeCB = NULL;
     AFSFileID stTargetFileID;
     LONG lCount;
+    BOOLEAN bReleaseVolumeLock = FALSE;
 
     __Enter
     {
@@ -4221,40 +4222,46 @@ AFSBuildMountPointTarget( IN GUID *AuthGroup,
 
                 try_return( ntStatus);
             }
-        }
-        else
-        {
 
             //
-            // Check if this volume has been deleted or taken offline
+            // pVolumeCB->VolumeLock held exclusive and
+            // pVolumeCB->VolumeReferenceCount has been incremented
+            // pVolumeCB->RootFcb == NULL
             //
 
-            if( BooleanFlagOn( pVolumeCB->Flags, AFS_VOLUME_FLAGS_OFFLINE))
-            {
-
-                AFSReleaseResource( &pDevExt->Specific.RDR.VolumeTreeLock);
-
-                try_return( ntStatus = STATUS_FILE_IS_OFFLINE);
-            }
+            bReleaseVolumeLock = TRUE;
+        }
+        else
+        {
 
             //
-            // Just to ensure that things don't get torn down AND we don't create a
-            // deadlock with invalidation
+            // AFSInitVolume returns with a VolumeReferenceCount
+            // obtain one to match
             //
 
             lCount = InterlockedIncrement( &pVolumeCB->VolumeReferenceCount);
 
-            AFSReleaseResource( &pDevExt->Specific.RDR.VolumeTreeLock);
-
-            AFSAcquireExcl( pVolumeCB->VolumeLock,
-                            TRUE);
+            AFSDbgLogMsg( AFS_SUBSYSTEM_VOLUME_REF_COUNTING,
+                          AFS_TRACE_LEVEL_VERBOSE,
+                          "AFSBuildMountPointTarget Increment count on volume %08lX Cnt %d\n",
+                          pVolumeCB,
+                          lCount);
 
-            lCount = InterlockedDecrement( &pVolumeCB->VolumeReferenceCount);
+            AFSReleaseResource( &pDevExt->Specific.RDR.VolumeTreeLock);
         }
 
         if( pVolumeCB->RootFcb == NULL)
         {
 
+            if ( bReleaseVolumeLock == FALSE)
+            {
+
+                AFSAcquireExcl( pVolumeCB->VolumeLock,
+                                TRUE);
+
+                bReleaseVolumeLock = TRUE;
+            }
+
             //
             // Initialize the root fcb for this volume
             //
@@ -4265,6 +4272,8 @@ AFSBuildMountPointTarget( IN GUID *AuthGroup,
             if( !NT_SUCCESS( ntStatus))
             {
 
+                lCount = InterlockedDecrement( &pVolumeCB->VolumeReferenceCount);
+
                 AFSReleaseResource( pVolumeCB->VolumeLock);
 
                 try_return( ntStatus);
@@ -4277,6 +4286,12 @@ AFSBuildMountPointTarget( IN GUID *AuthGroup,
             AFSReleaseResource( &pVolumeCB->RootFcb->NPFcb->Resource);
         }
 
+        if ( bReleaseVolumeLock == TRUE)
+        {
+
+            AFSReleaseResource( pVolumeCB->VolumeLock);
+        }
+
         AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING,
                       AFS_TRACE_LEVEL_VERBOSE_2,
                       "AFSBuildMountPointTarget Evaluated target of %wZ FID %08lX-%08lX-%08lX-%08lX as root volume\n",
@@ -4286,16 +4301,6 @@ AFSBuildMountPointTarget( IN GUID *AuthGroup,
                       pVolumeCB->ObjectInformation.FileId.Vnode,
                       pVolumeCB->ObjectInformation.FileId.Unique);
 
-        lCount = InterlockedIncrement( &pVolumeCB->VolumeReferenceCount);
-
-        AFSReleaseResource( pVolumeCB->VolumeLock);
-
-        AFSDbgLogMsg( AFS_SUBSYSTEM_VOLUME_REF_COUNTING,
-                      AFS_TRACE_LEVEL_VERBOSE,
-                      "AFSBuildMountPointTarget Increment count on volume %08lX Cnt %d\n",
-                      pVolumeCB,
-                      lCount);
-
         *TargetVolumeCB = pVolumeCB;
 
 try_exit:
@@ -4323,6 +4328,7 @@ AFSBuildRootVolume( IN GUID *AuthGroup,
     ULONGLONG       ullIndex = 0;
     AFSVolumeCB *pVolumeCB = NULL;
     LONG lCount;
+    BOOLEAN bReleaseVolumeLock = FALSE;
 
     __Enter
     {
@@ -4390,29 +4396,47 @@ AFSBuildRootVolume( IN GUID *AuthGroup,
 
                 try_return( ntStatus);
             }
+
+            //
+            // pVolumeCB->VolumeLock is held exclusive
+            // pVolumeCB->VolumeReferenceCount has been incremented
+            // pVolumeCB->RootFcb == NULL
+            //
+
+            bReleaseVolumeLock = TRUE;
         }
         else
         {
 
             //
-            // Just to ensure that things don't get torn down AND we don't create a
-            // deadlock with invalidation
+            // AFSInitVolume returns with a VolumeReferenceCount
+            // obtain one to match
             //
 
             lCount = InterlockedIncrement( &pVolumeCB->VolumeReferenceCount);
 
-            AFSReleaseResource( &pDevExt->Specific.RDR.VolumeTreeLock);
-
-            AFSAcquireExcl( pVolumeCB->VolumeLock,
-                            TRUE);
+            AFSDbgLogMsg( AFS_SUBSYSTEM_VOLUME_REF_COUNTING,
+                          AFS_TRACE_LEVEL_VERBOSE,
+                          "AFSBuildRootVolume Increment count on volume %08lX Cnt %d\n",
+                          pVolumeCB,
+                          lCount);
 
-            lCount = InterlockedDecrement( &pVolumeCB->VolumeReferenceCount);
+            AFSReleaseResource( &pDevExt->Specific.RDR.VolumeTreeLock);
         }
 
 
         if( pVolumeCB->RootFcb == NULL)
         {
 
+            if ( bReleaseVolumeLock == FALSE)
+            {
+
+                AFSAcquireExcl( pVolumeCB->VolumeLock,
+                                TRUE);
+
+                bReleaseVolumeLock = TRUE;
+            }
+
             //
             // Initialize the root fcb for this volume
             //
@@ -4423,6 +4447,8 @@ AFSBuildRootVolume( IN GUID *AuthGroup,
             if( !NT_SUCCESS( ntStatus))
             {
 
+                lCount = InterlockedDecrement( &pVolumeCB->VolumeReferenceCount);
+
                 AFSReleaseResource( pVolumeCB->VolumeLock);
 
                 try_return( ntStatus);
@@ -4435,6 +4461,12 @@ AFSBuildRootVolume( IN GUID *AuthGroup,
             AFSReleaseResource( &pVolumeCB->RootFcb->NPFcb->Resource);
         }
 
+        if ( bReleaseVolumeLock == TRUE)
+        {
+
+            AFSReleaseResource( pVolumeCB->VolumeLock);
+        }
+
         AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING,
                       AFS_TRACE_LEVEL_VERBOSE_2,
                       "AFSBuildRootVolume Evaluated target of %wZ FID %08lX-%08lX-%08lX-%08lX as root volume\n",
@@ -4444,16 +4476,6 @@ AFSBuildRootVolume( IN GUID *AuthGroup,
                       pVolumeCB->ObjectInformation.FileId.Vnode,
                       pVolumeCB->ObjectInformation.FileId.Unique);
 
-        lCount = InterlockedIncrement( &pVolumeCB->VolumeReferenceCount);
-
-        AFSReleaseResource( pVolumeCB->VolumeLock);
-
-        AFSDbgLogMsg( AFS_SUBSYSTEM_VOLUME_REF_COUNTING,
-                      AFS_TRACE_LEVEL_VERBOSE,
-                      "AFSBuildRootVolume Increment count on volume %08lX Cnt %d\n",
-                      pVolumeCB,
-                      lCount);
-
         *TargetVolumeCB = pVolumeCB;
 
 try_exit: