Re: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: Move FindVariable after AutoUpdateLangVariable

["Ming Huang" <huangming23@huawei.com>]

在 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 <devel@edk2.groups.io> On Behalf Of Ming
>> Huang via groups.io
>> Sent: Tuesday, June 30, 2020 8:26 PM
>> To: Jiang, Guomin <guomin.jiang@intel.com>; devel@edk2.groups.io; Wang,
>> Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Gao,
>> Liming <liming.gao@intel.com>
>> 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 <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 <jian.j.wang@intel.com>; Wu,
>>>> Hao A <hao.a.wu@intel.com>; Gao, Liming <liming.gao@intel.com>
>>>> 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 <huangming23@huawei.com>
>>>> ---
>>>>  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
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>> 
> 
> 
> .
>