Re: [PATCH v4 5/6] MdeModulePkg/Core: prevent re-acquire GCD memory lock

["Wang, Jian J" <jian.j.wang@intel.com>]

Sorry, forgot to update commit message:

> This issue is hidden in current code but exposed by introduction
> of freed-memory guard feature due to the fact that the feature
> will turn all pool allocation to page allocation.
> 
> The solution is moving the memory allocation in CoreGetMemorySpaceMap()
> to be out of the GCD memory map lock.

Regards,
Jian


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> Sent: Thursday, October 25, 2018 3:18 PM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2] [PATCH v4 5/6] MdeModulePkg/Core: prevent re-acquire GCD
> memory lock
> 
> > v4 changes:
> > a. add more comments from Laszlo
> 
> This issue is hidden in current code but exposed by introduction
> of freed-memory guard feature due to the fact that the feature
> will turn all pool allocation to page allocation.
> 
> The solution is move the memory allocation in CoreGetMemorySpaceMap()
> and CoreGetIoSpaceMap() to be out of the GCD memory map lock.
> 
>    CoreDumpGcdMemorySpaceMap()
> => CoreGetMemorySpaceMap()
> => CoreAcquireGcdMemoryLock () *
>    AllocatePool()
> => InternalAllocatePool()
> => CoreAllocatePool()
> => CoreAllocatePoolI()
> => CoreAllocatePoolPagesI()
> => CoreAllocatePoolPages()
> => FindFreePages()
> => PromoteMemoryResource()
> => CoreAcquireGcdMemoryLock()  **
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 87 +++++++++++++++++++++++++++++-
> -----------
>  1 file changed, 62 insertions(+), 25 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> index d9c65a8045..f99d6bb933 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -1691,10 +1691,10 @@ CoreGetMemorySpaceMap (
>    OUT EFI_GCD_MEMORY_SPACE_DESCRIPTOR  **MemorySpaceMap
>    )
>  {
> -  EFI_STATUS                       Status;
>    LIST_ENTRY                       *Link;
>    EFI_GCD_MAP_ENTRY                *Entry;
>    EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *Descriptor;
> +  UINTN                            DescriptorCount;
> 
>    //
>    // Make sure parameters are valid
> @@ -1706,38 +1706,75 @@ CoreGetMemorySpaceMap (
>      return EFI_INVALID_PARAMETER;
>    }
> 
> -  CoreAcquireGcdMemoryLock ();
> +  *NumberOfDescriptors  = 0;
> +  *MemorySpaceMap       = NULL;
> 
>    //
> -  // Count the number of descriptors
> +  // Take the lock, for entering the loop with the lock held.
>    //
> -  *NumberOfDescriptors = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
> +  CoreAcquireGcdMemoryLock ();
> +  while (TRUE) {
> +    //
> +    // Count the number of descriptors. It might be done more than once
> because
> +    // there's code which has to be running outside the GCD lock.
> +    //
> +    DescriptorCount = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
> +    if (DescriptorCount == *NumberOfDescriptors) {
> +      //
> +      // Fill in the MemorySpaceMap if no memory space map change.
> +      //
> +      Descriptor = *MemorySpaceMap;
> +      Link = mGcdMemorySpaceMap.ForwardLink;
> +      while (Link != &mGcdMemorySpaceMap) {
> +        Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
> +        BuildMemoryDescriptor (Descriptor, Entry);
> +        Descriptor++;
> +        Link = Link->ForwardLink;
> +      }
> +      //
> +      // We're done; exit the loop with the lock held.
> +      //
> +      break;
> +    }
> 
> -  //
> -  // Allocate the MemorySpaceMap
> -  //
> -  *MemorySpaceMap = AllocatePool (*NumberOfDescriptors * sizeof
> (EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
> -  if (*MemorySpaceMap == NULL) {
> -    Status = EFI_OUT_OF_RESOURCES;
> -    goto Done;
> -  }
> +    //
> +    // Release the lock before memory allocation, because it might cause
> +    // GCD lock conflict in one of calling path in AllocatPool().
> +    //
> +    CoreReleaseGcdMemoryLock ();
> +
> +    //
> +    // Allocate memory to store the MemorySpaceMap. Note it might be already
> +    // allocated if there's map descriptor change during memory allocation at
> +    // last time.
> +    //
> +    if (*MemorySpaceMap != NULL) {
> +      FreePool (*MemorySpaceMap);
> +    }
> 
> +    *MemorySpaceMap = AllocatePool (DescriptorCount *
> +                                    sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
> +    if (*MemorySpaceMap == NULL) {
> +      *NumberOfDescriptors = 0;
> +      return EFI_OUT_OF_RESOURCES;
> +    }
> +
> +    //
> +    // Save the descriptor count got before for another round of check to make
> +    // sure we won't miss any, since we have code running outside the GCD lock.
> +    //
> +    *NumberOfDescriptors = DescriptorCount;
> +    //
> +    // Re-acquire the lock, for the next iteration.
> +    //
> +    CoreAcquireGcdMemoryLock ();
> +  }
>    //
> -  // Fill in the MemorySpaceMap
> +  // We exited the loop with the lock held, release it.
>    //
> -  Descriptor = *MemorySpaceMap;
> -  Link = mGcdMemorySpaceMap.ForwardLink;
> -  while (Link != &mGcdMemorySpaceMap) {
> -    Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
> -    BuildMemoryDescriptor (Descriptor, Entry);
> -    Descriptor++;
> -    Link = Link->ForwardLink;
> -  }
> -  Status = EFI_SUCCESS;
> -
> -Done:
>    CoreReleaseGcdMemoryLock ();
> -  return Status;
> +
> +  return EFI_SUCCESS;
>  }
> 
> 
> --
> 2.16.2.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel