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.43; helo=mga05.intel.com; envelope-from=jian.j.wang@intel.com; receiver=edk2-devel@lists.01.org Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 2D1032117D29B for ; Tue, 23 Oct 2018 07:53:46 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Oct 2018 07:53:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,416,1534834800"; d="scan'208";a="80278233" Received: from shwdeopenpsi777.ccr.corp.intel.com ([10.239.158.27]) by fmsmga007.fm.intel.com with ESMTP; 23 Oct 2018 07:53:40 -0700 From: Jian J Wang To: edk2-devel@lists.01.org Cc: Star Zeng , Michael D Kinney , Jiewen Yao , Ruiyu Ni , Laszlo Ersek Date: Tue, 23 Oct 2018 22:53:29 +0800 Message-Id: <20181023145331.5768-4-jian.j.wang@intel.com> X-Mailer: git-send-email 2.16.2.windows.1 In-Reply-To: <20181023145331.5768-1-jian.j.wang@intel.com> References: <20181023145331.5768-1-jian.j.wang@intel.com> Subject: [PATCH v2 3/5] MdeModulePkg/Core: fix a lock issue in GCD memory map dump 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: Tue, 23 Oct 2018 14:53:46 -0000 > v2 changes: > a. Update implementation of CoreGetMemorySpaceMap() and > CoreGetIoSpaceMap() to avoid lock failure. Drop the code to > detect debug print level used to achieve the same effect. 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 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 | 140 ++++++++++++++++++++++++---------------- 1 file changed, 86 insertions(+), 54 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index d9c65a8045..133c3dcd87 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 Number; // // Make sure parameters are valid @@ -1706,38 +1706,54 @@ CoreGetMemorySpaceMap ( return EFI_INVALID_PARAMETER; } - CoreAcquireGcdMemoryLock (); - // // Count the number of descriptors // - *NumberOfDescriptors = CoreCountGcdMapEntry (&mGcdMemorySpaceMap); + Number = 0; + *NumberOfDescriptors = 0; + *MemorySpaceMap = NULL; + while (TRUE) { + // + // Allocate the MemorySpaceMap + // + if (Number != 0) { + if (*MemorySpaceMap != NULL) { + FreePool (*MemorySpaceMap); + } - // - // Allocate the MemorySpaceMap - // - *MemorySpaceMap = AllocatePool (*NumberOfDescriptors * sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR)); - if (*MemorySpaceMap == NULL) { - Status = EFI_OUT_OF_RESOURCES; - goto Done; - } + *MemorySpaceMap = AllocatePool (Number * sizeof(EFI_GCD_MEMORY_SPACE_DESCRIPTOR)); + if (*MemorySpaceMap == NULL) { + return EFI_OUT_OF_RESOURCES; + } - // - // 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; + *NumberOfDescriptors = Number; + } + + CoreAcquireGcdMemoryLock (); + + Number = CoreCountGcdMapEntry (&mGcdMemorySpaceMap); + if (Number == *NumberOfDescriptors) { + // + // Fill in the MemorySpaceMap + // + Descriptor = *MemorySpaceMap; + Link = mGcdMemorySpaceMap.ForwardLink; + while (Link != &mGcdMemorySpaceMap && Number != 0) { + Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE); + BuildMemoryDescriptor (Descriptor, Entry); + Descriptor++; + Number--; + Link = Link->ForwardLink; + } + + CoreReleaseGcdMemoryLock (); + break; + } + + CoreReleaseGcdMemoryLock (); } - Status = EFI_SUCCESS; -Done: - CoreReleaseGcdMemoryLock (); - return Status; + return EFI_SUCCESS; } @@ -1964,10 +1980,10 @@ CoreGetIoSpaceMap ( OUT EFI_GCD_IO_SPACE_DESCRIPTOR **IoSpaceMap ) { - EFI_STATUS Status; LIST_ENTRY *Link; EFI_GCD_MAP_ENTRY *Entry; EFI_GCD_IO_SPACE_DESCRIPTOR *Descriptor; + UINTN Number; // // Make sure parameters are valid @@ -1979,38 +1995,54 @@ CoreGetIoSpaceMap ( return EFI_INVALID_PARAMETER; } - CoreAcquireGcdIoLock (); + Number = 0; + *NumberOfDescriptors = 0; + *IoSpaceMap = NULL; + while (TRUE) { + // + // Allocate the IoSpaceMap + // + if (Number != 0) { + if (*IoSpaceMap != NULL) { + FreePool (*IoSpaceMap); + } - // - // Count the number of descriptors - // - *NumberOfDescriptors = CoreCountGcdMapEntry (&mGcdIoSpaceMap); + *IoSpaceMap = AllocatePool (Number * sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR)); + if (*IoSpaceMap == NULL) { + return EFI_OUT_OF_RESOURCES; + } - // - // Allocate the IoSpaceMap - // - *IoSpaceMap = AllocatePool (*NumberOfDescriptors * sizeof (EFI_GCD_IO_SPACE_DESCRIPTOR)); - if (*IoSpaceMap == NULL) { - Status = EFI_OUT_OF_RESOURCES; - goto Done; - } + *NumberOfDescriptors = Number; + } - // - // Fill in the IoSpaceMap - // - Descriptor = *IoSpaceMap; - Link = mGcdIoSpaceMap.ForwardLink; - while (Link != &mGcdIoSpaceMap) { - Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE); - BuildIoDescriptor (Descriptor, Entry); - Descriptor++; - Link = Link->ForwardLink; + CoreAcquireGcdIoLock (); + + // + // Count the number of descriptors + // + Number = CoreCountGcdMapEntry (&mGcdIoSpaceMap); + if (Number == *NumberOfDescriptors) { + // + // Fill in the IoSpaceMap + // + Descriptor = *IoSpaceMap; + Link = mGcdIoSpaceMap.ForwardLink; + while (Link != &mGcdIoSpaceMap && Number != 0) { + Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE); + BuildIoDescriptor (Descriptor, Entry); + Descriptor++; + Number--; + Link = Link->ForwardLink; + } + + CoreReleaseGcdIoLock (); + break; + } + + CoreReleaseGcdIoLock (); } - Status = EFI_SUCCESS; -Done: - CoreReleaseGcdIoLock (); - return Status; + return EFI_SUCCESS; } -- 2.16.2.windows.1