From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-oln040092065095.outbound.protection.outlook.com [40.92.65.95]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9FF3421D49180 for ; Thu, 27 Jul 2017 09:17:36 -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=CpI8G+V93Xg5Am+eSxtApC/p2jhifXszHxTUilv6Mqs=; b=H3+37EXTdDIw6ULEmKGoAKfmwggoiaO4NJt92NAIpGuqh1ZyiB3u3QsWPI4B6rAdGZWAS8ddBnHJO6KbppvJMke4SNsQP3+HsWaIXQ+BX/OSWhnVZUeAZk+9iw6oCJaRb+DwVreAsKw7Vf/vAs89TqEvYNZF85EU/EehBf8TbadmoZAyv6V1GNOI00OYeqv/RMzPZyGdQupCZ6/qD3qZz0i+hTaIpg5q5wjh6J2EIdiLFcFOzSpNmOCKqPncOZOvTEJ0tzacmwQj/1Nb54Ng+QseM+Kusld7mywMMuFizb25yd3osfZX6TEPaCvZxRi2DV2KSPcfHQY9bO5Ggxfh8g== Received: from HE1EUR01FT053.eop-EUR01.prod.protection.outlook.com (10.152.0.59) by HE1EUR01HT003.eop-EUR01.prod.protection.outlook.com (10.152.0.63) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.1240.9; Thu, 27 Jul 2017 16:19:37 +0000 Received: from AM4PR06MB1491.eurprd06.prod.outlook.com (10.152.0.56) by HE1EUR01FT053.mail.protection.outlook.com (10.152.1.73) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1240.9 via Frontend Transport; Thu, 27 Jul 2017 16:19:37 +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.1282.021; Thu, 27 Jul 2017 16:19:37 +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: AQHTA5vus1udQdORhEegPjUTqWKZfaJjLSJAgAADdaCABJ2G8IAAEHiQ Date: Thu, 27 Jul 2017 16:19:37 +0000 Message-ID: References: <4A89E2EF3DFEDB4C8BFDE51014F606A14D75CC1D@shsmsx102.ccr.corp.intel.com> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14D75CC1D@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:34B5B2BF50D947E3791114BF2B565EDC86FE270D1442CA27D1BC2DB8506094F0; UpperCasedChecksum:D678F30E86BB9A80590564F81176CB7D8B77A982739319722EEDDD7D37BFE15F; SizeAsReceived:7679; Count:45 x-tmn: [FI0stkOEpAjPk9SMe+1Qyyfyh/Alk+AosO3eZyfKf3xuEqR3U5/Xla9KmM4EKoU5] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; HE1EUR01HT003; 7:5AbveodQwv8rQ/fH6RnckwcJEmR7mmhT8gy2LOBo4TXRF1zrAVvINP0DOTlpZO2mBPvlu5bmvcJsbmCUuZxApdZ4b/uMyitsu/7wDRMndLX/rRsd65aKJinQ+uAhFqqrKwhcorJddg1tdaI/T3j7e5GU3qvCSICks0Cjh+n7EvE+t+MFrVIh2AVJN2FECpW1r19wJSoBL7Xdyt2aN4Abf5peuxCPxU3d3zM3ri16CJK0vGwljLZxO2y9TNWcvlF0RlTUBVuuCPUr2/XNRLZllfXh7BZ/3bK2q+Y8KJEPSzc418hZVjbBnRrzzaRkLNAYy3tEs1LK5oFagBLPFq7AyeH2XkRLoJqGHx+aW2p9vs9Cq/Qy1hIKuIadbKJkPpnhQmOllTVIAoZAyIMdEjgUOwLOClf+QoYne4YitKH9ks6K+1/Ogg45S26/mbS41IsAPeZAUCqjqQ51lJ7+7hw3auGt8opNu/Jcsfq7as998Eq5e8Kt90wXgdFjpxwwrjMkWHuLJjPomsfGhbHL/uqNlmp/ozztAeVDgWU2bsaU8zhowyokwy/r6x0wDdVe3h2cceju95hKnQruu1txXcHPf9LEw8OjtV6mBGWt5VIEFGSzvJePnDQkkbc5cL3mIsqxew3HkcC0QZkLXWKQCu+vbPYUr31C2GFIzOEKxapTX3NfYqDY6PwmhQkjBoO/EkOBLOjGP+DTp9jwqf5pKOZLVakn1UmBn4nxXy6q4lZXYg0TjAKuaNwN3tIm3xrFfK7ptHsS026NJR1OmbTUZ2iYFw== x-incomingheadercount: 45 x-eopattributedmessage: 0 x-forefront-antispam-report: EFV:NLI; SFV:NSPM; SFS:(7070007)(98901004); DIR:OUT; SFP:1901; SCL:1; SRVR:HE1EUR01HT003; H:AM4PR06MB1491.eurprd06.prod.outlook.com; FPR:; SPF:None; LANG:en; x-ms-office365-filtering-correlation-id: 2eea1898-2a63-47da-37e2-08d4d50b46cb 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)(2017031322350)(1601125374)(1603101448)(1701031045)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095); SRVR:HE1EUR01HT003; x-ms-traffictypediagnostic: HE1EUR01HT003: 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:HE1EUR01HT003; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:HE1EUR01HT003; x-forefront-prvs: 03818C953D spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Jul 2017 16:19:37.3500 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Internet X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1EUR01HT003 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: Thu, 27 Jul 2017 16:17:37 -0000 Content-Language: de-DE Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable 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. >=20 > 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/KabylakeSilic= onPkg/Pch/PchSmiDispatcher/Smm/IoTrap.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 th= is location may just be overwritten. The code linked currently doesn't seem to do anything as ForwardLink is nev= er 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 RemoveEntr= yList() does set the Links to EFI_BAD_POINTER. >=20 > 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 o= r a macro to do the PCD check and following the ASSERTin v2? >=20 > 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