[ros-diffs] [sir_richard] 55938: [NTOS]: A few other fixups to the page fault path: 1) Assert on empty kernel PTE instead of handling it as a bugcheck. Windows ASSERTs too. Also clarify some ASSERTs which Wind...

Author: sir_richard
Date: Wed Feb 29 23:11:21 2012
New Revision: 55938

URL: http://svn.reactos.org/svn/reactos?rev=55938&view=rev
Log:
[NTOS]: A few other fixups to the page fault path:
1) Assert on empty kernel PTE instead of handling it as a bugcheck. Windows ASSERTs too.
Also clarify some ASSERTs which Windows also does versus ASSERTs we are only doing due to
lack of support for said feature.
2) User page fault path can now distinguish between a user-mode PTE fault, and a
kernel-mode fault on a user PDE, both by creating a correct kernel PDE when needed instead
of always creating user PTEs, as well as by only touching the UsedPageTableEntry reference
counting mechanism when a user-address is in play.
3) Related to #2, also recognize when the faulting PTE is actually a PDE in the
self-mapping region -- another scenario when the "user fault" is actually a
kernel fault for a user PDE.
4) Add one more path where a Paged Pool PDE fixup can save the day instead of always
faulting.
5) Finally, related to #2 and #3, handle the MI_IS_PAGE_TABLE_OR_HYPER_ADDRESS scenario
for a User PDE by treating it as a user fault. The code looks deceptively similar but
there are slight differences which require the separate codepaths with some duplicated
code. The magic is in the ordering.
In trunk, these changes should not cause any regressions (let's hope so). On the
internal VAD-based Virtual Memory branch, they now allow booting to 3rd stage and a fully
usable ReactOS environment. MEMORY_AREA_VIRTUAL_MEMORY is gone on that branch. It's
coming.
[NTOS]: Use PAGE_READWRITE as hardcoded protection instead of PAGE_EXECUTE_READWRITE --
the difference is meaningless on ReactOS Mm but actually causes issues on ARM3 with VADs.

Modified:
    trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
    trunk/reactos/ntoskrnl/mm/anonmem.c

Modified: trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/pagfault.…
==============================================================================
--- trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c [iso-8859-1] Wed Feb 29 23:11:21 2012
@@ -614,25 +614,17 @@
     }
 
     //
-    // The PTE must be invalid
+    // The PTE must be invalid but not completely empty. It must also not be a
+    // prototype PTE as that scenario should've been handled above
     //
     ASSERT(TempPte.u.Hard.Valid == 0);
-
-    /* Check if the PTE is completely empty */
-    if (TempPte.u.Long == 0)
-    {
-        /* The address is not from any pageable area! */
-        KeBugCheckEx(PAGE_FAULT_IN_NONPAGED_AREA,
-                     (ULONG_PTR)Address,
-                     StoreInstruction,
-                     (ULONG_PTR)TrapInformation,
-                     2);
-    }
-
-    //
-    // No prototype, transition or page file software PTEs in ARM3 yet
-    //
     ASSERT(TempPte.u.Soft.Prototype == 0);
+    ASSERT(TempPte.u.Long != 0);
+
+    //
+    // No transition or page file software PTEs in ARM3 yet, so this must be a
+    // demand zero page
+    //
     ASSERT(TempPte.u.Soft.Transition == 0);
     ASSERT(TempPte.u.Soft.PageFileHigh == 0);
 
@@ -770,22 +762,12 @@
         /* Get the current thread */
         CurrentThread = PsGetCurrentThread();
 
-        // Check for a fault on the page table or hyperspace
-        if (MI_IS_PAGE_TABLE_OR_HYPER_ADDRESS(Address))
-        {
-            ASSERT(TempPte.u.Long != (MM_READWRITE <<
MM_PTE_SOFTWARE_PROTECTION_BITS));
-            ASSERT(TempPte.u.Soft.Prototype == 0);
-
-            /* Use the process working set */
-            CurrentProcess = (PEPROCESS)CurrentThread->Tcb.ApcState.Process;
-            WorkingSet = &CurrentProcess->Vm;
-        }
-        else
-        {
-            /* Otherwise use the system working set */
-            WorkingSet = &MmSystemCacheWs;
-            CurrentProcess = NULL;
-        }
+        /* Check for a fault on the page table or hyperspace */
+        if (MI_IS_PAGE_TABLE_OR_HYPER_ADDRESS(Address)) goto UserFault;
+
+        /* Use the system working set */
+        WorkingSet = &MmSystemCacheWs;
+        CurrentProcess = NULL;
 
         /* Acquire the working set lock */
         KeRaiseIrql(APC_LEVEL, &LockIrql);
@@ -884,6 +866,7 @@
     }
 
     /* This is a user fault */
+UserFault:
     CurrentThread = PsGetCurrentThread();
     CurrentProcess = (PEPROCESS)CurrentThread->Tcb.ApcState.Process;
 
@@ -935,6 +918,9 @@
     {
         /* Right now, we only handle scenarios where the PDE is totally empty */
         ASSERT(PointerPde->u.Long == 0);
+
+        /* Right now, we expect a valid protection mask on the VAD */
+        ASSERT(ProtectionCode != MM_NOACCESS);
 
         /* And go dispatch the fault on the PDE. This should handle the demand-zero */
 #if MI_TRACE_PFNS
@@ -978,9 +964,16 @@
     ProtoPte = MiCheckVirtualAddress(Address, &ProtectionCode, &Vad);
     if (ProtectionCode == MM_NOACCESS)
     {
-        /* This is a bogus VA */
+#if (_MI_PAGING_LEVELS == 2)
+        /* Could be a page table for paged pool */
+        MiCheckPdeForPagedPool(Address);
+#endif
+        /* Has the code above changed anything -- is this now a valid PTE? */
+        Status = (PointerPte->u.Hard.Valid == 1) ? STATUS_SUCCESS :
STATUS_ACCESS_VIOLATION;
+
+        /* Either this was a bogus VA or we've fixed up a paged pool PDE */
         MiUnlockProcessWorkingSet(CurrentProcess, CurrentThread);
-        return STATUS_ACCESS_VIOLATION;
+        return Status;
     }
 
     /* Check for non-demand zero PTE */
@@ -1007,17 +1000,34 @@
         return Status;
     }
 
-#if (_MI_PAGING_LEVELS == 2)
-    /* Add an additional page table reference */
-    MmWorkingSetList->UsedPageTableEntries[MiGetPdeOffset(Address)]++;
-    ASSERT(MmWorkingSetList->UsedPageTableEntries[MiGetPdeOffset(Address)] <=
PTE_COUNT);
-#endif
+    /*
+     * Check if this is a real user-mode address or actually a kernel-mode
+     * page table for a user mode address
+     */
+    if (Address <= MM_HIGHEST_USER_ADDRESS)
+    {
+        /* Add an additional page table reference */
+        MmWorkingSetList->UsedPageTableEntries[MiGetPdeOffset(Address)]++;
+        ASSERT(MmWorkingSetList->UsedPageTableEntries[MiGetPdeOffset(Address)] <=
PTE_COUNT);
+    }
+
+    /* No guard page support yet */
+    ASSERT((ProtectionCode & MM_DECOMMIT) == 0);
 
     /* Did we get a prototype PTE back? */
     if (!ProtoPte)
     {
-        /* No, create a new PTE. First, write the protection */
-        PointerPte->u.Soft.Protection = ProtectionCode;
+        /* Is this PTE actually part of the PDE-PTE self-mapping directory? */
+        if (PointerPde == MiAddressToPde(PTE_BASE))
+        {
+            /* Then it's really a demand-zero PDE (on behalf of user-mode) */
+            MI_WRITE_INVALID_PTE(PointerPte, DemandZeroPde);
+        }
+        else
+        {
+            /* No, create a new PTE. First, write the protection */
+            PointerPte->u.Soft.Protection = ProtectionCode;
+        }
 
         /* Lock the PFN database since we're going to grab a page */
         OldIrql = KeAcquireQueuedSpinLock(LockQueuePfnLock);
@@ -1052,17 +1062,30 @@
         /* And we're done with the lock */
         KeReleaseQueuedSpinLock(LockQueuePfnLock, OldIrql);
 
-        /* User fault, build a user PTE */
-        MI_MAKE_HARDWARE_PTE_USER(&TempPte,
-                                  PointerPte,
-                                  PointerPte->u.Soft.Protection,
-                                  PageFrameIndex);
+        /* Fault on user PDE, or fault on user PTE? */
+        if (PointerPte <= MiHighestUserPte)
+        {
+            /* User fault, build a user PTE */
+            MI_MAKE_HARDWARE_PTE_USER(&TempPte,
+                                      PointerPte,
+                                      PointerPte->u.Soft.Protection,
+                                      PageFrameIndex);
+        }
+        else
+        {
+            /* This is a user-mode PDE, create a kernel PTE for it */
+            MI_MAKE_HARDWARE_PTE(&TempPte,
+                                 PointerPte,
+                                 PointerPte->u.Soft.Protection,
+                                 PageFrameIndex);
+        }
 
         /* Write the dirty bit for writeable pages */
         if (MI_IS_PAGE_WRITEABLE(&TempPte)) MI_MAKE_DIRTY_PAGE(&TempPte);
 
         /* And now write down the PTE, making the address valid */
         MI_WRITE_VALID_PTE(PointerPte, TempPte);
+        ASSERT(MiGetPfnEntry(PageFrameIndex)->u1.Event == NULL);
 
         /* Demand zero */
         Status = STATUS_PAGE_FAULT_DEMAND_ZERO;

Modified: trunk/reactos/ntoskrnl/mm/anonmem.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/anonmem.c?rev=…
==============================================================================
--- trunk/reactos/ntoskrnl/mm/anonmem.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/anonmem.c [iso-8859-1] Wed Feb 29 23:11:21 2012
@@ -789,9 +789,9 @@
     MmLockAddressSpace(AddressSpace);
 
     //
-    // Force PAGE_EXECUTE_READWRITE for everything, for now
+    // Force PAGE_READWRITE for everything, for now
     //
-    Protect = PAGE_EXECUTE_READWRITE;
+    Protect = PAGE_READWRITE;
 
     if (PBaseAddress != 0)
     {