[ros-diffs] [sginsberg] 44119: - Add missing synchronization for access to PsLoadedModuleList. Inserting and removing entries to the list was protected by a spinlock, but some places didn't acquire the lock before looping the list when searching for module information. For efficiency, use an executive resource for other access than inserting/removing entries so we can have shared locking in some common cases (spinlock stays so access can be synchronized with DPCs). - Make SystemUnloadGdiDriverInformation ta

Author: sginsberg
Date: Thu Nov 12 20:41:39 2009
New Revision: 44119

URL: http://svn.reactos.org/svn/reactos?rev=44119&view=rev
Log:
- Add missing synchronization for access to PsLoadedModuleList. Inserting and removing
entries to the list was protected by a spinlock, but some places didn't acquire the
lock before looping the list when searching for module information. For efficiency, use an
executive resource for other access than inserting/removing entries so we can have shared
locking in some common cases (spinlock stays so access can be synchronized with DPCs).
- Make SystemUnloadGdiDriverInformation take the opaque SectionPointer (which is really
the Loader Entry) from the SYSTEM_GDI_DRIVER_INFORMATION structure returned in
SystemLoadGdiDriverInformation so it doesn't need to loop the loaded module list to
find it.
- Fix EngLoad/UnloadImage to do this. Also make EngLoadImage return the pointer to the
internal structure as the "handle" so EngUnloadImage doesn't need to loop
the driver list to find it. The code is still extremely broken and needs a rewrite.
- Remove some externs from related variables in the code -- we use something called
"headers".

Modified:
    trunk/reactos/ntoskrnl/ex/sysinfo.c
    trunk/reactos/ntoskrnl/include/internal/mm.h
    trunk/reactos/ntoskrnl/include/internal/ps.h
    trunk/reactos/ntoskrnl/io/iomgr/driver.c
    trunk/reactos/ntoskrnl/mm/ARM3/drvmgmt.c
    trunk/reactos/ntoskrnl/mm/mminit.c
    trunk/reactos/ntoskrnl/mm/sysldr.c
    trunk/reactos/subsystems/win32/win32k/ldr/loader.c

Modified: trunk/reactos/ntoskrnl/ex/sysinfo.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ex/sysinfo.c?rev=…
==============================================================================
--- trunk/reactos/ntoskrnl/ex/sysinfo.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/ex/sysinfo.c [iso-8859-1] Thu Nov 12 20:41:39 2009
@@ -138,17 +138,6 @@
 }
 
 /* FUNCTIONS *****************************************************************/
-
-/*
- * @implemented
- */
-#undef ExGetPreviousMode
-KPROCESSOR_MODE
-NTAPI
-ExGetPreviousMode (VOID)
-{
-    return KeGetPreviousMode();
-}
 
 /*
  * @implemented
@@ -1006,12 +995,23 @@
 /* Class 11 - Module Information */
 QSI_DEF(SystemModuleInformation)
 {
-    extern LIST_ENTRY PsLoadedModuleList;
-    return ExpQueryModuleInformation(&PsLoadedModuleList,
-                                     NULL,
-                                     (PRTL_PROCESS_MODULES)Buffer,
-                                     Size,
-                                     ReqSize);
+    NTSTATUS Status;
+
+    /* Acquire system module list lock */
+    KeEnterCriticalRegion();
+    ExAcquireResourceExclusiveLite(&PsLoadedModuleResource, TRUE);
+
+    /* Call the generic handler with the system module list */
+    Status = ExpQueryModuleInformation(&PsLoadedModuleList,
+                                       &MmLoadedUserImageList,
+                                       (PRTL_PROCESS_MODULES)Buffer,
+                                       Size,
+                                       ReqSize);
+
+    /* Release list lock and return status */
+    ExReleaseResourceLite(&PsLoadedModuleResource);
+    KeLeaveCriticalRegion();
+    return Status;
 }
 
 /* Class 12 - Locks Information */
@@ -1322,10 +1322,9 @@
 SSI_DEF(SystemLoadGdiDriverInformation)
 {
     PSYSTEM_GDI_DRIVER_INFORMATION DriverInfo = (PVOID)Buffer;
-    KPROCESSOR_MODE PreviousMode = KeGetPreviousMode();
     UNICODE_STRING ImageName;
     PVOID ImageBase;
-    PLDR_DATA_TABLE_ENTRY ModuleObject;
+    PVOID SectionPointer;
     ULONG_PTR EntryPoint;
     NTSTATUS Status;
     ULONG DirSize;
@@ -1338,8 +1337,8 @@
         return STATUS_INFO_LENGTH_MISMATCH;
     }
 
-    /* Only kernel-mode can call this function */
-    if (PreviousMode != KernelMode) return STATUS_PRIVILEGE_NOT_HELD;
+    /* Only kernel mode can call this function */
+    if (ExGetPreviousMode() != KernelMode) return STATUS_PRIVILEGE_NOT_HELD;
 
     /* Load the driver */
     ImageName = DriverInfo->DriverName;
@@ -1347,7 +1346,7 @@
                                NULL,
                                NULL,
                                0,
-                               (PVOID)&ModuleObject,
+                               &SectionPointer,
                                &ImageBase);
     if (!NT_SUCCESS(Status)) return Status;
 
@@ -1365,7 +1364,7 @@
 
     /* Save other data */
     DriverInfo->ImageAddress = ImageBase;
-    DriverInfo->SectionPointer = NULL;
+    DriverInfo->SectionPointer = SectionPointer;
     DriverInfo->EntryPoint = (PVOID)EntryPoint;
     DriverInfo->ImageLength = NtHeader->OptionalHeader.SizeOfImage;
 
@@ -1376,44 +1375,21 @@
 /* Class 27 - Unload Image */
 SSI_DEF(SystemUnloadGdiDriverInformation)
 {
-    PLDR_DATA_TABLE_ENTRY LdrEntry;
-    PLIST_ENTRY NextEntry;
-    PVOID BaseAddr = *((PVOID*)Buffer);
-
-    if(Size != sizeof(PVOID))
+    PVOID SectionPointer = Buffer;
+
+    /* Validate size */
+    if (Size != sizeof(PVOID))
+    {
+        /* Incorrect length, fail */
         return STATUS_INFO_LENGTH_MISMATCH;
-
-    if(KeGetPreviousMode() != KernelMode)
-        return STATUS_PRIVILEGE_NOT_HELD;
-
-    // Scan the module list
-    NextEntry = PsLoadedModuleList.Flink;
-    while(NextEntry != &PsLoadedModuleList)
-    {
-        LdrEntry = CONTAINING_RECORD(NextEntry,
-                                     LDR_DATA_TABLE_ENTRY,
-                                     InLoadOrderLinks);
-
-        if (LdrEntry->DllBase == BaseAddr)
-        {
-            // Found it.
-            break;
-        }
-
-        NextEntry = NextEntry->Flink;
-    }
-
-    // Check if we found the image
-    if(NextEntry != &PsLoadedModuleList)
-    {
-        return MmUnloadSystemImage(LdrEntry);
-    }
-    else
-    {
-        DPRINT1("Image 0x%x not found.\n", BaseAddr);
-        return STATUS_DLL_NOT_FOUND;
-    }
-
+    }
+
+    /* Only kernel mode can call this function */
+    if (ExGetPreviousMode() != KernelMode) return STATUS_PRIVILEGE_NOT_HELD;
+
+    /* Unload the image */
+    MmUnloadSystemImage(SectionPointer);
+    return STATUS_SUCCESS;
 }
 
 /* Class 28 - Time Adjustment Information */
@@ -1986,7 +1962,6 @@
     return STATUS_INVALID_INFO_CLASS;
 }
 
-
 NTSTATUS
 NTAPI
 NtFlushInstructionCache(IN HANDLE ProcessHandle,
@@ -2018,4 +1993,13 @@
     return KeGetCurrentProcessorNumber();
 }
 
-/* EOF */
+/*
+ * @implemented
+ */
+#undef ExGetPreviousMode
+KPROCESSOR_MODE
+NTAPI
+ExGetPreviousMode (VOID)
+{
+    return KeGetPreviousMode();
+}

Modified: trunk/reactos/ntoskrnl/include/internal/mm.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/include/internal/…
==============================================================================
--- trunk/reactos/ntoskrnl/include/internal/mm.h [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/include/internal/mm.h [iso-8859-1] Thu Nov 12 20:41:39 2009
@@ -27,6 +27,8 @@
 extern MEMORY_ALLOCATION_DESCRIPTOR MiFreeDescriptorOrg;
 
 extern LIST_ENTRY MmLoadedUserImageList;
+
+extern KMUTANT MmSystemLoadLock;
 
 extern ULONG MmNumberOfPagingFiles;
 

Modified: trunk/reactos/ntoskrnl/include/internal/ps.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/include/internal/…
==============================================================================
--- trunk/reactos/ntoskrnl/include/internal/ps.h [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/include/internal/ps.h [iso-8859-1] Thu Nov 12 20:41:39 2009
@@ -434,6 +434,8 @@
 extern LARGE_INTEGER ShortPsLockDelay;
 extern UNICODE_STRING PsNtDllPathName;
 extern LIST_ENTRY PsLoadedModuleList;
+extern KSPIN_LOCK PsLoadedModuleSpinLock;
+extern ERESOURCE PsLoadedModuleResource;
 extern ULONG_PTR PsNtosImageBase;
 
 //

Modified: trunk/reactos/ntoskrnl/io/iomgr/driver.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/io/iomgr/driver.c…
==============================================================================
--- trunk/reactos/ntoskrnl/io/iomgr/driver.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/io/iomgr/driver.c [iso-8859-1] Thu Nov 12 20:41:39 2009
@@ -692,8 +692,6 @@
                          OUT PWCHAR *MissingDriver,
                          OUT PLOAD_IMPORTS *LoadImports);
 
-extern KSPIN_LOCK PsLoadedModuleSpinLock;
-
 //
 // Used for images already loaded (boot drivers)
 //

Modified: trunk/reactos/ntoskrnl/mm/ARM3/drvmgmt.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/drvmgmt.c…
==============================================================================
--- trunk/reactos/ntoskrnl/mm/ARM3/drvmgmt.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/drvmgmt.c [iso-8859-1] Thu Nov 12 20:41:39 2009
@@ -21,8 +21,6 @@
 MM_DRIVER_VERIFIER_DATA MmVerifierData;
 LIST_ENTRY MiVerifierDriverAddedThunkListHead;
 ULONG MiActiveVerifierThunks;
-extern KMUTANT MmSystemLoadLock;
-extern LIST_ENTRY PsLoadedModuleList;
 
 /* PRIVATE FUNCTIONS *********************************************************/
 

Modified: trunk/reactos/ntoskrnl/mm/mminit.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/mminit.c?rev=4…
==============================================================================
--- trunk/reactos/ntoskrnl/mm/mminit.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/mminit.c [iso-8859-1] Thu Nov 12 20:41:39 2009
@@ -56,7 +56,6 @@
 UCHAR MmDisablePagingExecutive = 1; // Forced to off
 PMMPTE MmSharedUserDataPte;
 PMMSUPPORT MmKernelAddressSpace;
-extern KMUTANT MmSystemLoadLock;
 BOOLEAN MiDbgEnableMdDump =
 #ifdef _ARM_
 TRUE;

Modified: trunk/reactos/ntoskrnl/mm/sysldr.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/sysldr.c?rev=4…
==============================================================================
--- trunk/reactos/ntoskrnl/mm/sysldr.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/sysldr.c [iso-8859-1] Thu Nov 12 20:41:39 2009
@@ -30,6 +30,7 @@
 LIST_ENTRY PsLoadedModuleList;
 LIST_ENTRY MmLoadedUserImageList;
 KSPIN_LOCK PsLoadedModuleSpinLock;
+ERESOURCE PsLoadedModuleResource;
 ULONG_PTR PsNtosImageBase;
 KMUTANT MmSystemLoadLock;
 
@@ -438,15 +439,21 @@
 {
     KIRQL OldIrql;
 
-    /* Acquire the lock */
-    KeAcquireSpinLock(&PsLoadedModuleSpinLock, &OldIrql);
+    /* Acquire module list lock */
+    KeEnterCriticalRegion();
+    ExAcquireResourceExclusiveLite(&PsLoadedModuleResource, TRUE);
+
+    /* Acquire the spinlock too as we will insert or remove the entry */
+    OldIrql = KeAcquireSpinLockRaiseToSynch(&PsLoadedModuleSpinLock);
 
     /* Insert or remove from the list */
     Insert ? InsertTailList(&PsLoadedModuleList, &LdrEntry->InLoadOrderLinks)
:
              RemoveEntryList(&LdrEntry->InLoadOrderLinks);
 
-    /* Release the lock */
+    /* Release locks */
     KeReleaseSpinLock(&PsLoadedModuleSpinLock, OldIrql);
+    ExReleaseResourceLite(&PsLoadedModuleResource);
+    KeLeaveCriticalRegion();
 }
 
 VOID
@@ -1332,7 +1339,8 @@
     PLIST_ENTRY ListHead, NextEntry;
     ULONG EntrySize;
 
-    /* Setup the loaded module list and lock */
+    /* Setup the loaded module list and locks */
+    ExInitializeResourceLite(&PsLoadedModuleResource);
     KeInitializeSpinLock(&PsLoadedModuleSpinLock);
     InitializeListHead(&PsLoadedModuleList);
 
@@ -2005,6 +2013,7 @@
 
     /* Lock the list */
     KeEnterCriticalRegion();
+    ExAcquireResourceSharedLite(&PsLoadedModuleResource, TRUE);
 
     /* Loop the loaded module list */
     NextEntry = PsLoadedModuleList.Flink;
@@ -2046,6 +2055,7 @@
     }
 
     /* Release the lock */
+    ExReleaseResourceLite(&PsLoadedModuleResource);
     KeLeaveCriticalRegion();
 
     /* Free the string and return */

Modified: trunk/reactos/subsystems/win32/win32k/ldr/loader.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/ld…
==============================================================================
--- trunk/reactos/subsystems/win32/win32k/ldr/loader.c [iso-8859-1] (original)
+++ trunk/reactos/subsystems/win32/win32k/ldr/loader.c [iso-8859-1] Thu Nov 12 20:41:39
2009
@@ -29,11 +29,12 @@
 typedef struct _DRIVERS
 {
 	LIST_ENTRY ListEntry;
-	HANDLE ImageHandle;
+    PVOID SectionPointer;
 	UNICODE_STRING DriverName;
 }DRIVERS, *PDRIVERS;
 
 extern LIST_ENTRY GlobalDriverListHead;
+
 
 /*
  * Blatantly stolen from ldr/utils.c in ntdll.  I can't link ntdll from
@@ -199,8 +200,8 @@
 APIENTRY
 EngLoadImage (LPWSTR DriverName)
 {
-	HANDLE hImageHandle = NULL;
 	SYSTEM_GDI_DRIVER_INFORMATION GdiDriverInfo;
+	PDRIVERS DriverInfo = NULL;
 	NTSTATUS Status;
 
 	RtlInitUnicodeString(&GdiDriverInfo.DriverName, DriverName);
@@ -213,14 +214,14 @@
 		{
 			Current = CONTAINING_RECORD(CurrentEntry, DRIVERS, ListEntry);
 			if( Current && (0 == RtlCompareUnicodeString(&GdiDriverInfo.DriverName,
&Current->DriverName, FALSE)) ) {
-				hImageHandle = Current->ImageHandle;
+				DriverInfo = Current;
 				break;
 			}
 			CurrentEntry = CurrentEntry->Flink;
 		};
 	}
 
-	if( !hImageHandle )
+	if( !DriverInfo )
 	{
 		/* the driver was not loaded before, so let's do that */
 		Status = ZwSetSystemInformation(SystemLoadGdiDriverInformation, &GdiDriverInfo,
sizeof(SYSTEM_GDI_DRIVER_INFORMATION));
@@ -228,19 +229,17 @@
 			DPRINT1("ZwSetSystemInformation failed with Status 0x%lx\n", Status);
 		}
 		else {
-            PDRIVERS DriverInfo;
-			hImageHandle = (HANDLE)GdiDriverInfo.ImageAddress;
 			DriverInfo = ExAllocatePool(PagedPool, sizeof(DRIVERS));
 			DriverInfo->DriverName.MaximumLength = GdiDriverInfo.DriverName.MaximumLength;
 			DriverInfo->DriverName.Length = GdiDriverInfo.DriverName.Length;
 			DriverInfo->DriverName.Buffer = ExAllocatePool(PagedPool,
GdiDriverInfo.DriverName.MaximumLength);
 			RtlCopyUnicodeString(&DriverInfo->DriverName, &GdiDriverInfo.DriverName);
-			DriverInfo->ImageHandle = hImageHandle;
+			DriverInfo->SectionPointer = GdiDriverInfo.SectionPointer;
 			InsertHeadList(&GlobalDriverListHead, &DriverInfo->ListEntry);
 		}
 	}
 
-	return hImageHandle;
+	return DriverInfo;
 }
 
 VOID
@@ -248,11 +247,12 @@
 EngUnloadImage ( IN HANDLE hModule )
 {
   NTSTATUS Status;
+  PDRIVERS DriverInfo = (PDRIVERS)hModule;
 
   DPRINT("hModule 0x%x\n", hModule);
 
   Status = ZwSetSystemInformation(SystemUnloadGdiDriverInformation,
-    &hModule, sizeof(HANDLE));
+    DriverInfo->SectionPointer, sizeof(PVOID));
 
   if(!NT_SUCCESS(Status))
   {
@@ -261,27 +261,9 @@
   }
   else
   {
-	  /* remove from the list */
-	  if( !IsListEmpty(&GlobalDriverListHead) )
-	  {
-		  PLIST_ENTRY CurrentEntry = GlobalDriverListHead.Flink;
-		  PDRIVERS Current;
-		  /* probably the driver was already loaded, let's try to find it out */
-		  while( CurrentEntry != &GlobalDriverListHead )
-		  {
-			  Current = CONTAINING_RECORD(CurrentEntry, DRIVERS, ListEntry);
-
-			  if( Current ) {
-				  if(Current->ImageHandle == hModule) {
-					  ExFreePool(Current->DriverName.Buffer);
-					  RemoveEntryList(&Current->ListEntry);
-					  ExFreePool(Current);
-					  break;
-				  }
-			  }
-			  CurrentEntry = CurrentEntry->Flink;
-		  };
-	  }
+    ExFreePool(DriverInfo->DriverName.Buffer);
+    RemoveEntryList(&DriverInfo->ListEntry);
+    ExFreePool(DriverInfo);
   }
 }