From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR03-AM5-obe.outbound.protection.outlook.com (mail-oln040092070100.outbound.protection.outlook.com [40.92.70.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8EF6521D491B6 for ; Wed, 2 Aug 2017 05:21:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=rhQSk5g4LMUiaNB1Py75HENhPqWv+v0Fzvc9LEjXyB0=; b=Ujchs+capcClb4LLzCDsT3EcYDlUOC1S3zA0jLNhvME73QsMbN424xqTEzpGRZDbXAfPoE8Afq7hPAI0LeT1+1Lbpy2rXTput7T4oAca6KkRxhSW9EPQZeclIY9HXsOaHAeZdoJ56x0jVMVLGSl/V0TQtDQ2bIILHsoeml5v6IOYuzk6JoHyCq06AQ5H4cud7+2amLQIabliquL+879YVOQnChqxpiwOMoqLoz0cWmZ9EXE+aDvJjAX1Ixsm6WJMFAL7TPlCGXL1epFmzcNe8LwMKtSxQhHVJha1wakJXbwqqhPM3jFJaWMZn76lU2ax8MD7Nqbxxa6N+TvJiTQcDg== Received: from AM5EUR03FT007.eop-EUR03.prod.protection.outlook.com (10.152.16.55) by AM5EUR03HT010.eop-EUR03.prod.protection.outlook.com (10.152.16.67) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.1282.16; Wed, 2 Aug 2017 12:23:43 +0000 Received: from AM4PR06MB1491.eurprd06.prod.outlook.com (10.152.16.53) by AM5EUR03FT007.mail.protection.outlook.com (10.152.16.145) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1282.16 via Frontend Transport; Wed, 2 Aug 2017 12:23:42 +0000 Received: from AM4PR06MB1491.eurprd06.prod.outlook.com ([fe80::dcd0:60e6:90e0:871b]) by AM4PR06MB1491.eurprd06.prod.outlook.com ([fe80::dcd0:60e6:90e0:871b%13]) with mapi id 15.01.1304.023; Wed, 2 Aug 2017 12:23:42 +0000 From: =?iso-8859-1?Q?Marvin_H=E4user?= To: "edk2-devel@lists.01.org" CC: "Gao, Liming" , "michael.d.kinney@intel.com" Thread-Topic: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function. Thread-Index: AQHTA5vus1udQdORhEegPjUTqWKZfaJjLSJAgAADdaCABJ2G8IAAEHiQgAkqlhCAAAE7IA== Date: Wed, 2 Aug 2017 12:23:42 +0000 Message-ID: References: <4A89E2EF3DFEDB4C8BFDE51014F606A14D75CC1D@shsmsx102.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D75EAD3@shsmsx102.ccr.corp.intel.com> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14D75EAD3@shsmsx102.ccr.corp.intel.com> Accept-Language: de-DE, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: lists.01.org; dkim=none (message not signed) header.d=none;lists.01.org; dmarc=none action=none header.from=outlook.com; x-incomingtopheadermarker: OriginalChecksum:A0F09CEA013AC6B5A64606BE13C1B5A25E387E158CAEF28517DB26A791F17E95; UpperCasedChecksum:F770E6939487FD1AC800989223B94F4F00605A9D310DF46F3CCA6CA13DF21A4F; SizeAsReceived:7849; Count:45 x-tmn: [nY4fskGz8EKXKpYOGA8eBtVGNr5Jwivo2RFy5if4CZ6VmZ8e+j3wsCOEyf3uHLkh] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; AM5EUR03HT010; 7:bflf3AbIvUqBWoBY88ZZuCNXWgxPlMtBk/mJowM/cSZ4skjGpNuADNyU3DuU2aH7Ls8VtFI5fkbR5rrjenmAUBz5SswnVia9ylNWBsVxWK8fJNXsnunHB67K6awyfIFxMjc9UVfoWDdq0AQjWdvcI3Qs0Z4UvdMem30o9SJCbFFbe9IA0+1RktL9I2LHYT8Ux7FlzNqCaBeUk2jLBGWmjSWWdSDHU1UxDz9uvQPp+dSe0Ul84bAN4tTSioshfVIqzXevBnun7QcZikBP1uicvQ2+r3Z9Qs9xqbELTp4el96dK5thhIkxv5DzcngRPcsq/zuAODchCyqyKdWE6ZEryHzQnY03r0KFz5AFO7Ar7i3CxwCnxfmQD7tbLUX9Ee7yHxCT5XW0JehrmnRi9LEgTqWyEfNHv8MOI/THESyfSzFvEorFtWH7ikyxHddM27GVPZbDkstM/HnrYuaWZf9W68tfU8Kry68sZvuE5u3WwSmcve/ZEpy73qBHfPonDo1S5vcAnqU1yQvjbUTihJ9mcsv05aNwtS+hIVRKDzUDpFVzAi9YIhelb4Fb3Qce/Pe8GY0zxxtHvyeUjNoPxgh5X7kG+YIYcH080t58MJAucwGStCdgmf48dLkiwzKRflVxuZV8uwLpmvUsuaMpH4Ig2c5pC5Pw9UXbH1N+BwKqp0N+ULVqovyn4zKBusKBDjFJB/qKFfqFD9PGI48kW7+ErXJLEirymIpCnqtvEiwp01Xlg5npC1vqehwO4ybBz5X81UaLzNoYFGXGWC24FpQmvw== x-incomingheadercount: 45 x-eopattributedmessage: 0 x-forefront-antispam-report: EFV:NLI; SFV:NSPM; SFS:(7070007)(98901004); DIR:OUT; SFP:1901; SCL:1; SRVR:AM5EUR03HT010; H:AM4PR06MB1491.eurprd06.prod.outlook.com; FPR:; SPF:None; LANG:en; x-ms-office365-filtering-correlation-id: 9016012d-1014-4de2-2283-08d4d9a1503f x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(300000500095)(300135000095)(300000501095)(300135300095)(22001)(300000502095)(300135100095)(300000503095)(300135400095)(201702061074)(5061506573)(5061507331)(1603103135)(2017031320274)(2017031324274)(2017031323274)(2017031322377)(1601125374)(1603101448)(1701031045)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095); SRVR:AM5EUR03HT010; x-ms-traffictypediagnostic: AM5EUR03HT010: x-exchange-antispam-report-test: UriScan:(166708455590820)(189930954265078)(162533806227266)(228905959029699); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(444000031); SRVR:AM5EUR03HT010; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:AM5EUR03HT010; x-forefront-prvs: 0387D64A71 spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Aug 2017 12:23:42.4421 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Internet X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5EUR03HT010 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:21:46 -0000 Content-Language: de-DE Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Hey Liming, Thanks for your reply! Sure, that works as well, though usually I prefer to= 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 AS= SERT ((!FeaturePcdGet (PcdVerifyNodeInList) || IsNodeInList (FirstEntry, Se= condEntry)) && InternalBaseLibIsListValid (FirstEntry)); ? Correct me if I'm wrong, but wouldn't your suggestion not ASSERT if the fea= ture is enabled, the entry is not in the list, but the list is valid? ((TRU= E && 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. >=20 > Marvin: > How about directly use PCD checker with IsNodeInList()? >=20 > ASSERT (IsNodeInList (FirstEntry, SecondEntry)); =3D=3D> ASSERT > (FeaturePcdGet (PcdVerifyNodeInList) && IsNodeInList (FirstEntry, > SecondEntry))|| InternalBaseLibIsListValid (FirstEntry)); >=20 > 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 value= 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() function= . > >> > > > > >> > > > 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 List= . > >> > > > 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