Windows: Hold ProcessTreeLock across AFSValidateProcessEntry

AFSValidateProcessEntry() is called from AFSProcessCreate() which
holds the ProcessTree.TreeLock exclusive across the call and from
AFSCreate() and AFSRetrieveAuthGroup() where it is not held at all.

Add a parameter to AFSValidateProcessEntry() that indicates whether
or not the ProcessTree.TreeLock is held.  If it is held, it must be
held exclusive.  If it is not held, the lock must be acquired within
AFSValidateProcessEntry() and it must be held for the entire lifetime
of the pParentProcessCB reference.  Failure to hold the TreeLock
for the lifetime of the reference permits a race with AFSProcessDestroy()
that can result in the parent ProcessCB being destroyed while its
Resource is held.

Change-Id: I7cf0dff6bd541b0588a060d677a8e3d724858b96
Reviewed-on: http://gerrit.openafs.org/8455
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Rod Widdowson <rdw@steadingsoftware.com>
Tested-by: Jeffrey Altman <jaltman@your-file-system.com>
Reviewed-by: Jeffrey Altman <jaltman@your-file-system.com>

diff --git a/src/WINNT/afsrdr/kernel/fs/AFSAuthGroupSupport.cpp b/src/WINNT/afsrdr/kernel/fs/AFSAuthGroupSupport.cpp
index 105bf09b9b255cfbc2000bd2d06b9e96ce7b28fc..5656ad775ebadc0108f5e281be4054f313e15a2f 100644 (file)
--- a/src/WINNT/afsrdr/kernel/fs/AFSAuthGroupSupport.cpp
+++ b/src/WINNT/afsrdr/kernel/fs/AFSAuthGroupSupport.cpp
@@ -135,6 +135,7 @@ AFSRetrieveAuthGroup( IN ULONGLONG ProcessId,
                           ThreadId);
 
             AFSReleaseResource( pDeviceExt->Specific.Control.ProcessTree.TreeLock);
+
             try_return( ntStatus);
         }
 
@@ -224,7 +225,8 @@ AFSRetrieveAuthGroup( IN ULONGLONG ProcessId,
                           ProcessId,
                           ThreadId);
 
-            pAuthGroup = AFSValidateProcessEntry( PsGetCurrentProcessId());
+            pAuthGroup = AFSValidateProcessEntry( PsGetCurrentProcessId(),
+                                                  FALSE);
 
             if( pAuthGroup != NULL)
             {

diff --git a/src/WINNT/afsrdr/kernel/fs/AFSCreate.cpp b/src/WINNT/afsrdr/kernel/fs/AFSCreate.cpp
index fde3259ec4d2361ba01d581862e99a85bdb7cc1b..2ecf0e3739045dceea48ce96d24cd76bd9e325e1 100644 (file)
--- a/src/WINNT/afsrdr/kernel/fs/AFSCreate.cpp
+++ b/src/WINNT/afsrdr/kernel/fs/AFSCreate.cpp
@@ -120,7 +120,8 @@ AFSCommonCreate( IN PDEVICE_OBJECT DeviceObject,
         // Validate the process entry
         //
 
-        pAuthGroup = AFSValidateProcessEntry( PsGetCurrentProcessId());
+        pAuthGroup = AFSValidateProcessEntry( PsGetCurrentProcessId(),
+                                              FALSE);
 
         if( pAuthGroup != NULL)
         {

diff --git a/src/WINNT/afsrdr/kernel/fs/AFSProcessSupport.cpp b/src/WINNT/afsrdr/kernel/fs/AFSProcessSupport.cpp
index 88b7a3c87466b0ba97654ab1e0761349ac1e3e1b..35326b253b01aadc3eb1c092de5dc9c385c7acda 100644 (file)
--- a/src/WINNT/afsrdr/kernel/fs/AFSProcessSupport.cpp
+++ b/src/WINNT/afsrdr/kernel/fs/AFSProcessSupport.cpp
@@ -131,7 +131,8 @@ AFSProcessCreate( IN HANDLE ParentId,
             // Now assign the AuthGroup ACE
             //
 
-            AFSValidateProcessEntry( ProcessId);
+            AFSValidateProcessEntry( ProcessId,
+                                     TRUE);
         }
         else
         {
@@ -243,7 +244,8 @@ AFSProcessDestroy( IN HANDLE ProcessId)
 //
 
 GUID *
-AFSValidateProcessEntry( IN HANDLE ProcessId)
+AFSValidateProcessEntry( IN HANDLE  ProcessId,
+                         IN BOOLEAN bProcessTreeLocked)
 {
 
     GUID *pAuthGroup = NULL;
@@ -263,18 +265,22 @@ AFSValidateProcessEntry( IN HANDLE ProcessId)
     __Enter
     {
 
-        AFSDbgLogMsg( AFS_SUBSYSTEM_LOCK_PROCESSING,
-                      AFS_TRACE_LEVEL_VERBOSE,
-                      "AFSValidateProcessEntry Acquiring Control ProcessTree.TreeLock lock %08lX SHARED %08lX\n",
-                      pDeviceExt->Specific.Control.ProcessTree.TreeLock,
-                      PsGetCurrentThread());
-
         uniSIDString.Length = 0;
         uniSIDString.MaximumLength = 0;
         uniSIDString.Buffer = NULL;
 
-        AFSAcquireShared( pDeviceExt->Specific.Control.ProcessTree.TreeLock,
-                          TRUE);
+        if ( !bProcessTreeLocked)
+        {
+
+            AFSDbgLogMsg( AFS_SUBSYSTEM_LOCK_PROCESSING,
+                          AFS_TRACE_LEVEL_VERBOSE,
+                          "AFSValidateProcessEntry Acquiring Control ProcessTree.TreeLock lock %08lX SHARED %08lX\n",
+                          pDeviceExt->Specific.Control.ProcessTree.TreeLock,
+                          PsGetCurrentThread());
+
+            AFSAcquireShared( pDeviceExt->Specific.Control.ProcessTree.TreeLock,
+                              TRUE);
+        }
 
         AFSDbgLogMsg( AFS_SUBSYSTEM_AUTHGROUP_PROCESSING,
                       AFS_TRACE_LEVEL_VERBOSE,
@@ -290,10 +296,14 @@ AFSValidateProcessEntry( IN HANDLE ProcessId)
             pProcessCB == NULL)
         {
 
-            AFSReleaseResource( pDeviceExt->Specific.Control.ProcessTree.TreeLock);
+            if ( !bProcessTreeLocked)
+            {
 
-            AFSAcquireExcl( pDeviceExt->Specific.Control.ProcessTree.TreeLock,
-                            TRUE);
+                AFSReleaseResource( pDeviceExt->Specific.Control.ProcessTree.TreeLock);
+
+                AFSAcquireExcl( pDeviceExt->Specific.Control.ProcessTree.TreeLock,
+                                TRUE);
+            }
 
             ntStatus = AFSLocateHashEntry( pDeviceExt->Specific.Control.ProcessTree.TreeHead,
                                            ullProcessID,
@@ -319,12 +329,14 @@ AFSValidateProcessEntry( IN HANDLE ProcessId)
                               __FUNCTION__,
                               ullProcessID);
 
-                AFSReleaseResource( pDeviceExt->Specific.Control.ProcessTree.TreeLock);
-
                 try_return( ntStatus = STATUS_UNSUCCESSFUL);
             }
 
-            AFSConvertToShared( pDeviceExt->Specific.Control.ProcessTree.TreeLock);
+            if ( !bProcessTreeLocked)
+            {
+
+                AFSConvertToShared( pDeviceExt->Specific.Control.ProcessTree.TreeLock);
+            }
         }
 
         //
@@ -370,8 +382,6 @@ AFSValidateProcessEntry( IN HANDLE ProcessId)
         AFSAcquireExcl( &pProcessCB->Lock,
                         TRUE);
 
-        AFSReleaseResource( pDeviceExt->Specific.Control.ProcessTree.TreeLock);
-
 #if defined(_WIN64)
 
         //
@@ -782,6 +792,12 @@ try_exit:
         {
             RtlFreeUnicodeString( &uniSIDString);
         }
+
+        if ( !bProcessTreeLocked)
+        {
+
+            AFSReleaseResource( pDeviceExt->Specific.Control.ProcessTree.TreeLock);
+        }
     }
 
     return pAuthGroup;

diff --git a/src/WINNT/afsrdr/kernel/fs/Include/AFSCommon.h b/src/WINNT/afsrdr/kernel/fs/Include/AFSCommon.h
index 65c14af66c1dc5923d80cce2c1955439389c8d5f..9af1fa761f5e2f6c46bacb34358c6d406b49e88e 100644 (file)
--- a/src/WINNT/afsrdr/kernel/fs/Include/AFSCommon.h
+++ b/src/WINNT/afsrdr/kernel/fs/Include/AFSCommon.h
@@ -815,7 +815,8 @@ void
 AFSProcessDestroy( IN HANDLE ProcessId);
 
 GUID *
-AFSValidateProcessEntry( IN HANDLE ProcessId);
+AFSValidateProcessEntry( IN HANDLE  ProcessId,
+                         IN BOOLEAN bProcessTreeLocked);
 
 BOOLEAN
 AFSIs64BitProcess( IN ULONGLONG ProcessId);