From f76cf9a3fb30b8c89c91ca10eaff8308cab9630a Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Thu, 3 May 2012 20:01:22 -0400 Subject: [PATCH] Windows: FCB cleanup must be done before ObjectInfo When processing the cleanup and destruction of a File Control Block the related ObjectInfoCB is required for proper cleanup. Reorganize the AFSPrimaryVolumeWorkerThread logic to ensure that this is true. This involves dropping the VolumeCB->ObjectInfoTree.TreeLock around the AFSCleanupFcb() call. While the lock is released it is possible for the ObjectInfoCB->OpenReferenceCount to change. Therefore, new checks must be added after the lock is re-acquired to ensure that an in-use object is not destroyed. Change-Id: I6b26fb2fe1ef4077c6edd643ec40715c8e2928ac Reviewed-on: http://gerrit.openafs.org/7327 Tested-by: BuildBot Reviewed-by: Jeffrey Altman Tested-by: Jeffrey Altman --- src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp | 2 +- src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp | 148 +++++++++------------ 2 files changed, 62 insertions(+), 88 deletions(-) diff --git a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp index abc2d200fc..72b0a7883a 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp @@ -7153,7 +7153,7 @@ AFSCleanupFcb( IN AFSFcb *Fcb, AFSReleaseResource( &Fcb->NPFcb->Resource); - if( Fcb->OpenReferenceCount == 0) + if( Fcb->OpenReferenceCount <= 0) { // diff --git a/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp b/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp index e9305feb85..f0ec3eb8d4 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp @@ -1387,15 +1387,38 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) AFSDeleteDirEntry( pCurrentObject, pCurrentDirEntry); + if( pCurrentChildObject->ObjectReferenceCount <= 0 && + pCurrentChildObject->Fcb != NULL && + pCurrentChildObject->FileType == AFS_FILE_TYPE_FILE) + { + + // + // We must not hold pVolumeCB->ObjectInfoTree.TreeLock exclusive + // across an AFSCleanupFcb call since it can deadlock with an + // invalidation call from the service. + // + + AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock); + + // + // Dropping the TreeLock permits the + // pCurrentObject->ObjectReferenceCount to change + // + + AFSCleanupFcb( pCurrentChildObject->Fcb, + TRUE); + + AFSAcquireExcl( pVolumeCB->ObjectInfoTree.TreeLock, + TRUE); + } + if( pCurrentChildObject->ObjectReferenceCount <= 0) { if( pCurrentChildObject->Fcb != NULL) { - - pFcb = (AFSFcb *) InterlockedCompareExchangePointer( (PVOID *)&pCurrentChildObject->Fcb, NULL, (PVOID)pCurrentChildObject->Fcb); - - lFileType = pCurrentChildObject->FileType; + + AFSRemoveFcb( &pCurrentChildObject->Fcb); } if( pCurrentChildObject->FileType == AFS_FILE_TYPE_DIRECTORY && @@ -1427,33 +1450,6 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) pCurrentDirEntry = pNextDirEntry; - if ( pFcb != NULL) - { - - if( lFileType == AFS_FILE_TYPE_FILE) - { - // - // We must not hold pVolumeCB->ObjectInfoTree.TreeLock exclusive - // across an AFSCleanupFcb call since it can deadlock with an - // invalidation call from the service. - // - - AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock); - - // - // Dropping the TreeLock permits the - // pCurrentObject->ObjectReferenceCount to change - // - - AFSCleanupFcb( pFcb, - TRUE); - - AFSAcquireExcl( pVolumeCB->ObjectInfoTree.TreeLock, - TRUE); - } - - AFSRemoveFcb( &pFcb); - } } pCurrentObject->Specific.Directory.DirectoryNodeListHead = NULL; @@ -1536,71 +1532,49 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) else if( pCurrentObject->FileType == AFS_FILE_TYPE_FILE) { - if( BooleanFlagOn( pCurrentObject->Flags, AFS_OBJECT_FLAGS_DELETED) && - pCurrentObject->ObjectReferenceCount <= 0 && - ( pCurrentObject->Fcb == NULL || - pCurrentObject->Fcb->OpenReferenceCount == 0)) + AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock); + + if( pCurrentObject->Fcb != NULL) { - pFcb = (AFSFcb *) InterlockedCompareExchangePointer( (PVOID *)&pCurrentObject->Fcb, NULL, (PVOID)pCurrentObject->Fcb); - - if( pFcb != NULL) - { - - AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock); - - // - // Dropping the TreeLock permits the - // pCurrentObject->ObjectReferenceCount to change - // - - AFSCleanupFcb( pFcb, - TRUE); - - AFSAcquireExcl( pVolumeCB->ObjectInfoTree.TreeLock, - TRUE); - - AFSRemoveFcb( &pFcb); - } - - AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock); - - if( AFSAcquireExcl( pVolumeCB->ObjectInfoTree.TreeLock, - FALSE)) - { - - AFSDeleteObjectInfo( pCurrentObject); - - AFSConvertToShared( pVolumeCB->ObjectInfoTree.TreeLock); - - pCurrentObject = pNextObject; - - continue; - } - else - { - - bReleaseVolumeLock = FALSE; - - break; - } - } - else if( pCurrentObject->Fcb != NULL) - { - - AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock); - // // Dropping the TreeLock permits the // pCurrentObject->ObjectReferenceCount to change // AFSCleanupFcb( pCurrentObject->Fcb, - FALSE); - - AFSAcquireShared( pVolumeCB->ObjectInfoTree.TreeLock, - TRUE); + TRUE); } + + if( !AFSAcquireExcl( pVolumeCB->ObjectInfoTree.TreeLock, + FALSE)) + { + + bReleaseVolumeLock = FALSE; + + break; + } + + if( BooleanFlagOn( pCurrentObject->Flags, AFS_OBJECT_FLAGS_DELETED) && + pCurrentObject->ObjectReferenceCount <= 0 && + ( pCurrentObject->Fcb == NULL || + pCurrentObject->Fcb->OpenReferenceCount == 0)) + { + + if( pCurrentObject->Fcb != NULL) + { + + AFSRemoveFcb( &pCurrentObject->Fcb); + } + + AFSDeleteObjectInfo( pCurrentObject); + } + + AFSConvertToShared( pVolumeCB->ObjectInfoTree.TreeLock); + + pCurrentObject = pNextObject; + + continue; } pCurrentObject = pNextObject;