From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 D97602117A2BC for ; Tue, 23 Oct 2018 10:16:57 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 620E731676AA; Tue, 23 Oct 2018 17:16:56 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-214.rdu2.redhat.com [10.10.120.214]) by smtp.corp.redhat.com (Postfix) with ESMTP id A83D860F80; Tue, 23 Oct 2018 17:16:54 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Michael D Kinney , Ruiyu Ni , Jiewen Yao , Star Zeng References: <20181023145331.5768-1-jian.j.wang@intel.com> <20181023145331.5768-6-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: <61eb5e94-6c34-d64a-7175-34badaa15096@redhat.com> Date: Tue, 23 Oct 2018 19:16:53 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181023145331.5768-6-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.41]); Tue, 23 Oct 2018 17:16:56 +0000 (UTC) Subject: Re: [PATCH v2 5/5] MdeModulePkg/Core: fix-up for changes introduced by freed-memory guard 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 17:16:58 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/23/18 16:53, Jian J Wang wrote: >> v2 changes: >> a. Update IsHeapGuardEnabled() calling because its prototype change > > Two changes are fixed up: > a. Prototype change of function IsHeapGuardEnabled() This kind of separation, between the patches, is wrong then. If someone bisects the edk2 git history, and checks out the edk2 tree at patch #4 in this series, they will get a build failure. > b. More memory map descriptors are introduced by new feature. > MergeMemoryMap() is updated to merge freed-pages into adjacent memory > descriptor to reduce the overall descriptor number. In such cases, the usual way to structure the patch series is as follows: - First the patch is added that makes the dependent / consumer code more generic, so that it can later cope with the new feature. Right after this "prep" patch, the new code paths in the consumer code are not exercised in practice. Importantly however, there is neither a compilation failure, nor a functionality error. It's just that the new additions are not active yet; they work as NOPs. - Second, the patch with the new feature is added; it basically "snaps in place", and activates the code paths that were prepared earlier. In large patch sets, it's not uncommon to see 5+ "prep" patches, each equipping a separate aspect (or consumer site) for the new feature, gradually. And then the final feature patch is plugged in. Thanks Laszlo > > 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/Misc/MemoryProtection.c | 2 +- > MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 16 +++++++--------- > 2 files changed, 8 insertions(+), 10 deletions(-) > > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c > index fa8f8fe91a..6298b67db1 100644 > --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c > +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c > @@ -1250,7 +1250,7 @@ ApplyMemoryProtectionPolicy ( > // Don't overwrite Guard pages, which should be the first and/or last page, > // if any. > // > - if (IsHeapGuardEnabled ()) { > + if (IsHeapGuardEnabled (GUARD_HEAP_TYPE_PAGE|GUARD_HEAP_TYPE_POOL)) { > if (IsGuardPage (Memory)) { > Memory += EFI_PAGE_SIZE; > Length -= EFI_PAGE_SIZE; > diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c > index 05eb4f422b..f6595c90ed 100644 > --- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c > +++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c > @@ -32,6 +32,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > #include > > #include "DxeMain.h" > +#include "HeapGuard.h" > > #define PREVIOUS_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \ > ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) - (Size))) > @@ -205,16 +206,13 @@ MergeMemoryMap ( > NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize); > > do { > - MemoryBlockLength = (UINT64) (EfiPagesToSize (MemoryMapEntry->NumberOfPages)); > + MergeGuardPages (NewMemoryMapEntry, NextMemoryMapEntry->PhysicalStart); > + MemoryBlockLength = (UINT64) (EfiPagesToSize (NewMemoryMapEntry->NumberOfPages)); > if (((UINTN)NextMemoryMapEntry < (UINTN)MemoryMapEnd) && > - (MemoryMapEntry->Type == NextMemoryMapEntry->Type) && > - (MemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) && > - ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) == NextMemoryMapEntry->PhysicalStart)) { > - MemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages; > - if (NewMemoryMapEntry != MemoryMapEntry) { > - NewMemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages; > - } > - > + (NewMemoryMapEntry->Type == NextMemoryMapEntry->Type) && > + (NewMemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) && > + ((NewMemoryMapEntry->PhysicalStart + MemoryBlockLength) == NextMemoryMapEntry->PhysicalStart)) { > + NewMemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages; > NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (NextMemoryMapEntry, DescriptorSize); > continue; > } else { >