From 5c4e49062af0e9b81b963d1d6e50c7cc52c68eb9 Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Sat, 11 Feb 2012 12:49:33 -0500 Subject: [PATCH] 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 Tested-by: BuildBot Tested-by: Jeffrey Altman Reviewed-by: Jeffrey Altman --- src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp | 36 +++++------------- src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp | 38 ++++++++++++------- src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp | 13 +++---- .../afsrdr/kernel/lib/Include/AFSCommon.h | 2 +- 4 files changed, 41 insertions(+), 48 deletions(-) diff --git a/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp b/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp index 63fae848d..7777da9ea 100644 --- 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 0ea85fac5..9f697f795 100644 --- 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 1c9df6e6b..72d8fe211 100644 --- 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 ad4759c04..7f28406e4 100644 --- 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, -- 2.39.5