From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (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 1A39421CE748F for ; Wed, 2 Aug 2017 05:26:18 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Aug 2017 05:28:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,311,1498546800"; d="scan'208";a="118660632" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga002.jf.intel.com with ESMTP; 02 Aug 2017 05:28:17 -0700 Received: from fmsmsx114.amr.corp.intel.com (10.18.116.8) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 2 Aug 2017 05:28:17 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX114.amr.corp.intel.com (10.18.116.8) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 2 Aug 2017 05:28:16 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.146]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.116]) with mapi id 14.03.0319.002; Wed, 2 Aug 2017 20:28:14 +0800 From: "Gao, Liming" To: "Marvin.Haeuser@outlook.com" , "edk2-devel@lists.01.org" CC: "Kinney, Michael D" Thread-Topic: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function. Thread-Index: AQHTA5vus1udQdORhEegPjUTqWKZfaJjLSJAgAADdaCABJ2G8IAAEHiQgAkqlhCAAAE7IIAAA9sA Date: Wed, 2 Aug 2017 12:28:13 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D75EAFF@shsmsx102.ccr.corp.intel.com> References: <4A89E2EF3DFEDB4C8BFDE51014F606A14D75CC1D@shsmsx102.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D75EAD3@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 Aug 2017 12:26:18 -0000 Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Thanks for your correctness. I agree MACRO is better.=20 >-----Original Message----- >From: Marvin H=E4user [mailto:Marvin.Haeuser@outlook.com] >Sent: Wednesday, August 02, 2017 8:24 PM >To: edk2-devel@lists.01.org >Cc: Gao, Liming ; Kinney, Michael D > >Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function. > >Hey Liming, > >Thanks for your reply! Sure, that works as well, though usually I prefer t= o >write a static function or a macro instead of repeating. >If you prefer this, I'll get a V2 ready that way. Though shouldn't it be A= SSERT >((!FeaturePcdGet (PcdVerifyNodeInList) || IsNodeInList (FirstEntry, >SecondEntry)) && InternalBaseLibIsListValid (FirstEntry)); ? >Correct me if I'm wrong, but wouldn't your suggestion not ASSERT if the >feature is enabled, the entry is not in the list, but the list is valid? (= (TRUE && >FALSE) || TRUE) would return TRUE. > >Thanks, >Marvin. > >> -----Original Message----- >> From: Gao, Liming [mailto:liming.gao@intel.com] >> Sent: Wednesday, August 2, 2017 2:12 PM >> To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org >> Cc: Kinney, Michael D >> Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function. >> >> Marvin: >> How about directly use PCD checker with IsNodeInList()? >> >> ASSERT (IsNodeInList (FirstEntry, SecondEntry)); =3D=3D> ASSERT >> (FeaturePcdGet (PcdVerifyNodeInList) && IsNodeInList (FirstEntry, >> SecondEntry))|| InternalBaseLibIsListValid (FirstEntry)); >> >> Thanks >> Liming >> >-----Original Message----- >> >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> >Marvin H?user >> >Sent: Friday, July 28, 2017 12:20 AM >> >To: edk2-devel@lists.01.org >> >Cc: Kinney, Michael D ; Gao, Liming >> > >> >Subject: Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() >> function. >> > >> >Hi Liming, >> > >> >Comments are inline. Thanks for your response! >> > >> >Regards, >> >Marvin. >> > >> >> -----Original Message----- >> >> From: Gao, Liming [mailto:liming.gao@intel.com] >> >> Sent: Thursday, July 27, 2017 5:32 PM >> >> To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org >> >> Cc: Kinney, Michael D >> >> Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function. >> >> >> >> Marvin: >> >> Once this API is exposed as the public library API, what code will >> >> you >> >update >> >> to consume this API? >> > >> >The code which gave me the idea for this patch is this: >> >https://github.com/tianocore/edk2-platforms/blob/devel- >> >MinPlatform/Silicon/Intel/KabylakeSiliconPkg/Pch/PchSmiDispatcher/Smm >/ >> I >> >o >> >Trap.c#L508 >> >ForwardLink is obviously checked against EFI_BAD_POINTER to verify >> >whether the entry is still valid. Though, after being freed, the value >> >stored in this location may just be overwritten. >> >The code linked currently doesn't seem to do anything as ForwardLink is >> >never set to EFI_BAD_POINTER, though it can be updated with >> >IsNodeInList() to function (regardless of the entry being freed). >> >It seems to me that this is a leftover from the EDK era as EDK's >> >RemoveEntryList() does set the Links to EFI_BAD_POINTER. >> > >> >> >> >> And, for the patch, I have two comments. >> >> 1) Original logic uses PcdVerifyNodeInList to turn on the >> >> verification of NodeInList. New logic always turns on it. Because >> >> this check takes much overhead, this PCD is FALSE as the default valu= e in >> MdePkg.dec. >> >> 2) In IsListEmpty(), ASSERT() is missing for >> >> InternalBaseLibIsListValid (ListHead). >> > >> >Sorry for these oversights. Would you prefer an internal, static >> >function or a macro to do the PCD check and following the ASSERTin v2? >> > >> >> >> >> Thanks >> >> Liming >> >> > -----Original Message----- >> >> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >> >> > Of Marvin H?user >> >> > Sent: Tuesday, July 25, 2017 12:53 AM >> >> > To: edk2-devel@lists.01.org >> >> > Cc: Kinney, Michael D >> >> > Subject: Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() >> >> function. >> >> > >> >> > Hey Mike, >> >> > >> >> > In contrast to setting 'BackLink' and 'ForwardLink' to dummy values >> >> > such as EFI_BAD_POINTER, verifying that a node is part of a known >> >> > list is a safe way to determine whether a link is still valid >> >> > without needing to keep the memory allocated. If the Node, which >> >> > had its links set to dummys, is freed, the dummy values may be >> >> > overwriten and the check would pass despite the node being invalid. >> >> > This can, for example, be >> >> used in OpenKabylake's PchSmiDispatcher, which does the described >> >> check against EFI_BAD_POINTER (though if I remember correctly, the >> >> value is actually never explicitely set). The issue is that some >> >> function may pass a handle that was unregistered to the function. >> >> IsNodeInList() can be used to determine whether it was previously >> removed. >> >> > >> >> > Thanks for your response! >> >> > >> >> > Regards, >> >> > Marvin. >> >> > >> >> > > -----Original Message----- >> >> > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com] >> >> > > Sent: Monday, July 24, 2017 6:31 PM >> >> > > To: Marvin H=E4user ; edk2- >> >> > > devel@lists.01.org; Kinney, Michael D >> >> > > >> >> > > Cc: Gao, Liming >> >> > > Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() >> function. >> >> > > >> >> > > Hi Marvin, >> >> > > >> >> > > Can you provide a few more details on why you would like to see >> >> > > tis internal function promoted to a library class API? >> >> > > >> >> > > Thanks, >> >> > > >> >> > > Mike >> >> > > >> >> > > > -----Original Message----- >> >> > > > From: Marvin H=E4user [mailto:Marvin.Haeuser@outlook.com] >> >> > > > Sent: Sunday, July 23, 2017 3:11 AM >> >> > > > To: edk2-devel@lists.01.org >> >> > > > Cc: Kinney, Michael D ; Gao, Liming >> >> > > > >> >> > > > Subject: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() functio= n. >> >> > > > >> >> > > > This patch adds IsNodeInList() to BaseLib, which verifies the >> >> > > > given Node is part of the doubly-linked List provided. >> >> > > > >> >> > > > Contributed-under: TianoCore Contribution Agreement 1.1 >> >> > > > Signed-off-by: Marvin Haeuser >> >> > > > --- >> >> > > > MdePkg/Library/BaseLib/LinkedList.c | 70 >> >> > > > +++++++++++++++++++- >> >> > > > MdePkg/Include/Library/BaseLib.h | 25 +++++++ >> >> > > > MdePkg/Library/BaseLib/BaseLibInternals.h | 28 -------- >> >> > > > 3 files changed, 94 insertions(+), 29 deletions(-) >> >> > > > >> >> > > > diff --git a/MdePkg/Library/BaseLib/LinkedList.c >> >> > > > b/MdePkg/Library/BaseLib/LinkedList.c >> >> > > > index ba373f4b7be3..b364ae41c647 100644 >> >> > > > --- a/MdePkg/Library/BaseLib/LinkedList.c >> >> > > > +++ b/MdePkg/Library/BaseLib/LinkedList.c >> >> > > > @@ -1,7 +1,7 @@ >> >> > > > /** @file >> >> > > > Linked List Library Functions. >> >> > > > >> >> > > > - Copyright (c) 2006 - 2013, Intel Corporation. All rights >> >> > > > reserved.
>> >> > > > + Copyright (c) 2006 - 2017, Intel Corporation. All rights >> >> > > > reserved.
>> >> > > > This program and the accompanying materials >> >> > > > are licensed and made available under the terms and >> >> > > > conditions of the BSD License >> >> > > > which accompanies this distribution. The full text of the >> >> > > > license may be found at @@ -113,6 +113,74 @@ >> >> > > > InternalBaseLibIsNodeInList ( } >> >> > > > >> >> > > > /** >> >> > > > + Checks whether Node is part of a doubly-linked list. >> >> > > > + >> >> > > > + If List is NULL, then ASSERT(). >> >> > > > + If List->ForwardLink is NULL, then ASSERT(). >> >> > > > + If List->BackLink is NULL, then ASSERT(). >> >> > > > + If Node is NULL, then ASSERT(); If >> >> > > > + PcdMaximumLinkedListLength is not zero, and List contains >> >> > > > more than >> >> > > > + PcdMaximumLinkedListLength nodes, then ASSERT(). >> >> > > > + >> >> > > > + @param List A pointer to a node in a linked list. >> >> > > > + @param Node A pointer to the node to locate. >> >> > > > + >> >> > > > + @retval TRUE Node is in List. >> >> > > > + @retval FALSE Node isn't in List, or List is invalid. >> >> > > > + >> >> > > > +**/ >> >> > > > +BOOLEAN >> >> > > > +EFIAPI >> >> > > > +IsNodeInList ( >> >> > > > + IN CONST LIST_ENTRY *List, >> >> > > > + IN CONST LIST_ENTRY *Node >> >> > > > + ) >> >> > > > +{ >> >> > > > + UINTN Count; >> >> > > > + CONST LIST_ENTRY *Ptr; >> >> > > > + >> >> > > > + // >> >> > > > + // Test the validity of List and Node // ASSERT (List !=3D >> >> > > > + NULL); ASSERT (List->ForwardLink !=3D NULL); ASSERT >> >> > > > + (List->BackLink !=3D NULL); ASSERT (Node !=3D NULL); >> >> > > > + >> >> > > > + // >> >> > > > + // ASSERT List not too long >> >> > > > + // >> >> > > > + ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, >> >> > > > + FALSE)); >> >> > > > + >> >> > > > + Count =3D 0; >> >> > > > + Ptr =3D List; >> >> > > > + >> >> > > > + // >> >> > > > + // Check to see if Node is a member of List. >> >> > > > + // Exit early if the number of nodes in List >=3D >> >> > > > PcdMaximumLinkedListLength >> >> > > > + // >> >> > > > + do { >> >> > > > + Ptr =3D Ptr->ForwardLink; >> >> > > > + if (PcdGet32 (PcdMaximumLinkedListLength) > 0) { >> >> > > > + Count++; >> >> > > > + >> >> > > > + // >> >> > > > + // Return if the linked list is too long >> >> > > > + // >> >> > > > + if (Count =3D=3D PcdGet32 (PcdMaximumLinkedListLength)) = { >> >> > > > + return (BOOLEAN)(Ptr =3D=3D Node); >> >> > > > + } >> >> > > > + } >> >> > > > + >> >> > > > + if (Ptr =3D=3D Node) { >> >> > > > + return TRUE; >> >> > > > + } >> >> > > > + } while (Ptr !=3D List); >> >> > > > + >> >> > > > + return FALSE; >> >> > > > +} >> >> > > > + >> >> > > > +/** >> >> > > > Initializes the head node of a doubly-linked list, and >> >> > > > returns the pointer to >> >> > > > the head node of the doubly-linked list. >> >> > > > >> >> > > > diff --git a/MdePkg/Include/Library/BaseLib.h >> >> > > > b/MdePkg/Include/Library/BaseLib.h >> >> > > > index 791849b80406..4f3f4fd51f3f 100644 >> >> > > > --- a/MdePkg/Include/Library/BaseLib.h >> >> > > > +++ b/MdePkg/Include/Library/BaseLib.h >> >> > > > @@ -2869,6 +2869,31 @@ PathCleanUpDirectories( >> >> > > > >> >> > > > >> >> > > > /** >> >> > > > + Checks whether Node is part of a doubly-linked list. >> >> > > > + >> >> > > > + If List is NULL, then ASSERT(). >> >> > > > + If List->ForwardLink is NULL, then ASSERT(). >> >> > > > + If List->BackLink is NULL, then ASSERT(). >> >> > > > + If Node is NULL, then ASSERT(); If >> >> > > > + PcdMaximumLinkedListLength is not zero, and List contains >> >> > > > more than >> >> > > > + PcdMaximumLinkedListLength nodes, then ASSERT(). >> >> > > > + >> >> > > > + @param List A pointer to a node in a linked list. >> >> > > > + @param Node A pointer to the node to locate. >> >> > > > + >> >> > > > + @retval TRUE Node is in List. >> >> > > > + @retval FALSE Node isn't in List, or List is invalid. >> >> > > > + >> >> > > > +**/ >> >> > > > +BOOLEAN >> >> > > > +EFIAPI >> >> > > > +IsNodeInList ( >> >> > > > + IN CONST LIST_ENTRY *List, >> >> > > > + IN CONST LIST_ENTRY *Node >> >> > > > + ); >> >> > > > + >> >> > > > + >> >> > > > +/** >> >> > > > Initializes the head node of a doubly linked list, and >> >> > > > returns the pointer to >> >> > > > the head node of the doubly linked list. >> >> > > > >> >> > > > diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h >> >> > > > b/MdePkg/Library/BaseLib/BaseLibInternals.h >> >> > > > index ea387ce37d27..9dca97a0dcc9 100644 >> >> > > > --- a/MdePkg/Library/BaseLib/BaseLibInternals.h >> >> > > > +++ b/MdePkg/Library/BaseLib/BaseLibInternals.h >> >> > > > @@ -340,34 +340,6 @@ InternalSwitchStack ( >> >> > > > >> >> > > > >> >> > > > /** >> >> > > > - Worker function that locates the Node in the List. >> >> > > > - >> >> > > > - By searching the List, finds the location of the Node in Lis= t. >> >> > > > At the same time, >> >> > > > - verifies the validity of this list. >> >> > > > - >> >> > > > - If List is NULL, then ASSERT(). >> >> > > > - If List->ForwardLink is NULL, then ASSERT(). >> >> > > > - If List->backLink is NULL, then ASSERT(). >> >> > > > - If Node is NULL, then ASSERT(); >> >> > > > - If PcdMaximumLinkedListLength is not zero, and prior to >> >> > > > insertion the number >> >> > > > - of nodes in ListHead, including the ListHead node, is >> >> > > > greater than or >> >> > > > - equal to PcdMaximumLinkedListLength, then ASSERT(). >> >> > > > - >> >> > > > - @param List A pointer to a node in a linked list. >> >> > > > - @param Node A pointer to one nod. >> >> > > > - >> >> > > > - @retval TRUE Node is in List. >> >> > > > - @retval FALSE Node isn't in List, or List is invalid. >> >> > > > - >> >> > > > -**/ >> >> > > > -BOOLEAN >> >> > > > -EFIAPI >> >> > > > -IsNodeInList ( >> >> > > > - IN CONST LIST_ENTRY *List, >> >> > > > - IN CONST LIST_ENTRY *Node >> >> > > > - ); >> >> > > > - >> >> > > > -/** >> >> > > > Worker function that returns a bit field from Operand. >> >> > > > >> >> > > > Returns the bitfield specified by the StartBit and the >> >> > > > EndBit from Operand. >> >> > > > -- >> >> > > > 2.12.2.windows.2 >> >> > >> >> > _______________________________________________ >> >> > edk2-devel mailing list >> >> > edk2-devel@lists.01.org >> >> > https://lists.01.org/mailman/listinfo/edk2-devel >> >_______________________________________________ >> >edk2-devel mailing list >> >edk2-devel@lists.01.org >> >https://lists.01.org/mailman/listinfo/edk2-devel