[ros-diffs] [jgardou] 72989: [NTOS/MM] Miscellaneous fixes for legacy Mm section implementation - Always allocate a private page for IMAGE_SCN_CNT_UNINITIALIZED_DATA - Make sure a shared page is present before...

Author: jgardou
Date: Tue Oct 18 20:01:18 2016
New Revision: 72989
URL: http://svn.reactos.org/svn/reactos?rev=72989&view=rev
Log:
[NTOS/MM]
Miscellaneous fixes for legacy Mm section implementation
 - Always allocate a private page for IMAGE_SCN_CNT_UNINITIALIZED_DATA
 - Make sure a shared page is present before writing on a COW mapping and making a private
copy.
 - Fix a race condition : when paging out a file section, old Mm lists all of the process
maps, removing them one after the other and lowering the page reference count in the
process. Sometimes a page fault occur in the process, the mapping is added, but the page
refcount is not bumped because it requires locking the corresponding segment. Manage page
refcount under segment lock.
CORE-12047
Modified:
    trunk/reactos/ntoskrnl/mm/section.c
Modified: trunk/reactos/ntoskrnl/mm/section.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/section.c?rev=…
==============================================================================
--- trunk/reactos/ntoskrnl/mm/section.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/section.c [iso-8859-1] Tue Oct 18 20:01:18 2016
@@ -1366,39 +1366,45 @@
     HasSwapEntry = MmIsPageSwapEntry(Process, Address);
-    if (HasSwapEntry)
+    /* See if we should use a private page */
+    if ((HasSwapEntry) || (Segment->Image.Characteristics &
IMAGE_SCN_CNT_UNINITIALIZED_DATA))
     {
         SWAPENTRY DummyEntry;
         /*
          * Is it a wait entry?
          */
-        MmGetPageFileMapping(Process, Address, &SwapEntry);
-
-        if (SwapEntry == MM_WAIT_ENTRY)
-        {
-            MmUnlockSectionSegment(Segment);
-            MmUnlockAddressSpace(AddressSpace);
-            MiWaitForPageEvent(NULL, NULL);
-            MmLockAddressSpace(AddressSpace);
-            return STATUS_MM_RESTART_OPERATION;
-        }
-
-        /*
-         * Must be private page we have swapped out.
-         */
-
-        /*
-         * Sanity check
-         */
-        if (Segment->Flags & MM_PAGEFILE_SEGMENT)
-        {
-            DPRINT1("Found a swaped out private page in a pagefile
section.\n");
-            KeBugCheck(MEMORY_MANAGEMENT);
+        if (HasSwapEntry)
+        {
+            MmGetPageFileMapping(Process, Address, &SwapEntry);
+
+            if (SwapEntry == MM_WAIT_ENTRY)
+            {
+                MmUnlockSectionSegment(Segment);
+                MmUnlockAddressSpace(AddressSpace);
+                MiWaitForPageEvent(NULL, NULL);
+                MmLockAddressSpace(AddressSpace);
+                return STATUS_MM_RESTART_OPERATION;
+            }
+
+            /*
+             * Must be private page we have swapped out.
+             */
+
+            /*
+             * Sanity check
+             */
+            if (Segment->Flags & MM_PAGEFILE_SEGMENT)
+            {
+                DPRINT1("Found a swaped out private page in a pagefile
section.\n");
+                KeBugCheck(MEMORY_MANAGEMENT);
+            }
+            MmDeletePageFileMapping(Process, Address, &SwapEntry);
         }
         MmUnlockSectionSegment(Segment);
-        MmDeletePageFileMapping(Process, Address, &SwapEntry);
+
+        /* Tell everyone else we are serving the fault. */
         MmCreatePageFileMapping(Process, Address, MM_WAIT_ENTRY);
         MmUnlockAddressSpace(AddressSpace);
@@ -1411,12 +1417,16 @@
             KeBugCheck(MEMORY_MANAGEMENT);
         }
-        Status = MmReadFromSwapPage(SwapEntry, Page);
-        if (!NT_SUCCESS(Status))
-        {
-            DPRINT1("MmReadFromSwapPage failed, status = %x\n", Status);
-            KeBugCheck(MEMORY_MANAGEMENT);
-        }
+        if (HasSwapEntry)
+        {
+            Status = MmReadFromSwapPage(SwapEntry, Page);
+            if (!NT_SUCCESS(Status))
+            {
+                DPRINT1("MmReadFromSwapPage failed, status = %x\n", Status);
+                KeBugCheck(MEMORY_MANAGEMENT);
+            }
+        }
+
         MmLockAddressSpace(AddressSpace);
         MmDeletePageFileMapping(Process, PAddress, &DummyEntry);
         Status = MmCreateVirtualMapping(Process,
@@ -1434,7 +1444,8 @@
         /*
          * Store the swap entry for later use.
          */
-        MmSetSavedSwapEntryPage(Page, SwapEntry);
+        if (HasSwapEntry)
+            MmSetSavedSwapEntryPage(Page, SwapEntry);
         /*
          * Add the page to the process's working set
@@ -1536,15 +1547,9 @@
             return(Status);
         }
-        /*
-         * Mark the offset within the section as having valid, in-memory
-         * data
-         */
+        /* Lock both segment and process address space while we proceed. */
         MmLockAddressSpace(AddressSpace);
         MmLockSectionSegment(Segment);
-        Entry = MAKE_SSE(Page << PAGE_SHIFT, 1);
-        MmSetPageEntrySectionSegment(Segment, &Offset, Entry);
-        MmUnlockSectionSegment(Segment);
         MmDeletePageFileMapping(Process, PAddress, &FakeSwapEntry);
         DPRINT("CreateVirtualMapping Page %x Process %p PAddress %p Attributes
%x\n",
@@ -1562,6 +1567,11 @@
         ASSERT(MmIsPagePresent(Process, PAddress));
         MmInsertRmap(Page, Process, Address);
+        /* Set this section offset has being backed by our new page. */
+        Entry = MAKE_SSE(Page << PAGE_SHIFT, 1);
+        MmSetPageEntrySectionSegment(Segment, &Offset, Entry);
+        MmUnlockSectionSegment(Segment);
+
         MiSetPageEvent(Process, Address);
         DPRINT("Address 0x%p\n", Address);
         return(STATUS_SUCCESS);
@@ -1571,6 +1581,16 @@
         SWAPENTRY SwapEntry;
         SwapEntry = SWAPENTRY_FROM_SSE(Entry);
+
+        /* See if a page op is running on this segment. */
+        if (SwapEntry == MM_WAIT_ENTRY)
+        {
+            MmUnlockSectionSegment(Segment);
+            MmUnlockAddressSpace(AddressSpace);
+            MiWaitForPageEvent(NULL, NULL);
+            MmLockAddressSpace(AddressSpace);
+            return STATUS_MM_RESTART_OPERATION;
+        }
         /*
         * Release all our locks and read in the page from disk
@@ -1609,6 +1629,24 @@
             DPRINT1("Someone changed ppte entry while we slept (%x vs %x)\n",
Entry, Entry1);
             KeBugCheck(MEMORY_MANAGEMENT);
         }
+
+        /*
+         * Save the swap entry.
+         */
+        MmSetSavedSwapEntryPage(Page, SwapEntry);
+
+        /* Map the page into the process address space */
+        Status = MmCreateVirtualMapping(Process,
+                                        PAddress,
+                                        Region->Protect,
+                                        &Page,
+                                        1);
+        if (!NT_SUCCESS(Status))
+        {
+            DPRINT1("Unable to create virtual mapping\n");
+            KeBugCheck(MEMORY_MANAGEMENT);
+        }
+        MmInsertRmap(Page, Process, Address);
         /*
          * Mark the offset within the section as having valid, in-memory
@@ -1618,36 +1656,14 @@
         MmSetPageEntrySectionSegment(Segment, &Offset, Entry);
         MmUnlockSectionSegment(Segment);
-        /*
-         * Save the swap entry.
-         */
-        MmSetSavedSwapEntryPage(Page, SwapEntry);
-        Status = MmCreateVirtualMapping(Process,
-                                        PAddress,
-                                        Region->Protect,
-                                        &Page,
-                                        1);
-        if (!NT_SUCCESS(Status))
-        {
-            DPRINT1("Unable to create virtual mapping\n");
-            KeBugCheck(MEMORY_MANAGEMENT);
-        }
-        MmInsertRmap(Page, Process, Address);
         MiSetPageEvent(Process, Address);
         DPRINT("Address 0x%p\n", Address);
         return(STATUS_SUCCESS);
     }
     else
     {
-        /*
-         * If the section offset is already in-memory and valid then just
-         * take another reference to the page
-         */
-
+        /* We already have a page on this section offset. Map it into the process address
space. */
         Page = PFN_FROM_SSE(Entry);
-
-        MmSharePageEntrySectionSegment(Segment, &Offset);
-        MmUnlockSectionSegment(Segment);
         Status = MmCreateVirtualMapping(Process,
                                         PAddress,
@@ -1660,6 +1676,11 @@
             KeBugCheck(MEMORY_MANAGEMENT);
         }
         MmInsertRmap(Page, Process, Address);
+
+        /* Take a reference on it */
+        MmSharePageEntrySectionSegment(Segment, &Offset);
+        MmUnlockSectionSegment(Segment);
+
         MiSetPageEvent(Process, Address);
         DPRINT("Address 0x%p\n", Address);
         return(STATUS_SUCCESS);
@@ -1682,9 +1703,16 @@
     PMM_REGION Region;
     ULONG_PTR Entry;
     PEPROCESS Process = MmGetAddressSpaceOwner(AddressSpace);
-    SWAPENTRY SwapEntry;
     DPRINT("MmAccessFaultSectionView(%p, %p, %p)\n", AddressSpace, MemoryArea,
Address);
+
+    /* Make sure we have a page mapping for this address.  */
+    Status = MmNotPresentFaultSectionView(AddressSpace, MemoryArea, Address, TRUE);
+    if (!NT_SUCCESS(Status))
+    {
+        /* This is invalid access ! */
+        return Status;
+    }
     /*
      * Check if the page has already been set readwrite
@@ -1708,15 +1736,6 @@
                           &MemoryArea->Data.SectionData.RegionListHead,
                           Address, NULL);
     ASSERT(Region != NULL);
-    /*
-     * Lock the segment
-     */
-    MmLockSectionSegment(Segment);
-
-    OldPage = MmGetPfnForProcess(Process, Address);
-    Entry = MmGetPageEntrySectionSegment(Segment, &Offset);
-
-    MmUnlockSectionSegment(Segment);
     /*
      * Check if we are doing COW
@@ -1729,48 +1748,23 @@
         return(STATUS_ACCESS_VIOLATION);
     }
+    /* Get the page mapping this section offset. */
+    MmLockSectionSegment(Segment);
+    Entry = MmGetPageEntrySectionSegment(Segment, &Offset);
+
+    /* Get the current page mapping for the process */
+    ASSERT(MmIsPagePresent(Process, PAddress));
+    OldPage = MmGetPfnForProcess(Process, PAddress);
+    ASSERT(OldPage != 0);
+
     if (IS_SWAP_FROM_SSE(Entry) ||
             PFN_FROM_SSE(Entry) != OldPage)
     {
+        MmUnlockSectionSegment(Segment);
         /* This is a private page. We must only change the page protection. */
-        MmSetPageProtect(Process, Address, Region->Protect);
+        MmSetPageProtect(Process, PAddress, Region->Protect);
         return(STATUS_SUCCESS);
     }
-
-    if(OldPage == 0)
-        DPRINT("OldPage == 0!\n");
-
-    /*
-     * Get or create a pageop
-     */
-    MmLockSectionSegment(Segment);
-    Entry = MmGetPageEntrySectionSegment(Segment, &Offset);
-
-    /*
-     * Wait for any other operations to complete
-     */
-    if (Entry == SWAPENTRY_FROM_SSE(MM_WAIT_ENTRY))
-    {
-        MmUnlockSectionSegment(Segment);
-        MmUnlockAddressSpace(AddressSpace);
-        MiWaitForPageEvent(NULL, NULL);
-        /*
-         * Restart the operation
-         */
-        MmLockAddressSpace(AddressSpace);
-        DPRINT("Address 0x%p\n", Address);
-        return(STATUS_MM_RESTART_OPERATION);
-    }
-
-    MmDeleteRmap(OldPage, Process, PAddress);
-    MmDeleteVirtualMapping(Process, PAddress, NULL, NULL);
-    MmCreatePageFileMapping(Process, PAddress, MM_WAIT_ENTRY);
-
-    /*
-     * Release locks now we have the pageop
-     */
-    MmUnlockSectionSegment(Segment);
-    MmUnlockAddressSpace(AddressSpace);
     /*
      * Allocate a page
@@ -1789,12 +1783,18 @@
      */
     MiCopyFromUserPage(NewPage, OldPage);
-    MmLockAddressSpace(AddressSpace);
+    /*
+     * Unshare the old page.
+     */
+    DPRINT("Swapping page (Old %x New %x)\n", OldPage, NewPage);
+    MmDeleteVirtualMapping(Process, PAddress, NULL, NULL);
+    MmDeleteRmap(OldPage, Process, PAddress);
+    MmUnsharePageEntrySectionSegment(Section, Segment, &Offset, FALSE, FALSE, NULL);
+    MmUnlockSectionSegment(Segment);
     /*
      * Set the PTE to point to the new page
      */
-    MmDeletePageFileMapping(Process, PAddress, &SwapEntry);
     Status = MmCreateVirtualMapping(Process,
                                     PAddress,
                                     Region->Protect,
@@ -1806,15 +1806,7 @@
         KeBugCheck(MEMORY_MANAGEMENT);
         return(Status);
     }
-
-    /*
-     * Unshare the old page.
-     */
-    DPRINT("Swapping page (Old %x New %x)\n", OldPage, NewPage);
     MmInsertRmap(NewPage, Process, PAddress);
-    MmLockSectionSegment(Segment);
-    MmUnsharePageEntrySectionSegment(Section, Segment, &Offset, FALSE, FALSE, NULL);
-    MmUnlockSectionSegment(Segment);
     MiSetPageEvent(Process, Address);
     DPRINT("Address 0x%p\n", Address);
@@ -2147,6 +2139,9 @@
             else
             {
                 ULONG_PTR OldEntry;
+
+                MmLockSectionSegment(Context.Segment);
+
                 /*
                  * For non-private pages if the page wasn't direct mapped then
                  * set it back into the section segment entry so we don't loose
@@ -2163,7 +2158,6 @@
                              Address);
                 // If we got here, the previous entry should have been a wait
                 Entry = MAKE_SSE(Page << PAGE_SHIFT, 1);
-                MmLockSectionSegment(Context.Segment);
                 OldEntry = MmGetPageEntrySectionSegment(Context.Segment,
&Context.Offset);
                 ASSERT(OldEntry == 0 || OldEntry == MAKE_SWAP_SSE(MM_WAIT_ENTRY));
                 MmSetPageEntrySectionSegment(Context.Segment, &Context.Offset,
Entry);
@@ -2202,6 +2196,7 @@
         }
         else
         {
+            MmLockSectionSegment(Context.Segment);
             Status = MmCreateVirtualMapping(Process,
                                             Address,
                                             MemoryArea->Protect,
@@ -2213,6 +2208,7 @@
                          Address);
             Entry = MAKE_SSE(Page << PAGE_SHIFT, 1);
             MmSetPageEntrySectionSegment(Context.Segment, &Context.Offset, Entry);
+            MmUnlockSectionSegment(Context.Segment);
         }
         MmUnlockAddressSpace(AddressSpace);
         MiSetPageEvent(NULL, NULL);