From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.88; helo=mga01.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org 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 590D9203564AF for ; Wed, 29 Nov 2017 04:31:48 -0800 (PST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Nov 2017 04:36:11 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,473,1505804400"; d="scan'208";a="181926825" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga006.fm.intel.com with ESMTP; 29 Nov 2017 04:36:11 -0800 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 29 Nov 2017 04:36:11 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 29 Nov 2017 04:36:11 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.175]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.159]) with mapi id 14.03.0319.002; Wed, 29 Nov 2017 20:36:09 +0800 From: "Zeng, Star" To: Heyi Guo , "linaro-uefi@lists.linaro.org" , "edk2-devel@lists.01.org" CC: "Dong, Eric" , "Gao, Liming" , "Zeng, Star" Thread-Topic: [PATCH] MdeModulePkg/PerformanceLib: add lock protection Thread-Index: AQHTZzBeymyeNMbnUkW6FK4yEjpyuKMrTpmg Date: Wed, 29 Nov 2017 12:36:08 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B9BD8F3@shsmsx102.ccr.corp.intel.com> References: <1511753504-38548-1-git-send-email-heyi.guo@linaro.org> In-Reply-To: <1511753504-38548-1-git-send-email-heyi.guo@linaro.org> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg/PerformanceLib: add lock protection 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, 29 Nov 2017 12:31:48 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Pushed the patch at f1f7190bf3bf932b52287258a65c366ed5bdce13 after updating= the title to "MdeModulePkg/DxeCorePerformanceLib: add lock protection" and= removing a redundant "with". Thanks, Star -----Original Message----- From: Heyi Guo [mailto:heyi.guo@linaro.org]=20 Sent: Monday, November 27, 2017 11:32 AM To: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org Cc: Heyi Guo ; Zeng, Star ; Dong,= Eric ; Gao, Liming Subject: [PATCH] MdeModulePkg/PerformanceLib: add lock protection DXE performance gauge record access functions might be reentered since we a= re supporting something like USB hot-plug, which is a timer event where gBS= ->ConnectController might be called and then PERF will be called in CoreCon= nectSingleController. When StartGaugeEx is being reentered, not only the gauge record might be ov= erwritten, more serious situation will be caused if gauge data buffer reall= ocation procedure is interrupted, between line 180 and 187 in DxeCorePerfor= manceLib.c specifically. There, mMaxGaugeRecords will be doubled twice (den= oted as 4X), but mGaugeData only points to a buffer of size 2X, which will = probably cause the following 2X memory to be overflowed when gauge records = are increased. So we add EFI lock with with TPL_NOTIFY in StartGaugeEx/EndGaugeEx/GetGauge= Ex to avoid memory overflow and gauge data corruption. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Heyi Guo Cc: Star Zeng Cc: Eric Dong Cc: Liming Gao --- .../DxeCorePerformanceLib/DxeCorePerformanceLib.c | 67 ++++++++++++++++++= +++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceL= ib.c b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c index 51f488a..7c0e207 100644 --- a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c +++ b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c @@ -63,6 +63,11 @@ PERFORMANCE_EX_PROTOCOL mPerformanceExInterface =3D { =20 PERFORMANCE_PROPERTY mPerformanceProperty; =20 +// +// Gauge record lock to avoid data corruption or even memory overflow=20 +// STATIC EFI_LOCK mPerfRecordLock =3D EFI_INITIALIZE_LOCK_VARIABLE=20 +(TPL_NOTIFY); + /** Searches in the gauge array with keyword Handle, Token, Module and Ident= ifier. =20 @@ -162,6 +167,12 @@ StartGaugeEx ( UINTN OldGaugeDataSize; GAUGE_DATA_HEADER *OldGaugeData; UINT32 Index; + EFI_STATUS Status; + + Status =3D EfiAcquireLockOrFail (&mPerfRecordLock); if (EFI_ERROR=20 + (Status)) { + return Status; + } =20 Index =3D mGaugeData->NumberOfEntries; if (Index >=3D mMaxGaugeRecords) { @@ -175,6 +186,7 @@ StartGaugeEx ( =20 NewGaugeData =3D AllocateZeroPool (GaugeDataSize); if (NewGaugeData =3D=3D NULL) { + EfiReleaseLock (&mPerfRecordLock); return EFI_OUT_OF_RESOURCES; } =20 @@ -209,6 +221,8 @@ StartGaugeEx ( =20 mGaugeData->NumberOfEntries++; =20 + EfiReleaseLock (&mPerfRecordLock); + return EFI_SUCCESS; } =20 @@ -250,6 +264,12 @@ EndGaugeEx ( { GAUGE_DATA_ENTRY_EX *GaugeEntryExArray; UINT32 Index; + EFI_STATUS Status; + + Status =3D EfiAcquireLockOrFail (&mPerfRecordLock); if (EFI_ERROR=20 + (Status)) { + return Status; + } =20 if (TimeStamp =3D=3D 0) { TimeStamp =3D GetPerformanceCounter (); @@ -257,11 +277,13 @@ EndGauge= Ex ( =20 Index =3D InternalSearchForGaugeEntry (Handle, Token, Module, Identifier= ); if (Index >=3D mGaugeData->NumberOfEntries) { + EfiReleaseLock (&mPerfRecordLock); return EFI_NOT_FOUND; } GaugeEntryExArray =3D (GAUGE_DATA_ENTRY_EX *) (mGaugeData + 1); GaugeEntryExArray[Index].EndTimeStamp =3D TimeStamp; =20 + EfiReleaseLock (&mPerfRecordLock); return EFI_SUCCESS; } =20 @@ -274,6 +296,8 @@ EndGaugeEx ( If it stands for a valid entry, then EFI_SUCCESS is returned and GaugeDataEntryEx stores the pointer to that entry. =20 + This internal function is added to avoid releasing lock before each retu= rn statement. + @param LogEntryKey The key for the previous performance mea= surement log entry. If 0, then the first performance measure= ment log entry is retrieved. @param GaugeDataEntryEx The indirect pointer to the extended gau= ge data entry specified by LogEntryKey @@ -287,7 +311,7 @@ EndGaugeEx ( **/ EFI_STATUS EFIAPI -GetGaugeEx ( +InternalGetGaugeEx ( IN UINTN LogEntryKey, OUT GAUGE_DATA_ENTRY_EX **GaugeDataEntryEx ) @@ -314,6 +338,47 @@ GetGaugeEx ( } =20 /** + Retrieves a previously logged performance measurement. + It can also retrieve the log created by StartGauge and EndGauge of=20 + PERFORMANCE_PROTOCOL, and then assign the Identifier with 0. + + Retrieves the performance log entry from the performance log specified b= y LogEntryKey. + If it stands for a valid entry, then EFI_SUCCESS is returned and =20 + GaugeDataEntryEx stores the pointer to that entry. + + @param LogEntryKey The key for the previous performance mea= surement log entry. + If 0, then the first performance measure= ment log entry is retrieved. + @param GaugeDataEntryEx The indirect pointer to the extended gau= ge data entry specified by LogEntryKey + if the retrieval is successful. + + @retval EFI_SUCCESS The GuageDataEntryEx is successfully fou= nd based on LogEntryKey. + @retval EFI_NOT_FOUND The LogEntryKey is the last entry (equal= s to the total entry number). + @retval EFI_INVALIDE_PARAMETER The LogEntryKey is not a valid entry (gr= eater than the total entry number). + @retval EFI_INVALIDE_PARAMETER GaugeDataEntryEx is NULL. + +**/ +EFI_STATUS +EFIAPI +GetGaugeEx ( + IN UINTN LogEntryKey, + OUT GAUGE_DATA_ENTRY_EX **GaugeDataEntryEx + ) +{ + EFI_STATUS Status; + + Status =3D EfiAcquireLockOrFail (&mPerfRecordLock); if (EFI_ERROR=20 + (Status)) { + return Status; + } + + Status =3D InternalGetGaugeEx (LogEntryKey, GaugeDataEntryEx); + + EfiReleaseLock (&mPerfRecordLock); + + return Status; +} + +/** Adds a record at the end of the performance measurement log that records the start time of a performance measurement. =20 -- 2.7.4