From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <liming.gao@intel.com>
Received: from mga06.intel.com (mga06.intel.com [134.134.136.31])
(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 69C2521D491A7
for <edk2-devel@lists.01.org>; Wed, 2 Aug 2017 05:10:16 -0700 (PDT)
Received: from orsmga001.jf.intel.com ([10.7.209.18])
by orsmga104.jf.intel.com with ESMTP; 02 Aug 2017 05:12:26 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.41,311,1498546800"; d="scan'208";a="1158194613"
Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201])
by orsmga001.jf.intel.com with ESMTP; 02 Aug 2017 05:12:26 -0700
Received: from fmsmsx102.amr.corp.intel.com (10.18.124.200) by
FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS)
id 14.3.319.2; Wed, 2 Aug 2017 05:12:26 -0700
Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by
FMSMSX102.amr.corp.intel.com (10.18.124.200) with Microsoft SMTP Server (TLS)
id 14.3.319.2; Wed, 2 Aug 2017 05:12:25 -0700
Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.146]) by
SHSMSX104.ccr.corp.intel.com ([10.239.4.70]) with mapi id 14.03.0319.002;
Wed, 2 Aug 2017 20:12:24 +0800
From: "Gao, Liming" <liming.gao@intel.com>
To: "Marvin.Haeuser@outlook.com" <Marvin.Haeuser@outlook.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
CC: "Kinney, Michael D" <michael.d.kinney@intel.com>
Thread-Topic: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
Thread-Index: AQHTA5vus1udQdORhEegPjUTqWKZfaJjLSJAgAADdaCABJ2G8IAAEHiQgAkqlhA=
Date: Wed, 2 Aug 2017 12:12:24 +0000
Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D75EAD3@shsmsx102.ccr.corp.intel.com>
References: <AM4PR06MB14919843FB4DC32CDAF80FE280BA0@AM4PR06MB1491.eurprd06.prod.outlook.com>
<E92EE9817A31E24EB0585FDF735412F5A7D5E7B4@ORSMSX113.amr.corp.intel.com>
<AM4PR06MB14919E4F7E5930A082E9848D80BB0@AM4PR06MB1491.eurprd06.prod.outlook.com>
<4A89E2EF3DFEDB4C8BFDE51014F606A14D75CC1D@shsmsx102.ccr.corp.intel.com>
<AM4PR06MB1491DE4D337B3FD321B7240A80BE0@AM4PR06MB1491.eurprd06.prod.outlook.com>
In-Reply-To: <AM4PR06MB1491DE4D337B3FD321B7240A80BE0@AM4PR06MB1491.eurprd06.prod.outlook.com>
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 <edk2-devel.lists.01.org>
List-Unsubscribe: <https://lists.01.org/mailman/options/edk2-devel>,
<mailto:edk2-devel-request@lists.01.org?subject=unsubscribe>
List-Archive: <http://lists.01.org/pipermail/edk2-devel/>
List-Post: <mailto:edk2-devel@lists.01.org>
List-Help: <mailto:edk2-devel-request@lists.01.org?subject=help>
List-Subscribe: <https://lists.01.org/mailman/listinfo/edk2-devel>,
<mailto:edk2-devel-request@lists.01.org?subject=subscribe>
X-List-Received-Date: Wed, 02 Aug 2017 12:10:16 -0000
Content-Language: en-US
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Marvin:
How about directly use PCD checker with IsNodeInList()?
ASSERT (IsNodeInList (FirstEntry, SecondEntry));
=3D=3D>
ASSERT (FeaturePcdGet (PcdVerifyNodeInList) && IsNodeInList (FirstEntry, Se=
condEntry))|| 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 <michael.d.kinney@intel.com>; Gao, Liming
><liming.gao@intel.com>
>Subject: Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() functio=
n.
>
>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 <michael.d.kinney@intel.com>
>> 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/Io
>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 t=
his
>location may just be overwritten.
>The code linked currently doesn't seem to do anything as ForwardLink is ne=
ver
>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 o=
f
>> 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 <michael.d.kinney@intel.com>
>> > 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 pas=
s 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 <Marvin.Haeuser@outlook.com>; edk2-
>> > > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
>> > > Cc: Gao, Liming <liming.gao@intel.com>
>> > > 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 <michael.d.kinney@intel.com>; Gao, Liming
>> > > > <liming.gao@intel.com>
>> > > > 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 <Marvin.Haeuser@outlook.com>
>> > > > ---
>> > > > 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.<BR>
>> > > > + Copyright (c) 2006 - 2017, Intel Corporation. All rights
>> > > > reserved.<BR>
>> > > > 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