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

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

Sorry missing that one. I'll update it. Thanks for the comments.

Regards,
Jian


> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, October 26, 2018 9:18 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel <edk2-devel-
> bounces@lists.01.org>; 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>; Laszlo Ersek
> <lersek@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH v4 5/6] MdeModulePkg/Core: prevent re-acquire
> GCD memory lock
> 
> On 2018/10/25 15:20, Wang, Jian J wrote:
> > 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
> 
> How about using "Count the descriptors" here? No need to send new patch
> series for it.
> 
> >> because
> >> +    // there's code which has to be running outside the GCD lock.
> 
> I have given comment for this line at V3 series.
> How about using "AllocatePool() calling code below" instead of "there's
> code" to be more specific?
> 
> Thanks,
> Star
> 
> >> +    //
> >> +    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