* [PATCH] MdeModulePkg/PerformanceLib: add lock protection
@ 2017-11-27 3:31 Heyi Guo
2017-11-27 11:02 ` Zeng, Star
2017-11-29 12:36 ` Zeng, Star
0 siblings, 2 replies; 3+ messages in thread
From: Heyi Guo @ 2017-11-27 3:31 UTC (permalink / raw)
To: linaro-uefi, edk2-devel; +Cc: Heyi Guo, Star Zeng, Eric Dong, Liming Gao
DXE performance gauge record access functions might be reentered since
we are supporting something like USB hot-plug, which is a timer event
where gBS->ConnectController might be called and then PERF will be
called in CoreConnectSingleController.
When StartGaugeEx is being reentered, not only the gauge record might
be overwritten, more serious situation will be caused if gauge data
buffer reallocation procedure is interrupted, between line 180 and 187
in DxeCorePerformanceLib.c specifically. There, mMaxGaugeRecords will
be doubled twice (denoted 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/GetGaugeEx to avoid memory overflow and gauge
data corruption.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
---
.../DxeCorePerformanceLib/DxeCorePerformanceLib.c | 67 +++++++++++++++++++++-
1 file changed, 66 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.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 = {
PERFORMANCE_PROPERTY mPerformanceProperty;
+//
+// Gauge record lock to avoid data corruption or even memory overflow
+//
+STATIC EFI_LOCK mPerfRecordLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_NOTIFY);
+
/**
Searches in the gauge array with keyword Handle, Token, Module and Identifier.
@@ -162,6 +167,12 @@ StartGaugeEx (
UINTN OldGaugeDataSize;
GAUGE_DATA_HEADER *OldGaugeData;
UINT32 Index;
+ EFI_STATUS Status;
+
+ Status = EfiAcquireLockOrFail (&mPerfRecordLock);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
Index = mGaugeData->NumberOfEntries;
if (Index >= mMaxGaugeRecords) {
@@ -175,6 +186,7 @@ StartGaugeEx (
NewGaugeData = AllocateZeroPool (GaugeDataSize);
if (NewGaugeData == NULL) {
+ EfiReleaseLock (&mPerfRecordLock);
return EFI_OUT_OF_RESOURCES;
}
@@ -209,6 +221,8 @@ StartGaugeEx (
mGaugeData->NumberOfEntries++;
+ EfiReleaseLock (&mPerfRecordLock);
+
return EFI_SUCCESS;
}
@@ -250,6 +264,12 @@ EndGaugeEx (
{
GAUGE_DATA_ENTRY_EX *GaugeEntryExArray;
UINT32 Index;
+ EFI_STATUS Status;
+
+ Status = EfiAcquireLockOrFail (&mPerfRecordLock);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
if (TimeStamp == 0) {
TimeStamp = GetPerformanceCounter ();
@@ -257,11 +277,13 @@ EndGaugeEx (
Index = InternalSearchForGaugeEntry (Handle, Token, Module, Identifier);
if (Index >= mGaugeData->NumberOfEntries) {
+ EfiReleaseLock (&mPerfRecordLock);
return EFI_NOT_FOUND;
}
GaugeEntryExArray = (GAUGE_DATA_ENTRY_EX *) (mGaugeData + 1);
GaugeEntryExArray[Index].EndTimeStamp = TimeStamp;
+ EfiReleaseLock (&mPerfRecordLock);
return EFI_SUCCESS;
}
@@ -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.
+ This internal function is added to avoid releasing lock before each return statement.
+
@param LogEntryKey The key for the previous performance measurement log entry.
If 0, then the first performance measurement log entry is retrieved.
@param GaugeDataEntryEx The indirect pointer to the extended gauge 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 (
}
/**
+ Retrieves a previously logged performance measurement.
+ It can also retrieve the log created by StartGauge and EndGauge of PERFORMANCE_PROTOCOL,
+ and then assign the Identifier with 0.
+
+ Retrieves the performance log entry from the performance log specified by LogEntryKey.
+ If it stands for a valid entry, then EFI_SUCCESS is returned and
+ GaugeDataEntryEx stores the pointer to that entry.
+
+ @param LogEntryKey The key for the previous performance measurement log entry.
+ If 0, then the first performance measurement log entry is retrieved.
+ @param GaugeDataEntryEx The indirect pointer to the extended gauge data entry specified by LogEntryKey
+ if the retrieval is successful.
+
+ @retval EFI_SUCCESS The GuageDataEntryEx is successfully found based on LogEntryKey.
+ @retval EFI_NOT_FOUND The LogEntryKey is the last entry (equals to the total entry number).
+ @retval EFI_INVALIDE_PARAMETER The LogEntryKey is not a valid entry (greater 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 = EfiAcquireLockOrFail (&mPerfRecordLock);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = 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.
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] MdeModulePkg/PerformanceLib: add lock protection
2017-11-27 3:31 [PATCH] MdeModulePkg/PerformanceLib: add lock protection Heyi Guo
@ 2017-11-27 11:02 ` Zeng, Star
2017-11-29 12:36 ` Zeng, Star
1 sibling, 0 replies; 3+ messages in thread
From: Zeng, Star @ 2017-11-27 11:02 UTC (permalink / raw)
To: Heyi Guo, linaro-uefi@lists.linaro.org, edk2-devel@lists.01.org
Cc: Dong, Eric, Gao, Liming, Zeng, Star
Reviewed-by: Star Zeng <star.zeng@intel.com> with the redundant with at " with with " in the commit log.
Thanks,
Star
-----Original Message-----
From: Heyi Guo [mailto:heyi.guo@linaro.org]
Sent: Monday, November 27, 2017 11:32 AM
To: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org
Cc: Heyi Guo <heyi.guo@linaro.org>; Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: [PATCH] MdeModulePkg/PerformanceLib: add lock protection
DXE performance gauge record access functions might be reentered since we are supporting something like USB hot-plug, which is a timer event where gBS->ConnectController might be called and then PERF will be called in CoreConnectSingleController.
When StartGaugeEx is being reentered, not only the gauge record might be overwritten, more serious situation will be caused if gauge data buffer reallocation procedure is interrupted, between line 180 and 187 in DxeCorePerformanceLib.c specifically. There, mMaxGaugeRecords will be doubled twice (denoted 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/GetGaugeEx to avoid memory overflow and gauge data corruption.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
---
.../DxeCorePerformanceLib/DxeCorePerformanceLib.c | 67 +++++++++++++++++++++-
1 file changed, 66 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.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 = {
PERFORMANCE_PROPERTY mPerformanceProperty;
+//
+// Gauge record lock to avoid data corruption or even memory overflow
+// STATIC EFI_LOCK mPerfRecordLock = EFI_INITIALIZE_LOCK_VARIABLE
+(TPL_NOTIFY);
+
/**
Searches in the gauge array with keyword Handle, Token, Module and Identifier.
@@ -162,6 +167,12 @@ StartGaugeEx (
UINTN OldGaugeDataSize;
GAUGE_DATA_HEADER *OldGaugeData;
UINT32 Index;
+ EFI_STATUS Status;
+
+ Status = EfiAcquireLockOrFail (&mPerfRecordLock); if (EFI_ERROR
+ (Status)) {
+ return Status;
+ }
Index = mGaugeData->NumberOfEntries;
if (Index >= mMaxGaugeRecords) {
@@ -175,6 +186,7 @@ StartGaugeEx (
NewGaugeData = AllocateZeroPool (GaugeDataSize);
if (NewGaugeData == NULL) {
+ EfiReleaseLock (&mPerfRecordLock);
return EFI_OUT_OF_RESOURCES;
}
@@ -209,6 +221,8 @@ StartGaugeEx (
mGaugeData->NumberOfEntries++;
+ EfiReleaseLock (&mPerfRecordLock);
+
return EFI_SUCCESS;
}
@@ -250,6 +264,12 @@ EndGaugeEx (
{
GAUGE_DATA_ENTRY_EX *GaugeEntryExArray;
UINT32 Index;
+ EFI_STATUS Status;
+
+ Status = EfiAcquireLockOrFail (&mPerfRecordLock); if (EFI_ERROR
+ (Status)) {
+ return Status;
+ }
if (TimeStamp == 0) {
TimeStamp = GetPerformanceCounter (); @@ -257,11 +277,13 @@ EndGaugeEx (
Index = InternalSearchForGaugeEntry (Handle, Token, Module, Identifier);
if (Index >= mGaugeData->NumberOfEntries) {
+ EfiReleaseLock (&mPerfRecordLock);
return EFI_NOT_FOUND;
}
GaugeEntryExArray = (GAUGE_DATA_ENTRY_EX *) (mGaugeData + 1);
GaugeEntryExArray[Index].EndTimeStamp = TimeStamp;
+ EfiReleaseLock (&mPerfRecordLock);
return EFI_SUCCESS;
}
@@ -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.
+ This internal function is added to avoid releasing lock before each return statement.
+
@param LogEntryKey The key for the previous performance measurement log entry.
If 0, then the first performance measurement log entry is retrieved.
@param GaugeDataEntryEx The indirect pointer to the extended gauge 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 (
}
/**
+ Retrieves a previously logged performance measurement.
+ It can also retrieve the log created by StartGauge and EndGauge of
+ PERFORMANCE_PROTOCOL, and then assign the Identifier with 0.
+
+ Retrieves the performance log entry from the performance log specified by LogEntryKey.
+ If it stands for a valid entry, then EFI_SUCCESS is returned and
+ GaugeDataEntryEx stores the pointer to that entry.
+
+ @param LogEntryKey The key for the previous performance measurement log entry.
+ If 0, then the first performance measurement log entry is retrieved.
+ @param GaugeDataEntryEx The indirect pointer to the extended gauge data entry specified by LogEntryKey
+ if the retrieval is successful.
+
+ @retval EFI_SUCCESS The GuageDataEntryEx is successfully found based on LogEntryKey.
+ @retval EFI_NOT_FOUND The LogEntryKey is the last entry (equals to the total entry number).
+ @retval EFI_INVALIDE_PARAMETER The LogEntryKey is not a valid entry (greater 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 = EfiAcquireLockOrFail (&mPerfRecordLock); if (EFI_ERROR
+ (Status)) {
+ return Status;
+ }
+
+ Status = 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.
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] MdeModulePkg/PerformanceLib: add lock protection
2017-11-27 3:31 [PATCH] MdeModulePkg/PerformanceLib: add lock protection Heyi Guo
2017-11-27 11:02 ` Zeng, Star
@ 2017-11-29 12:36 ` Zeng, Star
1 sibling, 0 replies; 3+ messages in thread
From: Zeng, Star @ 2017-11-29 12:36 UTC (permalink / raw)
To: Heyi Guo, linaro-uefi@lists.linaro.org, edk2-devel@lists.01.org
Cc: Dong, Eric, Gao, Liming, Zeng, Star
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]
Sent: Monday, November 27, 2017 11:32 AM
To: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org
Cc: Heyi Guo <heyi.guo@linaro.org>; Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming <liming.gao@intel.com>
Subject: [PATCH] MdeModulePkg/PerformanceLib: add lock protection
DXE performance gauge record access functions might be reentered since we are supporting something like USB hot-plug, which is a timer event where gBS->ConnectController might be called and then PERF will be called in CoreConnectSingleController.
When StartGaugeEx is being reentered, not only the gauge record might be overwritten, more serious situation will be caused if gauge data buffer reallocation procedure is interrupted, between line 180 and 187 in DxeCorePerformanceLib.c specifically. There, mMaxGaugeRecords will be doubled twice (denoted 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/GetGaugeEx to avoid memory overflow and gauge data corruption.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
---
.../DxeCorePerformanceLib/DxeCorePerformanceLib.c | 67 +++++++++++++++++++++-
1 file changed, 66 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.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 = {
PERFORMANCE_PROPERTY mPerformanceProperty;
+//
+// Gauge record lock to avoid data corruption or even memory overflow
+// STATIC EFI_LOCK mPerfRecordLock = EFI_INITIALIZE_LOCK_VARIABLE
+(TPL_NOTIFY);
+
/**
Searches in the gauge array with keyword Handle, Token, Module and Identifier.
@@ -162,6 +167,12 @@ StartGaugeEx (
UINTN OldGaugeDataSize;
GAUGE_DATA_HEADER *OldGaugeData;
UINT32 Index;
+ EFI_STATUS Status;
+
+ Status = EfiAcquireLockOrFail (&mPerfRecordLock); if (EFI_ERROR
+ (Status)) {
+ return Status;
+ }
Index = mGaugeData->NumberOfEntries;
if (Index >= mMaxGaugeRecords) {
@@ -175,6 +186,7 @@ StartGaugeEx (
NewGaugeData = AllocateZeroPool (GaugeDataSize);
if (NewGaugeData == NULL) {
+ EfiReleaseLock (&mPerfRecordLock);
return EFI_OUT_OF_RESOURCES;
}
@@ -209,6 +221,8 @@ StartGaugeEx (
mGaugeData->NumberOfEntries++;
+ EfiReleaseLock (&mPerfRecordLock);
+
return EFI_SUCCESS;
}
@@ -250,6 +264,12 @@ EndGaugeEx (
{
GAUGE_DATA_ENTRY_EX *GaugeEntryExArray;
UINT32 Index;
+ EFI_STATUS Status;
+
+ Status = EfiAcquireLockOrFail (&mPerfRecordLock); if (EFI_ERROR
+ (Status)) {
+ return Status;
+ }
if (TimeStamp == 0) {
TimeStamp = GetPerformanceCounter (); @@ -257,11 +277,13 @@ EndGaugeEx (
Index = InternalSearchForGaugeEntry (Handle, Token, Module, Identifier);
if (Index >= mGaugeData->NumberOfEntries) {
+ EfiReleaseLock (&mPerfRecordLock);
return EFI_NOT_FOUND;
}
GaugeEntryExArray = (GAUGE_DATA_ENTRY_EX *) (mGaugeData + 1);
GaugeEntryExArray[Index].EndTimeStamp = TimeStamp;
+ EfiReleaseLock (&mPerfRecordLock);
return EFI_SUCCESS;
}
@@ -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.
+ This internal function is added to avoid releasing lock before each return statement.
+
@param LogEntryKey The key for the previous performance measurement log entry.
If 0, then the first performance measurement log entry is retrieved.
@param GaugeDataEntryEx The indirect pointer to the extended gauge 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 (
}
/**
+ Retrieves a previously logged performance measurement.
+ It can also retrieve the log created by StartGauge and EndGauge of
+ PERFORMANCE_PROTOCOL, and then assign the Identifier with 0.
+
+ Retrieves the performance log entry from the performance log specified by LogEntryKey.
+ If it stands for a valid entry, then EFI_SUCCESS is returned and
+ GaugeDataEntryEx stores the pointer to that entry.
+
+ @param LogEntryKey The key for the previous performance measurement log entry.
+ If 0, then the first performance measurement log entry is retrieved.
+ @param GaugeDataEntryEx The indirect pointer to the extended gauge data entry specified by LogEntryKey
+ if the retrieval is successful.
+
+ @retval EFI_SUCCESS The GuageDataEntryEx is successfully found based on LogEntryKey.
+ @retval EFI_NOT_FOUND The LogEntryKey is the last entry (equals to the total entry number).
+ @retval EFI_INVALIDE_PARAMETER The LogEntryKey is not a valid entry (greater 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 = EfiAcquireLockOrFail (&mPerfRecordLock); if (EFI_ERROR
+ (Status)) {
+ return Status;
+ }
+
+ Status = 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.
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-11-29 12:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-27 3:31 [PATCH] MdeModulePkg/PerformanceLib: add lock protection Heyi Guo
2017-11-27 11:02 ` Zeng, Star
2017-11-29 12:36 ` Zeng, Star
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox