Re: [win-pv-devel] [PATCH] Replace uses of MmAllocatePagesForMdlEx in __AllocatePage

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel [mailto:win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On
> Behalf Of Ben Chalmers
> Sent: 29 June 2018 16:24
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Ben Chalmers <ben.chalmers@xxxxxxxxxx>
> Subject: [win-pv-devel] [PATCH] Replace uses of MmAllocatePagesForMdlEx
> in __AllocatePage
> 
> 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.
> 
> This commit users MmAllocateContinguousMemorySpecifyCache
> to allocate memory instead, then builds and Mdl to wrap
> it up.
> 
> __AllocatePages is left unchanged (as we don't want
> to allocate multiple contiguous pages).  This issue
> has not been seen outside of xenvif calls to
> __AllocatePage and we expect a fix to the underlying
> Windows problem in the near future
> 
> Signed-off-by: Ben.Chalmers <ben.chalmers@xxxxxxxxxx>
> ---
>  src/xenvif/controller.c  | 12 +++++--
>  src/xenvif/receiver.c    | 56 ++++++++++++++++++++++--------
>  src/xenvif/transmitter.c | 24 +++++++++----
>  src/xenvif/util.h        | 89
> ++++++++++++++++++++++++++++++++++++++++++++----
>  4 files changed, 151 insertions(+), 30 deletions(-)
> 
> diff --git a/src/xenvif/controller.c b/src/xenvif/controller.c
> index 35901a2..37abb1d 100644
> --- a/src/xenvif/controller.c
> +++ b/src/xenvif/controller.c
> @@ -469,7 +469,9 @@ ControllerConnect(
>      if (Controller->Mdl == NULL)
>          goto fail7;
> 
> -    ASSERT(Controller->Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Controller->Mdl->MdlFlags
> +            & (MDL_MAPPED_TO_SYSTEM_VA
> +                | MDL_SOURCE_IS_NONPAGED_POOL));

Do we need to change these assertions? The intent is only to assert that the 
MappedSystemVa field is valid, as we're about to use it.

>      Controller->Shared = Controller->Mdl->MappedSystemVa;
>      ASSERT(Controller->Shared != NULL);
> 
> @@ -904,7 +906,9 @@ ControllerSetHashKey(
>      if (Mdl == NULL)
>          goto fail1;
> 
> -    ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Mdl->MdlFlags
> +            & (MDL_MAPPED_TO_SYSTEM_VA
> +                | MDL_SOURCE_IS_NONPAGED_POOL));
>      Buffer = Mdl->MappedSystemVa;
>      ASSERT(Buffer != NULL);
> 
> @@ -1083,7 +1087,9 @@ ControllerSetHashMapping(
>      if (Mdl == NULL)
>          goto fail2;
> 
> -    ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Mdl->MdlFlags
> +            & (MDL_MAPPED_TO_SYSTEM_VA
> +                | MDL_SOURCE_IS_NONPAGED_POOL));
>      Buffer = Mdl->MappedSystemVa;
>      ASSERT(Buffer != NULL);
> 
> diff --git a/src/xenvif/receiver.c b/src/xenvif/receiver.c
> index a6b3ad2..ab3e416 100644
> --- a/src/xenvif/receiver.c
> +++ b/src/xenvif/receiver.c
> @@ -169,7 +169,9 @@ __ReceiverPacketMdlInit(
>      Packet->Mdl.Size = sizeof (MDL) + sizeof (PFN_NUMBER);
>      Packet->Mdl.MdlFlags = Mdl->MdlFlags;
> 
> -    ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Mdl->MdlFlags &
> +           (MDL_MAPPED_TO_SYSTEM_VA |
> +            MDL_SOURCE_IS_NONPAGED_POOL));
>      Packet->Mdl.StartVa = Mdl->StartVa;
>      Packet->Mdl.MappedSystemVa = Mdl->MappedSystemVa;
> 
> @@ -406,7 +408,9 @@ ReceiverRingProcessTag(
> 
>      PayloadLength = Packet->Length - Info->Length;
> 
> -    ASSERT(Packet->Mdl.MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Packet->Mdl.MdlFlags &
> +           (MDL_MAPPED_TO_SYSTEM_VA |
> +            MDL_SOURCE_IS_NONPAGED_POOL));
>      BaseVa = Packet->Mdl.MappedSystemVa;
>      ASSERT(BaseVa != NULL);
> 
> @@ -497,7 +501,9 @@ ReceiverRingProcessChecksum(
>      if (Info->IpHeader.Length == 0)
>          return;
> 
> -    ASSERT(Packet->Mdl.MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Packet->Mdl.MdlFlags &
> +           (MDL_MAPPED_TO_SYSTEM_VA |
> +            MDL_SOURCE_IS_NONPAGED_POOL));
>      BaseVa = Packet->Mdl.MappedSystemVa;
>      ASSERT(BaseVa != NULL);
> 
> @@ -654,7 +660,9 @@ ReceiverRingPullup(
>          PUCHAR  SourceVa;
>          ULONG   CopyLength;
> 
> -        ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +        ASSERT(Mdl->MdlFlags &
> +               (MDL_MAPPED_TO_SYSTEM_VA |
> +                MDL_SOURCE_IS_NONPAGED_POOL));
>          SourceVa = Mdl->MappedSystemVa;
>          ASSERT(SourceVa != NULL);
> 
> @@ -700,7 +708,9 @@ __ReceiverRingPullupPacket(
>      XENVIF_PACKET_PAYLOAD       Payload;
>      ULONG                       Length;
> 
> -    ASSERT(Packet->Mdl.MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Packet->Mdl.MdlFlags
> +            & (MDL_MAPPED_TO_SYSTEM_VA
> +                | MDL_SOURCE_IS_NONPAGED_POOL));
>      BaseVa = Packet->Mdl.MappedSystemVa;
>      ASSERT(BaseVa != NULL);
> 
> @@ -744,7 +754,9 @@ __ReceiverRingBuildSegment(
> 
>      Info = &Packet->Info;
> 
> -    ASSERT(Packet->Mdl.MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Packet->Mdl.MdlFlags &
> +           (MDL_MAPPED_TO_SYSTEM_VA |
> +            MDL_SOURCE_IS_NONPAGED_POOL));
>      InfoVa = Packet->Mdl.MappedSystemVa;
>      ASSERT(InfoVa != NULL);
> 
> @@ -767,7 +779,9 @@ __ReceiverRingBuildSegment(
> 
>      Mdl = &Segment->Mdl;
> 
> -    ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Mdl->MdlFlags &
> +           (MDL_MAPPED_TO_SYSTEM_VA |
> +            MDL_SOURCE_IS_NONPAGED_POOL));
>      BaseVa = Mdl->MappedSystemVa;
>      ASSERT(BaseVa != NULL);
> 
> @@ -849,7 +863,9 @@ __ReceiverRingBuildSegment(
> 
>          Mdl = Mdl->Next;
> 
> -        ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +        ASSERT(Mdl->MdlFlags &
> +               (MDL_MAPPED_TO_SYSTEM_VA |
> +                MDL_SOURCE_IS_NONPAGED_POOL));
>          BaseVa = Mdl->MappedSystemVa;
>          ASSERT(BaseVa != NULL);
> 
> @@ -939,7 +955,9 @@ ReceiverRingProcessLargePacket(
> 
>      Packet->Mdl.Next = NULL;
> 
> -    ASSERT(Packet->Mdl.MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Packet->Mdl.MdlFlags &
> +           (MDL_MAPPED_TO_SYSTEM_VA |
> +            MDL_SOURCE_IS_NONPAGED_POOL));
>      InfoVa = Packet->Mdl.MappedSystemVa;
>      ASSERT(InfoVa != NULL);
> 
> @@ -1134,7 +1152,9 @@ ReceiverRingProcessStandardPacket(
>          if (Mdl == NULL)
>              goto fail2;
> 
> -        ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +        ASSERT(Mdl->MdlFlags &
> +               (MDL_MAPPED_TO_SYSTEM_VA |
> +                MDL_SOURCE_IS_NONPAGED_POOL));
>          BaseVa = Mdl->MappedSystemVa;
>          ASSERT(BaseVa != NULL);
> 
> @@ -1240,7 +1260,9 @@ ReceiverRingProcessPacket(
>      // Override offset to align
>      Packet->Offset = Receiver->IpAlignOffset;
> 
> -    ASSERT(Packet->Mdl.MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Packet->Mdl.MdlFlags &
> +           (MDL_MAPPED_TO_SYSTEM_VA |
> +            MDL_SOURCE_IS_NONPAGED_POOL));
>      BaseVa = Packet->Mdl.MappedSystemVa;
>      ASSERT(BaseVa != NULL);
> 
> @@ -1422,7 +1444,9 @@ __ReceiverRingReleaseLock(
>                                     XENVIF_RECEIVER_PACKET,
>                                     ListEntry);
> 
> -        ASSERT(Packet->Mdl.MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +        ASSERT(Packet->Mdl.MdlFlags &
> +               (MDL_MAPPED_TO_SYSTEM_VA |
> +                MDL_SOURCE_IS_NONPAGED_POOL));
>          BaseVa = Packet->Mdl.MappedSystemVa;
>          ASSERT(BaseVa != NULL);
> 
> @@ -2034,7 +2058,9 @@ ReceiverRingPoll(
> 
>                  ASSERT3U(rsp->id, ==, id);
> 
> -                ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +                ASSERT(Mdl->MdlFlags &
> +                       (MDL_MAPPED_TO_SYSTEM_VA |
> +                        MDL_SOURCE_IS_NONPAGED_POOL));
>                  BaseVa = Mdl->MappedSystemVa;
>                  ASSERT(BaseVa != NULL);
> 
> @@ -2419,7 +2445,9 @@ __ReceiverRingConnect(
>      if (Ring->Mdl == NULL)
>          goto fail3;
> 
> -    ASSERT(Ring->Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Ring->Mdl->MdlFlags &
> +           (MDL_MAPPED_TO_SYSTEM_VA |
> +            MDL_SOURCE_IS_NONPAGED_POOL));
>      Ring->Shared = Ring->Mdl->MappedSystemVa;
>      ASSERT(Ring->Shared != NULL);
> 
> diff --git a/src/xenvif/transmitter.c b/src/xenvif/transmitter.c
> index 1d46f85..bf6004a 100644
> --- a/src/xenvif/transmitter.c
> +++ b/src/xenvif/transmitter.c
> @@ -867,7 +867,9 @@ __TransmitterRingCopyPayload(
> 
>          Length = __min(Payload.Length, PAGE_SIZE);
> 
> -        ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +        ASSERT(Mdl->MdlFlags &
> +               (MDL_MAPPED_TO_SYSTEM_VA |
> +                MDL_SOURCE_IS_NONPAGED_POOL));
>          BaseVa = Mdl->MappedSystemVa;
>          ASSERT(BaseVa != NULL);
> 
> @@ -1187,7 +1189,9 @@ __TransmitterRingPrepareHeader(
> 
>      Mdl = Buffer->Mdl;
> 
> -    ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Mdl->MdlFlags &
> +           (MDL_MAPPED_TO_SYSTEM_VA |
> +            MDL_SOURCE_IS_NONPAGED_POOL));
>      BaseVa = Mdl->MappedSystemVa;
>      ASSERT(BaseVa != NULL);
> 
> @@ -1684,7 +1688,9 @@ __TransmitterRingPreparePacket(
> 
>              ASSERT3U(Mdl->ByteCount, <=, PAGE_SIZE - Trailer);
> 
> -            ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +            ASSERT(Mdl->MdlFlags &
> +                   (MDL_MAPPED_TO_SYSTEM_VA |
> +                    MDL_SOURCE_IS_NONPAGED_POOL));
>              BaseVa = Mdl->MappedSystemVa;
>              ASSERT(BaseVa != NULL);
> 
> @@ -1783,7 +1789,9 @@ __TransmitterRingPrepareArp(
> 
>      Mdl = Buffer->Mdl;
> 
> -    ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Mdl->MdlFlags &
> +           (MDL_MAPPED_TO_SYSTEM_VA |
> +            MDL_SOURCE_IS_NONPAGED_POOL));
>      BaseVa = Mdl->MappedSystemVa;
>      ASSERT(BaseVa != NULL);
> 
> @@ -1924,7 +1932,9 @@
> __TransmitterRingPrepareNeighbourAdvertisement(
> 
>      Mdl = Buffer->Mdl;
> 
> -    ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Mdl->MdlFlags &
> +           (MDL_MAPPED_TO_SYSTEM_VA |
> +            MDL_SOURCE_IS_NONPAGED_POOL));
>      BaseVa = Mdl->MappedSystemVa;
>      ASSERT(BaseVa != NULL);
> 
> @@ -3638,7 +3648,9 @@ __TransmitterRingConnect(
>      if (Ring->Mdl == NULL)
>          goto fail3;
> 
> -    ASSERT(Ring->Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);
> +    ASSERT(Ring->Mdl->MdlFlags &
> +           (MDL_MAPPED_TO_SYSTEM_VA |
> +            MDL_SOURCE_IS_NONPAGED_POOL));
>      Ring->Shared = Ring->Mdl->MappedSystemVa;
>      ASSERT(Ring->Shared != NULL);
> 
> diff --git a/src/xenvif/util.h b/src/xenvif/util.h
> index 30322d8..dfbe9ae 100644
> --- a/src/xenvif/util.h
> +++ b/src/xenvif/util.h
> @@ -210,10 +210,10 @@ __AllocatePages(
> 
>      MdlMappedSystemVa = MmMapLockedPagesSpecifyCache(Mdl,
>                                                       KernelMode,
> -                                                                          
> MmCached,
> -                                                                          
> NULL,
> -                                                                          
> FALSE,
> -
> NormalPagePriority);
> +                                                     MmCached,
> +                                                     NULL,
> +                                                     FALSE,
> +                                                     NormalPagePriority);
> 
>      status = STATUS_UNSUCCESSFUL;
>      if (MdlMappedSystemVa == NULL)

This appears to be a whitespace-only hunk so should be dropped or called out in 
the commit comment (so folks in future don't go looking for subtleties in it).

> @@ -244,11 +244,69 @@ fail1:
>      return NULL;
>  }
> 
> -#define __AllocatePage()    __AllocatePages(1)
> +
> +static FORCEINLINE PMDL
> +__AllocatePage()
> +{
> +    PHYSICAL_ADDRESS    LowAddress;
> +    PHYSICAL_ADDRESS    HighAddress;
> +    PHYSICAL_ADDRESS    Align;
> +    SIZE_T              TotalBytes;
> +    PMDL                Mdl;
> +    PUCHAR              MdlMappedSystemVa;
> +    NTSTATUS            status;
> +
> +    ASSERT3U(KeGetCurrentIrql(), <=, DISPATCH_LEVEL);
> +
> +    LowAddress.QuadPart  = 0ull;
> +    HighAddress.QuadPart = ~0ull;
> +    Align.QuadPart       = PAGE_SIZE;
> +    TotalBytes           = (SIZE_T)PAGE_SIZE;
> +
> +    MdlMappedSystemVa = MmAllocateContiguousMemorySpecifyCache(
> +                            TotalBytes,
> +                            LowAddress,
> +                            HighAddress,
> +                            Align,
> +                            MmCached);
> +
> +    status = STATUS_NO_MEMORY;
> +    if (MdlMappedSystemVa == NULL)
> +        goto fail1;
> +
> +

There's a double blank here.

> +    Mdl = IoAllocateMdl(MdlMappedSystemVa,
> +                        (ULONG)TotalBytes,
> +                        FALSE,
> +                        FALSE,
> +                        NULL);
> +

No need for a blank line here.

> +    if (Mdl == NULL)
> +        goto fail2;
> +
> +    MmBuildMdlForNonPagedPool(Mdl);
> +
> +    ASSERT3U(Mdl->ByteOffset, ==, 0);
> +    ASSERT3P(Mdl->StartVa, ==, MdlMappedSystemVa);
> +    ASSERT3P(Mdl->MappedSystemVa, ==, MdlMappedSystemVa);
> +
> +    RtlZeroMemory(MdlMappedSystemVa, Mdl->ByteCount);
> +
> +    return Mdl;
> +
> +fail2:
> +    Error("fail2\n");
> +
> +    MmFreeContiguousMemory(MdlMappedSystemVa);
> +fail1:
> +    Error("fail1 (%08x)\n", status);
> +
> +    return NULL;
> +}
> 
>  static FORCEINLINE VOID
>  __FreePages(
> -    IN       PMDL    Mdl
> +    IN  PMDL    Mdl

What changed? Must be whitespace.

>      )
>  {
>      PUCHAR   MdlMappedSystemVa;
> @@ -262,7 +320,24 @@ __FreePages(
>      ExFreePool(Mdl);
>  }
> 
> -#define __FreePage(_Mdl)    __FreePages(_Mdl)
> +static FORCEINLINE VOID
> +__FreePage(
> +    IN  PMDL    Mdl
> +    )
> +{
> +    PUCHAR  MdlMappedSystemVa;
> +
> +    ASSERT(Mdl->MdlFlags &
> +            (MDL_MAPPED_TO_SYSTEM_VA |
> +             MDL_SOURCE_IS_NONPAGED_POOL));
> +
> +    MdlMappedSystemVa = Mdl->MappedSystemVa;
> +
> +    IoFreeMdl(Mdl);
> +
> +    MmFreeContiguousMemory(MdlMappedSystemVa);
> +

Stray blank line.

  Paul

> +}
> 
>  static FORCEINLINE PCHAR
>  __strtok_r(
> --
> 2.10.1.windows.1
> 
> 
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/win-pv-devel
_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/win-pv-devel