RE: [PATCH xenvif] Replace uses of MmAllocatePagesForMdlEx in __AllocatePages()...

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of 
> Paul Durrant
> Sent: 16 June 2020 14:03
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Paul Durrant <pdurrant@xxxxxxxxxx>
> Subject: [PATCH xenvif] Replace uses of MmAllocatePagesForMdlEx in 
> __AllocatePages()...
> 
> From: Paul Durrant <pdurrant@xxxxxxxxxx>
> 
> ... again.
> 
> Commit 4f85d004 "Replace uses of MmAllocatePagesForMdlEx in __AllocatePage"
> modified __AllocatePage() (only) to use
> MmAllocateContinguousMemorySpecifyCache() as its source of memory. As stated
> in that commit:
> 
> "Windows appears to have an edge case bug in which zeroing memory using
> MmAllocatePAgesForMdlEx (which in Win 10 1803 happens even if you specify
> MM_DONT_ZERO_ALLOCATION) can cause a BSOD 139 1e."
> 
> That commit was reverted by e8fe14f6 since it was believed that the bug in
> Windows had been fixed. Subsequently, however, the same symptoms have been
> seen with recent updates of Server 2019. The stack is generally of the form:
> 
> nt!KeBugCheckEx
> nt!KiBugCheckDispatch+0x69
> nt!KiFastFailDispatch+0xd0
> nt!KiRaiseSecurityCheckFailure+0x30e
> nt!KiAcquireThreadStateLock+0x11fa90
> nt!KeSetIdealProcessorThreadEx+0xd0
> nt!MiZeroInParallelWorker+0x115016
> nt!MiZeroInParallel+0x11c
> nt!MiInitializeMdlBatchPages+0x2ae
> nt!MiAllocatePagesForMdl+0x192
> nt!MmAllocatePartitionNodePagesForMdlEx+0xc9
> nt!MmAllocatePagesForMdlEx+0x4d
> 
> Hence, this patch re-instates the fix originally put in place by 4f85d004
> but generalizes it to include AllocatePages() rather than just
> AllocatePage().
> 
> Reported-by: Jan Bakuwel <jan.bakuwel@xxxxxxxxx>
> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> ---
> 
> This fix will also be propogated to all other PV drivers.

This is actually failing in xenvbd, trying to allocate 2 pages for the shared 
ring, so I'm going to try the simpler alternative of
continuing to use MmAllocatePagesForMdlEx() but without passing the "don't 
zero" flag.

  Paul

> ---
>  src/xenvif/util.h | 77 +++++++++++++++++++----------------------------
>  1 file changed, 31 insertions(+), 46 deletions(-)
> 
> diff --git a/src/xenvif/util.h b/src/xenvif/util.h
> index 30322d8..339485b 100644
> --- a/src/xenvif/util.h
> +++ b/src/xenvif/util.h
> @@ -171,55 +171,43 @@ __FreePoolWithTag(
> 
>  static FORCEINLINE PMDL
>  __AllocatePages(
> -    IN  ULONG           Count
> +    IN ULONG            Count
>      )
>  {
>      PHYSICAL_ADDRESS    LowAddress;
>      PHYSICAL_ADDRESS    HighAddress;
> -    LARGE_INTEGER       SkipBytes;
> +    PHYSICAL_ADDRESS    Align;
>      SIZE_T              TotalBytes;
>      PMDL                Mdl;
>      PUCHAR              MdlMappedSystemVa;
>      NTSTATUS            status;
> 
> -    LowAddress.QuadPart = 0ull;
> +    ASSERT3U(KeGetCurrentIrql(), <=, DISPATCH_LEVEL);
> +
> +    LowAddress.QuadPart  = 0ull;
>      HighAddress.QuadPart = ~0ull;
> -    SkipBytes.QuadPart = 0ull;
> -    TotalBytes = (SIZE_T)PAGE_SIZE * Count;
> +    Align.QuadPart       = PAGE_SIZE;
> +    TotalBytes           = (SIZE_T)PAGE_SIZE * Count;
> 
> -    Mdl = MmAllocatePagesForMdlEx(LowAddress,
> -                                  HighAddress,
> -                                  SkipBytes,
> -                                  TotalBytes,
> -                                  MmCached,
> -                                  MM_DONT_ZERO_ALLOCATION);
> +    MdlMappedSystemVa = MmAllocateContiguousMemorySpecifyCache(TotalBytes,
> +                                                               LowAddress,
> +                                                               HighAddress,
> +                                                               Align,
> +                                                               MmCached);
> 
>      status = STATUS_NO_MEMORY;
> -    if (Mdl == NULL)
> +    if (MdlMappedSystemVa == NULL)
>          goto fail1;
> 
> -    if (Mdl->ByteCount < TotalBytes)
> +    Mdl = IoAllocateMdl(MdlMappedSystemVa,
> +                        (ULONG)TotalBytes,
> +                        FALSE,
> +                        FALSE,
> +                        NULL);
> +    if (Mdl == NULL)
>          goto fail2;
> 
> -    ASSERT((Mdl->MdlFlags & (MDL_MAPPED_TO_SYSTEM_VA |
> -                             MDL_PARTIAL_HAS_BEEN_MAPPED |
> -                             MDL_PARTIAL |
> -                             MDL_PARENT_MAPPED_SYSTEM_VA |
> -                             MDL_SOURCE_IS_NONPAGED_POOL |
> -                             MDL_IO_SPACE)) == 0);
> -
> -    MdlMappedSystemVa = MmMapLockedPagesSpecifyCache(Mdl,
> -                                                     KernelMode,
> -                                                                          
> MmCached,
> -                                                                          
> NULL,
> -                                                                          
> FALSE,
> -                                                                          
> NormalPagePriority);
> -
> -    status = STATUS_UNSUCCESSFUL;
> -    if (MdlMappedSystemVa == NULL)
> -        goto fail3;
> -
> -    Mdl->StartVa = PAGE_ALIGN(MdlMappedSystemVa);
> +    MmBuildMdlForNonPagedPool(Mdl);
> 
>      ASSERT3U(Mdl->ByteOffset, ==, 0);
>      ASSERT3P(Mdl->StartVa, ==, MdlMappedSystemVa);
> @@ -229,40 +217,37 @@ __AllocatePages(
> 
>      return Mdl;
> 
> -fail3:
> -    Error("fail3\n");
> -
>  fail2:
>      Error("fail2\n");
> 
> -    MmFreePagesFromMdl(Mdl);
> -    ExFreePool(Mdl);
> -
> +    MmFreeContiguousMemory(MdlMappedSystemVa);
>  fail1:
>      Error("fail1 (%08x)\n", status);
> 
>      return NULL;
>  }
> 
> -#define __AllocatePage()    __AllocatePages(1)
> +#define __AllocatePage() __AllocatePages(1)
> 
>  static FORCEINLINE VOID
>  __FreePages(
> -    IN       PMDL    Mdl
> +    IN PMDL Mdl
>      )
>  {
> -    PUCHAR   MdlMappedSystemVa;
> +    PUCHAR  MdlMappedSystemVa;
> +
> +    ASSERT(Mdl->MdlFlags &
> +            (MDL_MAPPED_TO_SYSTEM_VA |
> +             MDL_SOURCE_IS_NONPAGED_POOL));
> 
> -    ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
>      MdlMappedSystemVa = Mdl->MappedSystemVa;
> 
> -    MmUnmapLockedPages(MdlMappedSystemVa, Mdl);
> +    IoFreeMdl(Mdl);
> 
> -    MmFreePagesFromMdl(Mdl);
> -    ExFreePool(Mdl);
> +    MmFreeContiguousMemory(MdlMappedSystemVa);
>  }
> 
> -#define __FreePage(_Mdl)    __FreePages(_Mdl)
> +#define __FreePage(_Mdl) __FreePages(_Mdl)
> 
>  static FORCEINLINE PCHAR
>  __strtok_r(
> --
> 2.17.1
>