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:[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
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
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
