From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.115; helo=mga14.intel.com; envelope-from=jian.j.wang@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 81C6C2117D772 for ; Tue, 23 Oct 2018 22:27:19 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Oct 2018 22:27:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,419,1534834800"; d="scan'208";a="102139888" Received: from shwdeopenpsi777.ccr.corp.intel.com ([10.239.158.27]) by orsmga001.jf.intel.com with ESMTP; 23 Oct 2018 22:27:17 -0700 From: Jian J Wang To: edk2-devel@lists.01.org Cc: Star Zeng , Michael D Kinney , Jiewen Yao , Ruiyu Ni , Laszlo Ersek Date: Wed, 24 Oct 2018 13:26:19 +0800 Message-Id: <20181024052620.4088-6-jian.j.wang@intel.com> X-Mailer: git-send-email 2.16.2.windows.1 In-Reply-To: <20181024052620.4088-1-jian.j.wang@intel.com> References: <20181024052620.4088-1-jian.j.wang@intel.com> Subject: [PATCH v3 5/6] MdeModulePkg/Core: prevent re-acquire GCD memory lock X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Oct 2018 05:27:19 -0000 > v3 changes: > a. drop the changes to CoreGetIoSpaceMap() because it won't cause > problem > b. refine the logic in changes of CoreGetMemorySpaceMap() > and add more comments 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. Although it's rare, a while-loop is added to make sure that the count of descriptor of memory map is the same after memory allocation, because it's executed outside the GCD memory lock. CoreDumpGcdMemorySpaceMap() => CoreGetMemorySpaceMap() => CoreAcquireGcdMemoryLock () * AllocatePool() => InternalAllocatePool() => CoreAllocatePool() => CoreAllocatePoolI() => CoreAllocatePoolPagesI() => CoreAllocatePoolPages() => FindFreePages() => PromoteMemoryResource() => CoreAcquireGcdMemoryLock() ** Cc: Star Zeng Cc: Michael D Kinney Cc: Jiewen Yao Cc: Ruiyu Ni Cc: Laszlo Ersek Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jian J Wang --- MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 80 +++++++++++++++++++++++++++-------------- 1 file changed, 54 insertions(+), 26 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index d9c65a8045..bc02945721 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,66 @@ CoreGetMemorySpaceMap ( return EFI_INVALID_PARAMETER; } + *NumberOfDescriptors = 0; + *MemorySpaceMap = NULL; + CoreAcquireGcdMemoryLock (); - // - // Count the number of descriptors - // - *NumberOfDescriptors = CoreCountGcdMapEntry (&mGcdMemorySpaceMap); + 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; + } - // - // Allocate the MemorySpaceMap - // - *MemorySpaceMap = AllocatePool (*NumberOfDescriptors * sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR)); - if (*MemorySpaceMap == NULL) { - Status = EFI_OUT_OF_RESOURCES; - goto Done; - } + break; + } - // - // Fill in the MemorySpaceMap - // - 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; + // + // 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 number 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; + CoreAcquireGcdMemoryLock (); } - Status = EFI_SUCCESS; -Done: CoreReleaseGcdMemoryLock (); - return Status; + + return EFI_SUCCESS; } -- 2.16.2.windows.1