Windows: AFSRemoveFcb() cannot race

Modify AFSRemoveFcb to use InterlockedComparePointerExchange
to ensure that only one thread can remove and deallocate an
AFSFcb structure.

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

diff --git a/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp b/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp
index 63fae848dbd2ddcd295c7f55c4d35f1a606d692d..7777da9eaa6874e65ee17f2fc7949aca073bf462 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp
+++ b/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp
@@ -2198,9 +2198,7 @@ try_exit:
             if( bAllocatedFcb)
             {
 
-                AFSRemoveFcb( pObjectInfo->Fcb);
-
-                pObjectInfo->Fcb = NULL;
+                AFSRemoveFcb( &pObjectInfo->Fcb);
             }
 
             *Fcb = NULL;
@@ -2492,9 +2490,7 @@ try_exit:
             if( bAllocatedFcb)
             {
 
-                AFSRemoveFcb( pParentObject->Fcb);
-
-                pParentObject->Fcb = NULL;
+                AFSRemoveFcb( &pParentObject->Fcb);
             }
 
             *Fcb = NULL;
@@ -3013,9 +3009,7 @@ try_exit:
             if( bAllocatedFcb)
             {
 
-                AFSRemoveFcb( pObjectInfo->Fcb);
-
-                pObjectInfo->Fcb = NULL;
+                AFSRemoveFcb( &pObjectInfo->Fcb);
             }
 
             *Fcb = NULL;
@@ -3435,9 +3429,7 @@ try_exit:
             if( bAllocatedFcb)
             {
 
-                AFSRemoveFcb( pObjectInfo->Fcb);
-
-                pObjectInfo->Fcb = NULL;
+                AFSRemoveFcb( &pObjectInfo->Fcb);
             }
 
             *Fcb = NULL;
@@ -3737,10 +3729,10 @@ try_exit:
 
                 AFSRemoveCcb( NULL,
                               *Ccb);
-
-                *Ccb = NULL;
             }
 
+            *Ccb = NULL;
+
             if( bAllocatedFcb)
             {
 
@@ -3748,14 +3740,10 @@ try_exit:
                 // Need to tear down this Fcb since it is not in the tree for the worker thread
                 //
 
-                AFSRemoveFcb( *Fcb);
-
-                pParentObjectInfo->Specific.Directory.PIOCtlDirectoryCB->ObjectInformation->Fcb = NULL;
+                AFSRemoveFcb( &pParentObjectInfo->Specific.Directory.PIOCtlDirectoryCB->ObjectInformation->Fcb);
             }
 
             *Fcb = NULL;
-
-            *Ccb = NULL;
         }
     }
 
@@ -3975,10 +3963,10 @@ try_exit:
 
                 AFSRemoveCcb( NULL,
                               *Ccb);
-
-                *Ccb = NULL;
             }
 
+            *Ccb = NULL;
+
             if( bAllocateFcb)
             {
 
@@ -3986,14 +3974,10 @@ try_exit:
                 // Need to tear down this Fcb since it is not in the tree for the worker thread
                 //
 
-                AFSRemoveFcb( *Fcb);
-
-                DirectoryCB->ObjectInformation->Fcb = NULL;
+                AFSRemoveFcb( &DirectoryCB->ObjectInformation->Fcb);
             }
 
             *Fcb = NULL;
-
-            *Ccb = NULL;
         }
     }
 

diff --git a/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp b/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp
index 0ea85fac5813606d3191669d4f6a43221838b795..9f697f79571d02059a63f509cb73980598f65166 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp
+++ b/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp
@@ -792,7 +792,7 @@ AFSRemoveVolume( IN AFSVolumeCB *VolumeCB)
             if( VolumeCB->ObjectInformation.Specific.Directory.PIOCtlDirectoryCB->ObjectInformation->Fcb != NULL)
             {
 
-                AFSRemoveFcb( VolumeCB->ObjectInformation.Specific.Directory.PIOCtlDirectoryCB->ObjectInformation->Fcb);
+                AFSRemoveFcb( &VolumeCB->ObjectInformation.Specific.Directory.PIOCtlDirectoryCB->ObjectInformation->Fcb);
             }
 
             AFSDeleteObjectInfo( VolumeCB->ObjectInformation.Specific.Directory.PIOCtlDirectoryCB->ObjectInformation);
@@ -1054,9 +1054,19 @@ AFSRemoveRootFcb( IN AFSFcb *RootFcb)
 //
 
 void
-AFSRemoveFcb( IN AFSFcb *Fcb)
+AFSRemoveFcb( IN AFSFcb **ppFcb)
 {
 
+    AFSFcb * pFcb;
+
+    pFcb = (AFSFcb *) InterlockedCompareExchangePointer( (PVOID *)ppFcb, NULL, (PVOID)(*ppFcb));
+
+    if ( pFcb == NULL)
+    {
+
+        return;
+    }
+
     //
     // Uninitialize the file lock if it is a file
     //
@@ -1064,23 +1074,23 @@ AFSRemoveFcb( IN AFSFcb *Fcb)
     AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING,
                   AFS_TRACE_LEVEL_VERBOSE,
                   "AFSRemoveFcb Removing Fcb %08lX\n",
-                  Fcb);
+                  pFcb);
 
-    if( Fcb->Header.NodeTypeCode == AFS_FILE_FCB)
+    if( pFcb->Header.NodeTypeCode == AFS_FILE_FCB)
     {
 
-        FsRtlUninitializeFileLock( &Fcb->Specific.File.FileLock);
+        FsRtlUninitializeFileLock( &pFcb->Specific.File.FileLock);
 
         //
         // The resource we allocated
         //
 
-        ExDeleteResourceLite( &Fcb->NPFcb->Specific.File.ExtentsResource );
+        ExDeleteResourceLite( &pFcb->NPFcb->Specific.File.ExtentsResource );
 
-        ExDeleteResourceLite( &Fcb->NPFcb->Specific.File.DirtyExtentsListLock);
+        ExDeleteResourceLite( &pFcb->NPFcb->Specific.File.DirtyExtentsListLock);
 
     }
-    else if( Fcb->Header.NodeTypeCode == AFS_DIRECTORY_FCB)
+    else if( pFcb->Header.NodeTypeCode == AFS_DIRECTORY_FCB)
     {
 
 
@@ -1090,29 +1100,29 @@ AFSRemoveFcb( IN AFSFcb *Fcb)
     // Tear down the FM specific contexts
     //
 
-    FsRtlTeardownPerStreamContexts( &Fcb->Header);
+    FsRtlTeardownPerStreamContexts( &pFcb->Header);
 
     //
     // Delete the resources
     //
 
-    ExDeleteResourceLite( &Fcb->NPFcb->Resource);
+    ExDeleteResourceLite( &pFcb->NPFcb->Resource);
 
-    ExDeleteResourceLite( &Fcb->NPFcb->PagingResource);
+    ExDeleteResourceLite( &pFcb->NPFcb->PagingResource);
 
-    ExDeleteResourceLite( &Fcb->NPFcb->CcbListLock);
+    ExDeleteResourceLite( &pFcb->NPFcb->CcbListLock);
 
     //
     // The non paged region
     //
 
-    AFSExFreePool( Fcb->NPFcb);
+    AFSExFreePool( pFcb->NPFcb);
 
     //
     // And the Fcb itself, which includes the name
     //
 
-    AFSExFreePool( Fcb);
+    AFSExFreePool( pFcb);
 
     return;
 }

diff --git a/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp b/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp
index 1c9df6e6b3fdbdf91753a4dc9057f9793f13dc9a..72d8fe21150db0284bbe1dbaba7b763f80fa3fde 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp
+++ b/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp
@@ -1056,8 +1056,7 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context)
                 continue;
             }
 
-            if( pVolumeCB->ObjectInfoListHead == NULL &&
-                pVolumeCB != AFSGlobalRoot)
+            if( pVolumeCB->ObjectInfoListHead == NULL)
             {
 
                 AFSReleaseResource( pVolumeCB->VolumeLock);
@@ -1202,7 +1201,7 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context)
 
                                     AFSReleaseResource( &pCurrentObject->Fcb->NPFcb->Resource);
 
-                                    AFSRemoveFcb( pCurrentObject->Fcb);
+                                    AFSRemoveFcb( &pCurrentObject->Fcb);
                                 }
 
                                 if( pCurrentObject->Specific.Directory.PIOCtlDirectoryCB != NULL)
@@ -1211,7 +1210,7 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context)
                                     if( pCurrentObject->Specific.Directory.PIOCtlDirectoryCB->ObjectInformation->Fcb != NULL)
                                     {
 
-                                        AFSRemoveFcb( pCurrentObject->Specific.Directory.PIOCtlDirectoryCB->ObjectInformation->Fcb);
+                                        AFSRemoveFcb( &pCurrentObject->Specific.Directory.PIOCtlDirectoryCB->ObjectInformation->Fcb);
                                     }
 
                                     AFSDeleteObjectInfo( pCurrentObject->Specific.Directory.PIOCtlDirectoryCB->ObjectInformation);
@@ -1412,7 +1411,7 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context)
 
                                             AFSReleaseResource( &pCurrentChildObject->Fcb->NPFcb->Resource);
 
-                                            AFSRemoveFcb( pCurrentChildObject->Fcb);
+                                            AFSRemoveFcb( &pCurrentChildObject->Fcb);
                                         }
 
                                         if( pCurrentChildObject->FileType == AFS_FILE_TYPE_DIRECTORY &&
@@ -1422,7 +1421,7 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context)
                                             if( pCurrentChildObject->Specific.Directory.PIOCtlDirectoryCB->ObjectInformation->Fcb != NULL)
                                             {
 
-                                                AFSRemoveFcb( pCurrentChildObject->Specific.Directory.PIOCtlDirectoryCB->ObjectInformation->Fcb);
+                                                AFSRemoveFcb( &pCurrentChildObject->Specific.Directory.PIOCtlDirectoryCB->ObjectInformation->Fcb);
                                             }
 
                                             AFSDeleteObjectInfo( pCurrentChildObject->Specific.Directory.PIOCtlDirectoryCB->ObjectInformation);
@@ -1556,7 +1555,7 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context)
 
                                     AFSReleaseResource( &pCurrentObject->Fcb->NPFcb->Resource);
 
-                                    AFSRemoveFcb( pCurrentObject->Fcb);
+                                    AFSRemoveFcb( &pCurrentObject->Fcb);
                                 }
 
                                 AFSDeleteObjectInfo( pCurrentObject);

diff --git a/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h b/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h
index ad4759c0468a75d24cc9f573b4a5b6bc74ac8042..7f28406e466e5ce5c735f45c1f986260ce9c6f08 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h
+++ b/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h
@@ -515,7 +515,7 @@ NTSTATUS
 AFSInitCcb( IN OUT AFSCcb **Ccb);
 
 void
-AFSRemoveFcb( IN AFSFcb *Fcb);
+AFSRemoveFcb( IN AFSFcb **Fcb);
 
 NTSTATUS
 AFSRemoveCcb( IN AFSFcb *Fcb,