From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from huawei.com (huawei.com [45.249.212.32]) by mx.groups.io with SMTP id smtpd.web10.9430.1593780521576037108 for ; Fri, 03 Jul 2020 05:48:42 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.32, mailfrom: huangming23@huawei.com) Received: from DGGEMS409-HUB.china.huawei.com (unknown [172.30.72.58]) by Forcepoint Email with ESMTP id 3A34CA75562C90618B11 for ; Fri, 3 Jul 2020 20:48:38 +0800 (CST) Received: from [127.0.0.1] (10.78.51.60) by DGGEMS409-HUB.china.huawei.com (10.3.19.209) with Microsoft SMTP Server id 14.3.487.0; Fri, 3 Jul 2020 20:48:31 +0800 Subject: Re: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: Move FindVariable after AutoUpdateLangVariable To: "Jiang, Guomin" , "devel@edk2.groups.io" , "Wang, Jian J" , "Wu, Hao A" , "Gao, Liming" CC: "lidongzhan@huawei.com" , "songdongkuang@huawei.com" , "wanghuiqiang@huawei.com" , "qiuliangen@huawei.com" , "shenlimei@huawei.com" , "xiewenyi2@huawei.com" , References: <1593410773-62704-1-git-send-email-huangming23@huawei.com> <1593410773-62704-2-git-send-email-huangming23@huawei.com> From: "Ming Huang" Message-ID: <16e7bca1-f07a-ce3e-7699-97603828d0b5@huawei.com> Date: Fri, 3 Jul 2020 20:48:31 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.78.51.60] X-CFilter-Loop: Reflected Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: 7bit 在 2020/7/1 8:22, Jiang, Guomin 写道: > So I think the key point is why AutoUpdateLangVariable() return success rather than fail, if is it reasonable for this case or we need other error handing? I don't think AutoUpdateLangVariable() should return fail while occur reclaim internal in AutoUpdateLangVariable () function. The problem is the Variable(VARIABLE_POINTER_TRACK) get by FindVariable is invald in this situation and this Variable will be pass to UpdateVariable(). if (mVariableModuleGlobal->VariableGlobal.AuthSupport) { Status = AuthVariableLibProcessVariable (VariableName, VendorGuid, Data, DataSize, Attributes); } else { // This Variable is invald while occur reclaim internal in AutoUpdateLangVariable () Status = UpdateVariable (VariableName, VendorGuid, Data, DataSize, Attributes, 0, 0, &Variable, NULL); } > > I am glad to help you but I can't reproduce it until now, can you provide a step to reproduce it in simulation platform. I am not familiar with simulation platform. We reproduct this issue in our board once. For accelerating reproduction this issue, Add Reclaim() to AutoUpdateLangVariable() for test. Thanks, Ming > > If it is urgent, I suggest that discuss with your internal team first and explain that we need consider the risk check it into edk2. > > Best Regards > Guomin > >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Ming >> Huang via groups.io >> Sent: Tuesday, June 30, 2020 8:26 PM >> To: Jiang, Guomin ; devel@edk2.groups.io; Wang, >> Jian J ; Wu, Hao A ; Gao, >> Liming >> Cc: lidongzhan@huawei.com; songdongkuang@huawei.com; >> wanghuiqiang@huawei.com; qiuliangen@huawei.com; >> shenlimei@huawei.com; xiewenyi2@huawei.com >> Subject: Re: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: >> Move FindVariable after AutoUpdateLangVariable >> >> >> >> 在 2020/6/30 8:58, Jiang, Guomin 写道: >>> Hi Huang, >>> >>> >From issue statement, I guess that >>> 1. AutoUpdateLangVariable() invoked, and it will invoke FindVariable() >>> first, at the same time, reclaim occur and Variable.CurrPtr is invalid, it return >> with success 2. UpdateVariable() is invoked when The old Lang's State is valid >> and the new Lang's State is also valid. >>> 3. In the situation, FindVariable() checked Lang's State and only enable one >> Lang's State. But it didn't in fact. >>> 4. BmForEachVariable() deadloop in the situation. >>> >>> Am I right? >> >> Yes, right. >> >> Thanks, >> Ming >> >>> >>>> -----Original Message----- >>>> From: devel@edk2.groups.io On Behalf Of Ming >>>> Huang via groups.io >>>> Sent: Monday, June 29, 2020 2:06 PM >>>> To: devel@edk2.groups.io; Wang, Jian J ; Wu, >>>> Hao A ; Gao, Liming >>>> Cc: lidongzhan@huawei.com; huangming23@huawei.com; >>>> songdongkuang@huawei.com; wanghuiqiang@huawei.com; >>>> qiuliangen@huawei.com; shenlimei@huawei.com; >> xiewenyi2@huawei.com >>>> Subject: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: >> Move >>>> FindVariable after AutoUpdateLangVariable >>>> >>>> When occur reclaim in AutoUpdateLangVariable(), the CurrPtr of >>>> Variable is invalid. The State will be update with wrong position >>>> after UpdateVariable in this situation and two valid PlatformLang or Lang >> variables will exist. >>>> BmForEachVariable() will enter endless loop while exist two valid >>>> PlatformLang variables. So FindVariable() should be invoked atfer >>>> AutoUpdateLangVariable(). >>>> >>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2667 >>>> >>>> Signed-off-by: Ming Huang >>>> --- >>>> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 26 >>>> ++++++++++---------- >>>> 1 file changed, 13 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c >>>> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c >>>> index 1e71fc6..0cec981 100644 >>>> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c >>>> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c >>>> @@ -2741,6 +2741,19 @@ VariableServiceSetVariable ( >>>> mVariableModuleGlobal->NonVolatileLastVariableOffset = (UINTN) >>>> NextVariable - (UINTN) Point; >>>> } >>>> >>>> + if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) { >>>> + // >>>> + // Hook the operation of setting PlatformLangCodes/PlatformLang >>>> + and >>>> LangCodes/Lang. >>>> + // >>>> + Status = AutoUpdateLangVariable (VariableName, Data, DataSize); >>>> + if (EFI_ERROR (Status)) { >>>> + // >>>> + // The auto update operation failed, directly return to avoid >>>> inconsistency between PlatformLang and Lang. >>>> + // >>>> + goto Done; >>>> + } >>>> + } >>>> + >>>> // >>>> // Check whether the input variable is already existed. >>>> // >>>> @@ -2763,19 +2776,6 @@ VariableServiceSetVariable ( >>>> } >>>> } >>>> >>>> - if (!FeaturePcdGet (PcdUefiVariableDefaultLangDeprecate)) { >>>> - // >>>> - // Hook the operation of setting PlatformLangCodes/PlatformLang and >>>> LangCodes/Lang. >>>> - // >>>> - Status = AutoUpdateLangVariable (VariableName, Data, DataSize); >>>> - if (EFI_ERROR (Status)) { >>>> - // >>>> - // The auto update operation failed, directly return to avoid >> inconsistency >>>> between PlatformLang and Lang. >>>> - // >>>> - goto Done; >>>> - } >>>> - } >>>> - >>>> if (mVariableModuleGlobal->VariableGlobal.AuthSupport) { >>>> Status = AuthVariableLibProcessVariable (VariableName, >>>> VendorGuid, Data, DataSize, Attributes); >>>> } else { >>>> -- >>>> 2.8.1 >>>> >>>> >>>> >>> >>> >>> >> >> >> > > > . >