[PATCH edk2 v1 0/1] Fix a infrequent issue in variable

[PATCH edk2 v1 0/1] Fix a infrequent issue in variable

[Ming Huang]

https://bugzilla.tianocore.org/show_bug.cgi?id=2667


Ming Huang (1):
  MdeModulePkg/Variable: Move FindVariable after AutoUpdateLangVariable

 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 26 ++++++++++----------
 1 file changed, 13 insertions(+), 13 deletions(-)

-- 
2.8.1

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

[Ming Huang]

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

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

[Guomin Jiang]

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?

> -----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
> 
> 
> 

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

[Ming Huang]

在 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
>>
>>
>> 
> 
> 
> 

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

[Guomin Jiang]

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 am glad to help you but I can't reproduce it until now, can you provide a step to reproduce it in simulation platform.

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
> >>
> >>
> >>
> >
> >
> >
> 
> 
> 

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

[Ming Huang]

在 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
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>> 
> 
> 
> .
> 

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

[Guomin Jiang]

Hi Ming,

The new posted change https://edk2.groups.io/g/devel/topic/75412007#62327 may be helpful for this issue.

Can you add the change in your code and verify it?

Thanks
Guomin
> -----Original Message-----
> From: Ming Huang <huangming23@huawei.com>
> Sent: Friday, July 3, 2020 8:49 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; john.garry@huawei.com
> Subject: Re: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable:
> Move FindVariable after AutoUpdateLangVariable
> 
> 
> 
> 在 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
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>>
> >>
> >>
> >> 
> >
> >
> > .
> >

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

[Ming Huang]

Hi GuoMin,

I don't think the new change can fix this issue(id=2667). We use older edk2 version which don't have 
SynchronizeRuntimeVariableCache().

Thanks
Ming

-----邮件原件-----
发件人: Jiang, Guomin [mailto:guomin.jiang@intel.com] 
发送时间: 2020年7月22日 8:47
收件人: Huangming (Mark) <huangming23@huawei.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>
抄送: Lidongzhan <lidongzhan@huawei.com>; Songdongkuang <songdongkuang@huawei.com>; wanghuiqiang <wanghuiqiang@huawei.com>; qiuliangen <qiuliangen@huawei.com>; Shenlimei <shenlimei@huawei.com>; xiewenyi (A) <xiewenyi2@huawei.com>; John Garry <john.garry@huawei.com>
主题: RE: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: Move FindVariable after AutoUpdateLangVariable

Hi Ming,

The new posted change https://edk2.groups.io/g/devel/topic/75412007#62327 may be helpful for this issue.

Can you add the change in your code and verify it?

Thanks
Guomin
> -----Original Message-----
> From: Ming Huang <huangming23@huawei.com>
> Sent: Friday, July 3, 2020 8:49 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; john.garry@huawei.com
> Subject: Re: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable:
> Move FindVariable after AutoUpdateLangVariable
> 
> 
> 
> 在 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
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>>
> >>
> >>
> >> 
> >
> >
> > .
> >

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

[Guomin Jiang]

Hi Ming,

Can you try the newest edk2?

Thanks
Guomin
> -----Original Message-----
> From: Huangming (Mark) <huangming23@huawei.com>
> Sent: Wednesday, July 22, 2020 2:44 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 <lidongzhan@huawei.com>; Songdongkuang
> <songdongkuang@huawei.com>; wanghuiqiang
> <wanghuiqiang@huawei.com>; qiuliangen <qiuliangen@huawei.com>;
> Shenlimei <shenlimei@huawei.com>; xiewenyi (A)
> <xiewenyi2@huawei.com>; John Garry <john.garry@huawei.com>
> Subject: 答复: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable:
> Move FindVariable after AutoUpdateLangVariable
> 
> Hi GuoMin,
> 
> I don't think the new change can fix this issue(id=2667). We use older edk2
> version which don't have SynchronizeRuntimeVariableCache().
> 
> Thanks
> Ming
> 
> -----邮件原件-----
> 发件人: Jiang, Guomin [mailto:guomin.jiang@intel.com]
> 发送时间: 2020年7月22日 8:47
> 收件人: Huangming (Mark) <huangming23@huawei.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>
> 抄送: Lidongzhan <lidongzhan@huawei.com>; Songdongkuang
> <songdongkuang@huawei.com>; wanghuiqiang
> <wanghuiqiang@huawei.com>; qiuliangen <qiuliangen@huawei.com>;
> Shenlimei <shenlimei@huawei.com>; xiewenyi (A)
> <xiewenyi2@huawei.com>; John Garry <john.garry@huawei.com>
> 主题: RE: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: Move
> FindVariable after AutoUpdateLangVariable
> 
> Hi Ming,
> 
> The new posted change
> https://edk2.groups.io/g/devel/topic/75412007#62327 may be helpful for
> this issue.
> 
> Can you add the change in your code and verify it?
> 
> Thanks
> Guomin
> > -----Original Message-----
> > From: Ming Huang <huangming23@huawei.com>
> > Sent: Friday, July 3, 2020 8:49 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; john.garry@huawei.com
> > Subject: Re: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable:
> > Move FindVariable after AutoUpdateLangVariable
> >
> >
> >
> > 在 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
> > >>>>
> > >>>>
> > >>>>
> > >>>
> > >>>
> > >>>
> > >>
> > >>
> > >> 
> > >
> > >
> > > .
> > >

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

[Ming Huang]

Hi Guomin,

Ok, it will take some time to do this.

Thanks
Ming

-----邮件原件-----
发件人: Jiang, Guomin [mailto:guomin.jiang@intel.com] 
发送时间: 2020年7月22日 14:49
收件人: Huangming (Mark) <huangming23@huawei.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>
抄送: Lidongzhan <lidongzhan@huawei.com>; Songdongkuang <songdongkuang@huawei.com>; wanghuiqiang <wanghuiqiang@huawei.com>; qiuliangen <qiuliangen@huawei.com>; Shenlimei <shenlimei@huawei.com>; xiewenyi (A) <xiewenyi2@huawei.com>; John Garry <john.garry@huawei.com>
主题: RE: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: Move FindVariable after AutoUpdateLangVariable

Hi Ming,

Can you try the newest edk2?

Thanks
Guomin
> -----Original Message-----
> From: Huangming (Mark) <huangming23@huawei.com>
> Sent: Wednesday, July 22, 2020 2:44 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 <lidongzhan@huawei.com>; Songdongkuang 
> <songdongkuang@huawei.com>; wanghuiqiang <wanghuiqiang@huawei.com>; 
> qiuliangen <qiuliangen@huawei.com>; Shenlimei <shenlimei@huawei.com>; 
> xiewenyi (A) <xiewenyi2@huawei.com>; John Garry 
> <john.garry@huawei.com>
> Subject: 答复: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable:
> Move FindVariable after AutoUpdateLangVariable
> 
> Hi GuoMin,
> 
> I don't think the new change can fix this issue(id=2667). We use older 
> edk2 version which don't have SynchronizeRuntimeVariableCache().
> 
> Thanks
> Ming
> 
> -----邮件原件-----
> 发件人: Jiang, Guomin [mailto:guomin.jiang@intel.com]
> 发送时间: 2020年7月22日 8:47
> 收件人: Huangming (Mark) <huangming23@huawei.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>
> 抄送: Lidongzhan <lidongzhan@huawei.com>; Songdongkuang 
> <songdongkuang@huawei.com>; wanghuiqiang <wanghuiqiang@huawei.com>; 
> qiuliangen <qiuliangen@huawei.com>; Shenlimei <shenlimei@huawei.com>; 
> xiewenyi (A) <xiewenyi2@huawei.com>; John Garry 
> <john.garry@huawei.com>
> 主题: RE: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable: Move 
> FindVariable after AutoUpdateLangVariable
> 
> Hi Ming,
> 
> The new posted change
> https://edk2.groups.io/g/devel/topic/75412007#62327 may be helpful for 
> this issue.
> 
> Can you add the change in your code and verify it?
> 
> Thanks
> Guomin
> > -----Original Message-----
> > From: Ming Huang <huangming23@huawei.com>
> > Sent: Friday, July 3, 2020 8:49 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; john.garry@huawei.com
> > Subject: Re: [edk2-devel] [PATCH edk2 v1 1/1] MdeModulePkg/Variable:
> > Move FindVariable after AutoUpdateLangVariable
> >
> >
> >
> > 在 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
> > >>>>
> > >>>>
> > >>>>
> > >>>
> > >>>
> > >>>
> > >>
> > >>
> > >> 
> > >
> > >
> > > .
> > >