MdeModulePkg/Variable/RuntimeDxe: Fix return status from Reclaim()

[edk2-devel] [Patch] MdeModulePkg/Variable/RuntimeDxe: Fix return status from Reclaim()

Posted by Michael D Kinney 3 years, 9 months ago

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

Update Reclaim() to return the error status from the reclaim
operation and not the status of SynchronizeRuntimeVariableCache()
that can be EFI_SUCCESS even through the status from reclaim
is an error.  Without this change, the return status from
SetVariable() can be EFI_SUCCESS even though the variable was
not actually set.  This occurs if the variable store is full
and a Reclaim() is invoked to free up space and even after all
possible space is freed, there is still not enough room for
the variable being set.  This condition should return
EFI_OUT_OF_RESOURCES.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 .../Universal/Variable/RuntimeDxe/Variable.c  | 30 +++++++++++--------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 1e71fc642c..41f8ff4ceb 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -531,6 +531,7 @@ Reclaim (
   VOID                  *Point1;
   BOOLEAN               FoundAdded;
   EFI_STATUS            Status;
+  EFI_STATUS            DoneStatus;
   UINTN                 CommonVariableTotalSize;
   UINTN                 CommonUserVariableTotalSize;
   UINTN                 HwErrVariableTotalSize;
@@ -774,25 +775,30 @@ Reclaim (
   }
 
 Done:
+  DoneStatus = EFI_SUCCESS;
   if (IsVolatile || mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
-    Status =  SynchronizeRuntimeVariableCache (
-                &mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache,
-                0,
-                VariableStoreHeader->Size
-                );
-    ASSERT_EFI_ERROR (Status);
+    DoneStatus = SynchronizeRuntimeVariableCache (
+                   &mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache,
+                   0,
+                   VariableStoreHeader->Size
+                   );
+    ASSERT_EFI_ERROR (DoneStatus);
     FreePool (ValidBuffer);
   } else {
     //
     // For NV variable reclaim, we use mNvVariableCache as the buffer, so copy the data back.
     //
     CopyMem (mNvVariableCache, (UINT8 *) (UINTN) VariableBase, VariableStoreHeader->Size);
-    Status =  SynchronizeRuntimeVariableCache (
-                &mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
-                0,
-                VariableStoreHeader->Size
-                );
-    ASSERT_EFI_ERROR (Status);
+    DoneStatus = SynchronizeRuntimeVariableCache (
+                   &mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
+                   0,
+                   VariableStoreHeader->Size
+                   );
+    ASSERT_EFI_ERROR (DoneStatus);
+  }
+
+  if (!EFI_ERROR (Status) && EFI_ERROR (DoneStatus)) {
+    Status = DoneStatus;
   }
 
   return Status;
-- 
2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#62327): https://edk2.groups.io/g/devel/message/62327
Mute This Topic: https://groups.io/mt/75412007/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch] MdeModulePkg/Variable/RuntimeDxe: Fix return status from Reclaim()

Posted by Wu, Hao A 3 years, 9 months ago

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Friday, July 10, 2020 11:05 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [Patch] MdeModulePkg/Variable/RuntimeDxe: Fix return status from
> Reclaim()
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2844
> 
> Update Reclaim() to return the error status from the reclaim operation and
> not the status of SynchronizeRuntimeVariableCache() that can be
> EFI_SUCCESS even through the status from reclaim is an error.  Without this
> change, the return status from
> SetVariable() can be EFI_SUCCESS even though the variable was not actually
> set.  This occurs if the variable store is full and a Reclaim() is invoked to free
> up space and even after all possible space is freed, there is still not enough
> room for the variable being set.  This condition should return
> EFI_OUT_OF_RESOURCES.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  .../Universal/Variable/RuntimeDxe/Variable.c  | 30 +++++++++++--------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 1e71fc642c..41f8ff4ceb 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -531,6 +531,7 @@ Reclaim (
>    VOID                  *Point1;
>    BOOLEAN               FoundAdded;
>    EFI_STATUS            Status;
> +  EFI_STATUS            DoneStatus;
>    UINTN                 CommonVariableTotalSize;
>    UINTN                 CommonUserVariableTotalSize;
>    UINTN                 HwErrVariableTotalSize;
> @@ -774,25 +775,30 @@ Reclaim (
>    }
> 
>  Done:
> +  DoneStatus = EFI_SUCCESS;
>    if (IsVolatile || mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
> -    Status =  SynchronizeRuntimeVariableCache (
> -                &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCach
> e,
> -                0,
> -                VariableStoreHeader->Size
> -                );
> -    ASSERT_EFI_ERROR (Status);
> +    DoneStatus = SynchronizeRuntimeVariableCache (
> +                   &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCach
> e,
> +                   0,
> +                   VariableStoreHeader->Size
> +                   );
> +    ASSERT_EFI_ERROR (DoneStatus);
>      FreePool (ValidBuffer);
>    } else {
>      //
>      // For NV variable reclaim, we use mNvVariableCache as the buffer, so
> copy the data back.
>      //
>      CopyMem (mNvVariableCache, (UINT8 *) (UINTN) VariableBase,
> VariableStoreHeader->Size);
> -    Status =  SynchronizeRuntimeVariableCache (
> -                &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> -                0,
> -                VariableStoreHeader->Size
> -                );
> -    ASSERT_EFI_ERROR (Status);
> +    DoneStatus = SynchronizeRuntimeVariableCache (
> +                   &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> +                   0,
> +                   VariableStoreHeader->Size
> +                   );
> +    ASSERT_EFI_ERROR (DoneStatus);
> +  }
> +
> +  if (!EFI_ERROR (Status) && EFI_ERROR (DoneStatus)) {
> +    Status = DoneStatus;
>    }


Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> 
>    return Status;
> --
> 2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#62470): https://edk2.groups.io/g/devel/message/62470
Mute This Topic: https://groups.io/mt/75412007/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch] MdeModulePkg/Variable/RuntimeDxe: Fix return status from Reclaim()

Posted by Wang, Jian J 3 years, 9 months ago

Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

Regards,
Jian

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D
> Kinney
> Sent: Friday, July 10, 2020 11:05 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2-devel] [Patch] MdeModulePkg/Variable/RuntimeDxe: Fix return
> status from Reclaim()
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2844
> 
> Update Reclaim() to return the error status from the reclaim
> operation and not the status of SynchronizeRuntimeVariableCache()
> that can be EFI_SUCCESS even through the status from reclaim
> is an error.  Without this change, the return status from
> SetVariable() can be EFI_SUCCESS even though the variable was
> not actually set.  This occurs if the variable store is full
> and a Reclaim() is invoked to free up space and even after all
> possible space is freed, there is still not enough room for
> the variable being set.  This condition should return
> EFI_OUT_OF_RESOURCES.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  .../Universal/Variable/RuntimeDxe/Variable.c  | 30 +++++++++++--------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 1e71fc642c..41f8ff4ceb 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -531,6 +531,7 @@ Reclaim (
>    VOID                  *Point1;
>    BOOLEAN               FoundAdded;
>    EFI_STATUS            Status;
> +  EFI_STATUS            DoneStatus;
>    UINTN                 CommonVariableTotalSize;
>    UINTN                 CommonUserVariableTotalSize;
>    UINTN                 HwErrVariableTotalSize;
> @@ -774,25 +775,30 @@ Reclaim (
>    }
> 
>  Done:
> +  DoneStatus = EFI_SUCCESS;
>    if (IsVolatile || mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
> -    Status =  SynchronizeRuntimeVariableCache (
> -                &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache,
> -                0,
> -                VariableStoreHeader->Size
> -                );
> -    ASSERT_EFI_ERROR (Status);
> +    DoneStatus = SynchronizeRuntimeVariableCache (
> +                   &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache,
> +                   0,
> +                   VariableStoreHeader->Size
> +                   );
> +    ASSERT_EFI_ERROR (DoneStatus);
>      FreePool (ValidBuffer);
>    } else {
>      //
>      // For NV variable reclaim, we use mNvVariableCache as the buffer, so copy
> the data back.
>      //
>      CopyMem (mNvVariableCache, (UINT8 *) (UINTN) VariableBase,
> VariableStoreHeader->Size);
> -    Status =  SynchronizeRuntimeVariableCache (
> -                &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> -                0,
> -                VariableStoreHeader->Size
> -                );
> -    ASSERT_EFI_ERROR (Status);
> +    DoneStatus = SynchronizeRuntimeVariableCache (
> +                   &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> +                   0,
> +                   VariableStoreHeader->Size
> +                   );
> +    ASSERT_EFI_ERROR (DoneStatus);
> +  }
> +
> +  if (!EFI_ERROR (Status) && EFI_ERROR (DoneStatus)) {
> +    Status = DoneStatus;
>    }
> 
>    return Status;
> --
> 2.21.0.windows.1
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#62471): https://edk2.groups.io/g/devel/message/62471
Mute This Topic: https://groups.io/mt/75412007/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-