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:[email protected]] > Sent: Thursday, October 25, 2018 3:18 PM > To: [email protected] > Cc: Kinney, Michael D <[email protected]>; Ni, Ruiyu > <[email protected]>; Yao, Jiewen <[email protected]>; Zeng, Star > <[email protected]>; Laszlo Ersek <[email protected]> > 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 <[email protected]> > Cc: Michael D Kinney <[email protected]> > Cc: Jiewen Yao <[email protected]> > Cc: Ruiyu Ni <[email protected]> > Cc: Laszlo Ersek <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <[email protected]> > --- > 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 > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
