Re: [ros-dev] [ros-diffs] [tkreuzer] 64589: [NTOSKRNL] Implement MiInsertVadEx, replacing duplicated code from NtAllocateVirtualMemory and MiMapViewOfDataSection

[Alex Ionescu]

I think it would be really great that instead of combining functions
together for "removing code duplication" and not even bothering to think
why there's a reason why the code was duplicated, people would work on
actual improvements like Jerome or Pierre, instead of re-factoring code
that works just to make it 'simpler'. Especially since this introduced a
few bugs which had to be later fixed. There's a million bugs in ReactOS,
including in the memory manager, so fixing those seems to make more sense
than refactoring.

Just my 2 cents.

Best regards,
Alex Ionescu

On Tue, Oct 7, 2014 at 5:31 PM, [e-mail] wrote:
> Author: tkreuzer
> Date: Wed Oct  8 00:31:17 2014
> New Revision: 64589
>
> URL: http://svn.reactos.org/svn/reactos?rev=64589&view=rev
> Log:
> [NTOSKRNL]
> Implement MiInsertVadEx, replacing duplicated code from
> NtAllocateVirtualMemory and MiMapViewOfDataSection
>
> Modified:
>     trunk/reactos/ntoskrnl/mm/ARM3/miarm.h
>     trunk/reactos/ntoskrnl/mm/ARM3/section.c
>     trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c
>     trunk/reactos/ntoskrnl/mm/ARM3/virtual.c
>
> Modified: trunk/reactos/ntoskrnl/mm/ARM3/miarm.h
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/miarm.h?re...
>
> ==============================================================================
> --- trunk/reactos/ntoskrnl/mm/ARM3/miarm.h      [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/mm/ARM3/miarm.h      [iso-8859-1] Wed Oct  8
> 00:31:17 2014
> @@ -2144,6 +2144,16 @@
>      IN PEPROCESS Process
>  );
>
> +NTSTATUS
> +NTAPI
> +MiInsertVadEx(
> _Inout_ PMMVAD Vad,
> _In_ ULONG_PTR *BaseAddress,
> _In_ SIZE_T ViewSize,
> _In_ ULONG_PTR HighestAddress,
> _In_ ULONG_PTR Alignment,
> _In_ ULONG AllocationType);
>
> VOID
>  NTAPI
>  MiInsertBasedSection(
>
> Modified: trunk/reactos/ntoskrnl/mm/ARM3/section.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/section.c?...
>
> ==============================================================================
> --- trunk/reactos/ntoskrnl/mm/ARM3/section.c    [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/mm/ARM3/section.c    [iso-8859-1] Wed Oct  8
> 00:31:17 2014
> @@ -1252,9 +1252,7 @@
>                         IN ULONG AllocationType)
>  {
>      PMMVAD_LONG Vad;
> PETHREAD Thread = PsGetCurrentThread();
> PMMSUPPORT AddressSpace;
> ULONG_PTR StartAddress, EndingAddress;
> ULONG_PTR StartAddress;
> ULONG_PTR ViewSizeInPages;
> PSUBSECTION Subsection;
> PSEGMENT Segment;
>
> @@ -1263,8 +1261,6 @@
>      ULONG QuotaCharge = 0, QuotaExcess = 0;
>      PMMPTE PointerPte, LastPte;
>      MMPTE TempPte;
>
> PMMADDRESS_NODE Parent;
>
> TABLE_SEARCH_RESULT Result;
> DPRINT("Mapping ARM3 data section\n");
>
> /* Get the segment for this section */
>
> @@ -1361,6 +1357,7 @@
>  /* Write all the data required in the VAD for handling a fault */
>  Vad->ControlArea = ControlArea;
> Vad->u.VadFlags.CommitCharge = 0;
> Vad->u.VadFlags.Protection = ProtectionMask;
> Vad->u2.VadFlags2.FileOffset = (ULONG)(SectionOffset->QuadPart >> 16);
> Vad->u2.VadFlags2.Inherit = (InheritDisposition == ViewShare);
>
> @@ -1437,100 +1434,23 @@
>          StartAddress = 0;
>      }
> /* Lock the address space and make sure the process is alive */
> AddressSpace = MmGetCurrentAddressSpace();
> MmLockAddressSpace(AddressSpace);
> if (Process->VmDeleted)
> {
>    MmUnlockAddressSpace(AddressSpace);
>    ExFreePoolWithTag(Vad, 'ldaV');
>    DPRINT1("The process is dying\n");
>    return STATUS_PROCESS_IS_TERMINATING;
> }
> /* Did the caller specify an address? */
> if (StartAddress == 0)
> {
>    /* ARM3 does not support these flags yet */
>    ASSERT(Process->VmTopDown == 0);
>    ASSERT(ZeroBits == 0);
>    /* Which way should we search? */
>    if (AllocationType & MEM_TOP_DOWN)
>    {
>        /* No, find an address top-down */
>        Result = MiFindEmptyAddressRangeDownTree(*ViewSize,
>
> (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS,
>                                                 _64K,
>                                                 &Process->VadRoot,
>                                                 &StartAddress,
>                                                 &Parent);
>    }
>    else
>    {
>        /* No, find an address bottom-up */
>        Result = MiFindEmptyAddressRangeInTree(*ViewSize,
>                                               _64K,
>                                               &Process->VadRoot,
>                                               &Parent,
>                                               &StartAddress);
>    }
>    /* Check if we found a suitable location */
>    if (Result == TableFoundNode)
>    {
>        DPRINT1("Not enough free space to insert this section!\n");
>        MmUnlockAddressSpace(AddressSpace);
>        MiDereferenceControlArea(ControlArea);
>        ExFreePoolWithTag(Vad, 'ldaV');
>        return STATUS_CONFLICTING_ADDRESSES;
>    }
>    /* Get the ending address, which is the last piece we need for
>
> the VAD */
>    EndingAddress = StartAddress + (ViewSizeInPages * PAGE_SIZE) - 1;
> }
> else
> {
>    /* Get the ending address, which is the last piece we need for
>
> the VAD */
>    EndingAddress = StartAddress + (ViewSizeInPages * PAGE_SIZE)  - 1;
>    /* Make sure it doesn't conflict with an existing allocation */
>    Result = MiCheckForConflictingNode(StartAddress >> PAGE_SHIFT,
>                                       EndingAddress >> PAGE_SHIFT,
>                                       &Process->VadRoot,
>                                       &Parent);
>    if (Result == TableFoundNode)
>    {
>        DPRINT1("Conflict with SEC_BASED or manually based
>
> section!\n");
>        MmUnlockAddressSpace(AddressSpace);
>        MiDereferenceControlArea(ControlArea);
>        ExFreePoolWithTag(Vad, 'ldaV');
>        return STATUS_CONFLICTING_ADDRESSES;
>    }
> }
> /* Now set the VAD address */
> Vad->StartingVpn = StartAddress >> PAGE_SHIFT;
> Vad->EndingVpn = EndingAddress >> PAGE_SHIFT;
> /* Pretend as if we own the working set */
> MiLockProcessWorkingSetUnsafe(Process, Thread);
> /* Insert the VAD */
> Process->VadRoot.NodeHint = Vad;
> MiInsertNode(&Process->VadRoot, (PVOID)Vad, Parent, Result);
> /* Release the working set */
> MiUnlockProcessWorkingSetUnsafe(Process, Thread);
> /* Unlock the address space */
> MmUnlockAddressSpace(AddressSpace);
>
> Status = MiInsertVadEx((PMMVAD)Vad,
>                       &StartAddress,
>                       ViewSizeInPages * PAGE_SIZE,
>                       MAXULONG_PTR >> ZeroBits,
>                       MM_VIRTMEM_GRANULARITY,
>                       AllocationType);
>
> if (!NT_SUCCESS(Status))
>
> {
>    return Status;
>
> }
>
> /* Windows stores this for accounting purposes, do so as well */
> if (!Segment->u2.FirstMappedVa) Segment->u2.FirstMappedVa =
>
> (PVOID)StartAddress;
>  /* Finally, let the caller know where, and for what size, the view
>
> was mapped */
> *ViewSize = (ULONG_PTR)EndingAddress - (ULONG_PTR)StartAddress + 1;
> *ViewSize = ViewSizeInPages * PAGE_SIZE;
> *BaseAddress = (PVOID)StartAddress;
> DPRINT("Start and region: 0x%p, 0x%p\n", *BaseAddress, *ViewSize);
> return STATUS_SUCCESS;
>
> Modified: trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c?...
>
> ==============================================================================
> --- trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c    [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/mm/ARM3/vadnode.c    [iso-8859-1] Wed Oct  8
> 00:31:17 2014
> @@ -193,6 +193,138 @@
>      MiInsertNode(&Process->VadRoot, (PVOID)Vad, Parent, Result);
>  }
>
> +NTSTATUS
> +NTAPI
> +MiInsertVadEx(
> _Inout_ PMMVAD Vad,
> _In_ ULONG_PTR *BaseAddress,
> _In_ SIZE_T ViewSize,
> _In_ ULONG_PTR HighestAddress,
> _In_ ULONG_PTR Alignment,
> _In_ ULONG AllocationType)
>
> +{
> ULONG_PTR StartingAddress, EndingAddress;
> PEPROCESS CurrentProcess;
> PETHREAD CurrentThread;
> TABLE_SEARCH_RESULT Result;
> PMMADDRESS_NODE Parent;
> /* Align the view size to pages */
> ViewSize = ALIGN_UP_BY(ViewSize, PAGE_SIZE);
> /* Get the current process */
> CurrentProcess = PsGetCurrentProcess();
> /* Acquire the address creation lock and make sure the process is
>
> alive */
> KeAcquireGuardedMutex(&CurrentProcess->AddressCreationLock);
> if (CurrentProcess->VmDeleted)
> {
>    KeReleaseGuardedMutex(&CurrentProcess->AddressCreationLock);
>    DPRINT1("The process is dying\n");
>    return STATUS_PROCESS_IS_TERMINATING;
> }
> /* Did the caller specify an address? */
> if (*BaseAddress == 0)
> {
>    /* Make sure HighestAddress is not too large */
>    HighestAddress = min(HighestAddress,
>
> (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS);
>    /* Which way should we search? */
>    if ((AllocationType & MEM_TOP_DOWN) || CurrentProcess->VmTopDown)
>    {
>        /* Find an address top-down */
>        Result = MiFindEmptyAddressRangeDownTree(ViewSize,
>                                                 HighestAddress,
>                                                 Alignment,
>
> &CurrentProcess->VadRoot,
>                                                 &StartingAddress,
>                                                 &Parent);
>    }
>    else
>    {
>        /* Find an address bottom-up */
>        Result = MiFindEmptyAddressRangeInTree(ViewSize,
>                                               Alignment,
>
> &CurrentProcess->VadRoot,
>                                               &Parent,
>                                               &StartingAddress);
>    }
>    /* Get the ending address, which is the last piece we need for
>
> the VAD */
>    EndingAddress = StartingAddress + ViewSize - 1;
>    /* Check if we found a suitable location */
>    if ((Result == TableFoundNode) || (EndingAddress >
>
> HighestAddress))
>    {
>        DPRINT1("Not enough free space to insert this VAD node!\n");
>        KeReleaseGuardedMutex(&CurrentProcess->AddressCreationLock);
>        return STATUS_CONFLICTING_ADDRESSES;
>    }
>    ASSERT(StartingAddress != 0);
>    ASSERT(StartingAddress < (ULONG_PTR)HighestAddress);
>    ASSERT(EndingAddress > StartingAddress);
> }
> else
> {
>    /* Calculate the starting and ending address */
>    StartingAddress = ALIGN_DOWN_BY(*BaseAddress, Alignment);
>    EndingAddress = StartingAddress + ViewSize - 1;
>    /* Make sure it doesn't conflict with an existing allocation */
>    Result = MiCheckForConflictingNode(StartingAddress >> PAGE_SHIFT,
>                                       EndingAddress >> PAGE_SHIFT,
>                                       &CurrentProcess->VadRoot,
>                                       &Parent);
>    if (Result == TableFoundNode)
>    {
>        DPRINT1("Given address conflicts with existing node\n");
>        KeReleaseGuardedMutex(&CurrentProcess->AddressCreationLock);
>        return STATUS_CONFLICTING_ADDRESSES;
>    }
> }
> /* Now set the VAD address */
> Vad->StartingVpn = StartingAddress >> PAGE_SHIFT;
> Vad->EndingVpn = EndingAddress >> PAGE_SHIFT;
> /* Check if the VAD is to be secured */
> if (Vad->u2.VadFlags2.OneSecured)
> {
>    /* This *must* be a long VAD! */
>    ASSERT(Vad->u2.VadFlags2.LongVad);
>    /* Yeah this is retarded, I didn't invent it! */
>    ((PMMVAD_LONG)Vad)->u3.Secured.StartVpn = StartingAddress;
>    ((PMMVAD_LONG)Vad)->u3.Secured.EndVpn = EndingAddress;
> }
> /* Lock the working set */
> CurrentThread = PsGetCurrentThread();
> MiLockProcessWorkingSetUnsafe(CurrentProcess, CurrentThread);
> /* Insert the VAD */
> CurrentProcess->VadRoot.NodeHint = Vad;
> MiInsertNode(&CurrentProcess->VadRoot, (PVOID)Vad, Parent, Result);
> /* Release the working set */
> MiUnlockProcessWorkingSetUnsafe(CurrentProcess, CurrentThread);
> /* Update the process' virtual size, and peak virtual size */
> CurrentProcess->VirtualSize += ViewSize;
> if (CurrentProcess->VirtualSize > CurrentProcess->PeakVirtualSize)
> {
>    CurrentProcess->PeakVirtualSize = CurrentProcess->VirtualSize;
> }
> /* Unlock the address space */
> KeReleaseGuardedMutex(&CurrentProcess->AddressCreationLock);
> *BaseAddress = StartingAddress;
> return STATUS_SUCCESS;
>
> +}
>
> VOID
>  NTAPI
>  MiInsertBasedSection(IN PSECTION Section)
>
> Modified: trunk/reactos/ntoskrnl/mm/ARM3/virtual.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/virtual.c?...
>
> ==============================================================================
> --- trunk/reactos/ntoskrnl/mm/ARM3/virtual.c    [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/mm/ARM3/virtual.c    [iso-8859-1] Wed Oct  8
> 00:31:17 2014
> @@ -4304,12 +4304,12 @@
>  {
>      PEPROCESS Process;
>      PMEMORY_AREA MemoryArea;
> PFN_NUMBER PageCount;
> PMMVAD Vad = NULL, FoundVad;
> NTSTATUS Status;
> PMMSUPPORT AddressSpace;
> PVOID PBaseAddress;
> ULONG_PTR PRegionSize, StartingAddress, EndingAddress, HighestAddress;
> ULONG_PTR PRegionSize, StartingAddress, EndingAddress;
> ULONG_PTR HighestAddress = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS;
> PEPROCESS CurrentProcess = PsGetCurrentProcess();
> KPROCESSOR_MODE PreviousMode = KeGetPreviousMode();
> PETHREAD CurrentThread = PsGetCurrentThread();
>
> @@ -4319,7 +4319,6 @@
>      MMPTE TempPte;
>      PMMPTE PointerPte, PointerPde, LastPte;
>      TABLE_SEARCH_RESULT Result;
>
> PMMADDRESS_NODE Parent;
> PAGED_CODE();
>
> /* Check for valid Zero bits */
>
> @@ -4544,7 +4543,6 @@
>              // This is a blind commit, all we need is the region size
>              //
>              PRegionSize = ROUND_TO_PAGES(PRegionSize);
>        PageCount = BYTES_TO_PAGES(PRegionSize);
>        EndingAddress = 0;
>        StartingAddress = 0;
>
> @@ -4563,13 +4561,6 @@
>                      goto FailPathNoLock;
>                  }
>              }
>        else
>        {
>            //
>            // Use the highest VAD address as maximum
>            //
>            HighestAddress = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS;
>        }
>    }
>    else
>    {
>
> @@ -4579,8 +4570,8 @@
>              // fall based on the aligned address and the passed in region
> size
>              //
>              EndingAddress = ((ULONG_PTR)PBaseAddress + PRegionSize - 1) |
> (PAGE_SIZE - 1);
>        StartingAddress = ROUND_DOWN((ULONG_PTR)PBaseAddress, _64K);
>        PageCount = BYTES_TO_PAGES(EndingAddress - StartingAddress);
>        PRegionSize = EndingAddress + 1 -
>
> ROUND_DOWN((ULONG_PTR)PBaseAddress, _64K);
>        StartingAddress = (ULONG_PTR)PBaseAddress;
>    }
>
>    //
>
> @@ -4594,7 +4585,7 @@
>              goto FailPathNoLock;
>          }
>    Vad->u.LongFlags = 0;
>    RtlZeroMemory(Vad, sizeof(MMVAD_LONG));
>    if (AllocationType & MEM_COMMIT) Vad->u.VadFlags.MemCommit = 1;
>    Vad->u.VadFlags.Protection = ProtectionMask;
>    Vad->u.VadFlags.PrivateMemory = 1;
>
> @@ -4602,124 +4593,24 @@
>          Vad->ControlArea = NULL; // For Memory-Area hack
>      //
>    // Lock the address space and make sure the process isn't already
>
> dead
>    //
>    AddressSpace = MmGetCurrentAddressSpace();
>    MmLockAddressSpace(AddressSpace);
>    if (Process->VmDeleted)
>    {
>        Status = STATUS_PROCESS_IS_TERMINATING;
>        goto FailPath;
>    }
>    //
>    // Did we have a base address? If no, find a valid address that
>
> is 64KB
>    // aligned in the VAD tree. Otherwise, make sure that the address
>
> range
>    // which was passed in isn't already conflicting with an existing
>
> address
>    // range.
>    //
>    if (!PBaseAddress)
>    {
>        /* Which way should we search? */
>        if ((AllocationType & MEM_TOP_DOWN) || Process->VmTopDown)
>        {
>            /* Find an address top-down */
>            Result = MiFindEmptyAddressRangeDownTree(PRegionSize,
>                                                     HighestAddress,
>                                                     _64K,
>
> &Process->VadRoot,
>                                                     &StartingAddress,
>                                                     &Parent);
>        }
>        else
>        {
>            /* Find an address bottom-up */
>            Result = MiFindEmptyAddressRangeInTree(PRegionSize,
>                                                   _64K,
>                                                   &Process->VadRoot,
>                                                   &Parent,
>                                                   &StartingAddress);
>        }
>        if (Result == TableFoundNode)
>        {
>            Status = STATUS_NO_MEMORY;
>            goto FailPath;
>        }
>        //
>        // Now we know where the allocation ends. Make sure it
>
> doesn't end up
>        // somewhere in kernel mode.
>        //
>        ASSERT(StartingAddress != 0);
>        ASSERT(StartingAddress < (ULONG_PTR)MM_HIGHEST_USER_ADDRESS);
>        EndingAddress = (StartingAddress + PRegionSize - 1) |
>
> (PAGE_SIZE - 1);
>        ASSERT(EndingAddress > StartingAddress);
>        if (EndingAddress > HighestAddress)
>        {
>            Status = STATUS_NO_MEMORY;
>            goto FailPath;
>        }
>    }
>    else
>    {
>        /* Make sure it doesn't conflict with an existing allocation
>
> */
>        Result = MiCheckForConflictingNode(StartingAddress >>
>
> PAGE_SHIFT,
>                                           EndingAddress >>
>
> PAGE_SHIFT,
>                                           &Process->VadRoot,
>                                           &Parent);
>        if (Result == TableFoundNode)
>        {
>            //
>            // The address specified is in conflict!
>            //
>            Status = STATUS_CONFLICTING_ADDRESSES;
>            goto FailPath;
>        }
>    }
>    //
>    // Write out the VAD fields for this allocation
>    //
>    Vad->StartingVpn = StartingAddress >> PAGE_SHIFT;
>    Vad->EndingVpn = EndingAddress >> PAGE_SHIFT;
>    //
>    // FIXME: Should setup VAD bitmap
>    //
>    Status = STATUS_SUCCESS;
>    //
>    // Lock the working set and insert the VAD into the process VAD
>
> tree
>    //
>    MiLockProcessWorkingSetUnsafe(Process, CurrentThread);
>    Process->VadRoot.NodeHint = Vad;
>    MiInsertNode(&Process->VadRoot, (PVOID)Vad, Parent, Result);
>    MiUnlockProcessWorkingSetUnsafe(Process, CurrentThread);
>    //
>    // Make sure the actual region size is at least as big as the
>    // requested region size and update the value
>    //
>    ASSERT(PRegionSize <= (EndingAddress + 1 - StartingAddress));
>    PRegionSize = (EndingAddress + 1 - StartingAddress);
>    //
>    // Update the virtual size of the process, and if this is now the
>
> highest
>    // virtual size we have ever seen, update the peak virtual size
>
> to reflect
>    // this.
>    //
>    Process->VirtualSize += PRegionSize;
>    if (Process->VirtualSize > Process->PeakVirtualSize)
>    {
>        Process->PeakVirtualSize = Process->VirtualSize;
>    }
>    //
>    // Release address space and detach and dereference the target
>
> process if
>    // Insert the VAD
>    //
>    Status = MiInsertVadEx(Vad,
>                           &StartingAddress,
>                           PRegionSize,
>                           HighestAddress,
>                           MM_VIRTMEM_GRANULARITY,
>                           AllocationType);
>    if (!NT_SUCCESS(Status))
>    {
>        DPRINT1("Failed to insert the VAD!\n");
>        goto FailPathNoLock;
>    }
>    //
>    // Detach and dereference the target process if
>    // it was different from the current process
>    //
>    MmUnlockAddressSpace(AddressSpace);
>    if (Attached) KeUnstackDetachProcess(&ApcState);
>    if (ProcessHandle != NtCurrentProcess())
>
> ObDereferenceObject(Process);