Windows: FCB cleanup must be done before ObjectInfo
authorJeffrey Altman <jaltman@your-file-system.com>
Fri, 4 May 2012 00:01:22 +0000 (20:01 -0400)
committerJeffrey Altman <jaltman@secure-endpoints.com>
Fri, 4 May 2012 17:27:53 +0000 (10:27 -0700)
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 <buildbot@rampaginggeek.com>
Reviewed-by: Jeffrey Altman <jaltman@secure-endpoints.com>
Tested-by: Jeffrey Altman <jaltman@secure-endpoints.com>

src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp
src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp

index abc2d20..72b0a78 100644 (file)
@@ -7153,7 +7153,7 @@ AFSCleanupFcb( IN AFSFcb *Fcb,
 
             AFSReleaseResource( &Fcb->NPFcb->Resource);
 
-            if( Fcb->OpenReferenceCount == 0)
+            if( Fcb->OpenReferenceCount <= 0)
             {
 
                 //
index e9305fe..f0ec3eb 100644 (file)
@@ -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))
-                        {
-
-                            pFcb = (AFSFcb *) InterlockedCompareExchangePointer( (PVOID *)&pCurrentObject->Fcb, NULL, (PVOID)pCurrentObject->Fcb);
+                        AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock);
 
-                            if( pFcb != NULL)
-                            {
+                        if( pCurrentObject->Fcb != NULL)
+                        {
 
-                                AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock);
+                            //
+                            // Dropping the TreeLock permits the
+                            // pCurrentObject->ObjectReferenceCount to change
+                            //
 
-                                //
-                                // Dropping the TreeLock permits the
-                                // pCurrentObject->ObjectReferenceCount to change
-                                //
+                            AFSCleanupFcb( pCurrentObject->Fcb,
+                                           TRUE);
+                        }
 
-                                AFSCleanupFcb( pFcb,
-                                               TRUE);
+                        if( !AFSAcquireExcl( pVolumeCB->ObjectInfoTree.TreeLock,
+                                             FALSE))
+                        {
 
-                                AFSAcquireExcl( pVolumeCB->ObjectInfoTree.TreeLock,
-                                                TRUE);
+                            bReleaseVolumeLock = FALSE;
 
-                                AFSRemoveFcb( &pFcb);
-                            }
+                            break;
+                        }
 
-                            AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock);
+                        if( BooleanFlagOn( pCurrentObject->Flags, AFS_OBJECT_FLAGS_DELETED) &&
+                            pCurrentObject->ObjectReferenceCount <= 0 &&
+                            ( pCurrentObject->Fcb == NULL ||
+                              pCurrentObject->Fcb->OpenReferenceCount == 0))
+                        {
 
-                            if( AFSAcquireExcl( pVolumeCB->ObjectInfoTree.TreeLock,
-                                                FALSE))
+                            if( pCurrentObject->Fcb != NULL)
                             {
 
-                                AFSDeleteObjectInfo( pCurrentObject);
-
-                                AFSConvertToShared( pVolumeCB->ObjectInfoTree.TreeLock);
-
-                                pCurrentObject = pNextObject;
-
-                                continue;
+                                AFSRemoveFcb( &pCurrentObject->Fcb);
                             }
-                            else
-                            {
-
-                                bReleaseVolumeLock = FALSE;
 
-                                break;
-                            }
+                            AFSDeleteObjectInfo( pCurrentObject);
                         }
-                        else if( pCurrentObject->Fcb != NULL)
-                        {
 
-                            AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock);
+                        AFSConvertToShared( pVolumeCB->ObjectInfoTree.TreeLock);
 
-                            //
-                            // Dropping the TreeLock permits the
-                            // pCurrentObject->ObjectReferenceCount to change
-                            //
+                        pCurrentObject = pNextObject;
 
-                            AFSCleanupFcb( pCurrentObject->Fcb,
-                                           FALSE);
-
-                            AFSAcquireShared( pVolumeCB->ObjectInfoTree.TreeLock,
-                                              TRUE);
-                        }
+                        continue;
                     }
 
                     pCurrentObject = pNextObject;