From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c01::22d; helo=mail-pl0-x22d.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pl0-x22d.google.com (mail-pl0-x22d.google.com [IPv6:2607:f8b0:400e:c01::22d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id F25BB21A04E2F for ; Sat, 25 Nov 2017 01:05:39 -0800 (PST) Received: by mail-pl0-x22d.google.com with SMTP id 61so5658417plf.4 for ; Sat, 25 Nov 2017 01:09:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language; bh=bFCBtSJUB3RDRk7RjaLN2gI0zQY1+dgh24uQxTKlXW8=; b=bHNxC2+FR6mBSDQETVwcS21Ru21SoLOkbEq60V00a1L34eMA+ZHuvIi1sCTQ3AjXMw 01cyJUTraZkucfEAPf1frRW1Qd85bj6GQ9uIUX/jTBO3cJb9lMS/VLTrDnRNpdTVULUq PGyYtUKe55GbHlhX+okvKyNepSE++k2VhAf7c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=bFCBtSJUB3RDRk7RjaLN2gI0zQY1+dgh24uQxTKlXW8=; b=gAY1Pqs5FvNwD4Pk6lQ9IqwbbjupVp5ktGiN3plonIJJnzEZLP4YRJpHEHIiHP8wLY +eIDMyfSkwtaJ/nN3G4/v/7WsfMywvG0B0kcjUvEZ80k6UTe8RnVVF/BhwY84LCH2hLr DMX9LeNh1Y2pUx+A8MBauQphf+2hpB+rWn/kBoEymA6CrOM+2x3+sx352NCpsjuCjm7B Nk3XDC9h/DkXCorf+rleA6iXCLDSVfa/02PZBQZIRjFsq+gtHHiqxZzx22zbHycbYBBS FdvDl6N9bSP8e569QxfZrv60gIPYorpWxvM7BHFyBNR1k17m1DRZZ4W3DMw2H/hCmpGe T+uA== X-Gm-Message-State: AJaThX65zQWr3WX1xHcdINbpqu+tyvV1qW11RwDMr58dFf+q4hLbD/dn c60o7HUyXEwV9RsbgfYj2I8iew== X-Google-Smtp-Source: AGs4zMbWXboPs0TotplgnIHgidzR3lpg5UB6+myprHY4CR0PtSgHezYphXrDCI4kodq6uc4nZ2FIJQ== X-Received: by 10.84.234.198 with SMTP id i6mr32168945plt.260.1511600998674; Sat, 25 Nov 2017 01:09:58 -0800 (PST) Received: from [10.152.199.178] ([104.237.91.79]) by smtp.gmail.com with ESMTPSA id d28sm45496876pfb.105.2017.11.25.01.09.56 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 25 Nov 2017 01:09:57 -0800 (PST) To: "Zeng, Star" , "edk2-devel@lists.01.org" Cc: "Carsey, Jaben" , Daryl McDaniel , "Gao, Liming" References: <6c7964be-bc2b-3ee4-e09e-c309fec7210f@linaro.org> <0C09AFA07DD0434D9E2A0C6AEB0483103B9BBCD9@shsmsx102.ccr.corp.intel.com> From: Heyi Guo Message-ID: Date: Sat, 25 Nov 2017 17:09:55 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B9BBCD9@shsmsx102.ccr.corp.intel.com> Subject: Re: [RFC] Add EFI lock when creating new gauge record 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: Sat, 25 Nov 2017 09:05:40 -0000 X-Groupsio-MsgNum: 17981 Content-Type: multipart/mixed; boundary="------------30B837547B94A4D728EC3F1C" Content-Language: en-US --------------30B837547B94A4D728EC3F1C Content-Type: text/plain; charset=gbk; format=flowed Content-Transfer-Encoding: 8bit Hi Star, Sorry to make a mistake with the code package :) Please see my test results below. ÔÚ 11/24/2017 5:41 PM, Zeng, Star дµÀ: > Good problem statement. > > EndGaugeEx and GetGaugeEx also need to be updated as they are operating same memory buffer, otherwise the performance data may be messed up. > > Another idea is to just use a global variable, for example mRecording, to prevent reentrance, but that there may be perf data missing. > > How to realize " not impacted much " in " the performance measuring data was not impacted much by this patch "? > That means there is a little performance impact? I tested more than 50 times on my platform with the original version and the modified one, and calculated the average "Total Duration", which were 32567ms and 32600ms respectively. So the performance impact almost can be ignored. > > Could you re-measure with all StartGaugeEx, EndGaugeEx and GetGaugeEx add lock? Good point. The above result was tested with all of them locked. I also attached my patch. Thanks and regards, Gary (Heyi Guo) > > > Cc Liming. > > > Thanks, > Star > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Heyi Guo > Sent: Friday, November 24, 2017 3:04 PM > To: edk2-devel@lists.01.org > Cc: Carsey, Jaben ; Daryl McDaniel > Subject: [edk2] [RFC] Add EFI lock when creating new gauge record > > Hi folks, > > We got occasional system exceptions after enabling performance measuring feature in edk2. After debugging, we found there is potential memory overflow in DXE/DXE_CORE PerformanceLib when PERF_START is reentered, and reentrance is possible 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. > However I can't reproduce the issue right now; please let me know if PERF reentrance is not theoretically possible in the latest edk2 code. > > 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. > > My proposal is to add EFI lock with TPL notify in StartGaugeEx to avoid such situation. The test result seemed good on our platforms and the performance measuring data was not impacted much by this patch. > > Please let me know your comments. > > Thanks, > > Gary (Heyi Guo) > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel --------------30B837547B94A4D728EC3F1C Content-Type: message/rfc822; name="[PATCH] MdeModulePkg_PerformanceLib add lock protection.eml" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="[PATCH] MdeModulePkg_PerformanceLib add lock protection.eml" Delivered-To: heyi.guo@linaro.org Received: by 10.103.22.197 with SMTP id 188csp3317717vsw; Sat, 25 Nov 2017 00:48:33 -0800 (PST) X-Google-Smtp-Source: AGs4zMZV2OAMg5UQxdq8QdGIno9UOZJYICKYOxHD5tLTfrwfgxGqjZvzIAU1kpCB0lR34VikUGZF X-Received: by 10.36.20.81 with SMTP id 78mr19164317itg.127.1511599713282; Sat, 25 Nov 2017 00:48:33 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1511599713; cv=none; d=google.com; s=arc-20160816; b=gWPwqeTl17Ypk/Y/8bwQJFSOBCt1CCfAoOirKr1CrPzFoFEI5ob5eoIQEnZs0uIukU C8+CGtJJJBqH+kmiJlrz+ZZLZnOkPh+/aYeJjZd1S84B0pBno4dkCImDAOm4wLFESXtJ uRPlPPmHSHDIcy9mlqN17EewfsXdWCyT378MCp4R4farXPJq+uNgOnBI/atSr45/7Pb/ 1tP0weQ8450Dct03qBtRZZBZkOTgNjiSYlR99fIU1g5tCb/96HUKYJoWt/dfSBL5JsWw jM1hXSmTTYYFbWj0DFUJJ6MAbvzhxv+UvDh3VgpF0MiITQPGv018kteTSn+cLHqiAIVB risQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=mime-version:message-id:date:subject:cc:to:from :arc-authentication-results; bh=973xfojSpjO/94sq/z7ZJ9bkWVx+ZTUykEA75ebhZlg=; b=sqIbNyZi/iBRQujMIKbEsCz/no1PG9VUdSrWWS3c8LxLf+43LTJBBzZ8YYXDvabDPj begZxOapSQ+xC6wcPJKXPPzBbB8x2tr3Mn0oPHAeh6dvxQnlBZCMRiBsj7spm4K6A+Xw XA95EZpzXhXMrFmwj8FnsHGAJ/JlKQTJ8xwkCsCDeRA/rN/2EbOIKfcA6N8H2tOyLY22 +gySt8lx9bjMwfxUknHz7E3zYx+6A5H3DiBknTrXaxDWcn6XNp4FgJqaWMj8jEfgr2gJ V1RV7DEyJs+CDlmBBUWkFQ1tePoqpR4vDTy+5H+nhPkw/SiaNSxFVdrC3Hs6kPo3D4he WJLg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of guoheyi@huawei.com designates 45.249.212.191 as permitted sender) smtp.mailfrom=guoheyi@huawei.com Return-Path: Received: from szxga05-in.huawei.com (szxga05-in.huawei.com. [45.249.212.191]) by mx.google.com with ESMTPS id p10si10558383iti.94.2017.11.25.00.48.28 for (version=TLS1 cipher=AES128-SHA bits=128/128); Sat, 25 Nov 2017 00:48:33 -0800 (PST) Received-SPF: pass (google.com: domain of guoheyi@huawei.com designates 45.249.212.191 as permitted sender) client-ip=45.249.212.191; Authentication-Results: mx.google.com; spf=pass (google.com: domain of guoheyi@huawei.com designates 45.249.212.191 as permitted sender) smtp.mailfrom=guoheyi@huawei.com Received: from 172.30.72.58 (EHLO DGGEMS405-HUB.china.huawei.com) ([172.30.72.58]) by dggrg05-dlp.huawei.com (MOS 4.4.6-GA FastPath queued) with ESMTP id DLO47069; Sat, 25 Nov 2017 16:44:31 +0800 (CST) Received: from HGH1000039998.huawei.com (10.184.68.188) by DGGEMS405-HUB.china.huawei.com (10.3.19.205) with Microsoft SMTP Server id 14.3.361.1; Sat, 25 Nov 2017 16:44:20 +0800 From: Heyi Guo To: CC: , , Subject: [PATCH] MdeModulePkg/PerformanceLib: add lock protection Date: Sat, 25 Nov 2017 16:42:29 +0800 Message-ID: <1511599349-93148-1-git-send-email-guoheyi@huawei.com> X-Mailer: git-send-email 2.8.1 MIME-Version: 1.0 Content-Type: text/plain X-Originating-IP: [10.184.68.188] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090203.5A192D6F.0051,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 48652040663a9cb0a73ce4e283924e94 From: Heyi Guo 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 --- .../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.8.1 --------------30B837547B94A4D728EC3F1C--