Re: [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth

[Guo Heyi <heyi.guo@linaro.org>]

Hi Ray,

Sorry to disturb, but I didn't find the patch committed. Could you help to do
that?

Thanks,
Heyi


On Thu, Mar 01, 2018 at 12:46:32PM +0800, Ni, Ruiyu wrote:
> On 3/1/2018 10:39 AM, Heyi Guo wrote:
> >Function BmRepairAllControllers may recursively call itself if some
> >driver health protocol returns EfiDriverHealthStatusReconnectRequired.
> >However, driver health protocol of some buggy third party driver may
> >always return such status even after one and another reconnect. The
> >endless iteration will cause stack overflow and then system exception,
> >and it may be not easy to find that the exception is actually caused
> >by stack overflow.
> >
> >So we limit the number of reconnect retry to 10 to improve code
> >robustness, and DEBUG_CODE is moved ahead before recursive repair to
> >track the repair result.
> >
> >We also remove a duplicated declaration of BmRepairAllControllers() in
> >InternalBm.h in this patch, for it is only a trivial change.
> >
> >Contributed-under: TianoCore Contribution Agreement 1.1
> >Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> >Cc: Star Zeng <star.zeng@intel.com>
> >Cc: Eric Dong <eric.dong@intel.com>
> >Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >Cc: Laszlo Ersek <lersek@redhat.com>
> >---
> >
> >Notes:
> >     v2
> >     - Use argument instead of global variable to record the recursive
> >       count [Ray]
> >     - Move DEBUG_CODE before recursively calling BmRepairAllControllers()
> >       to track the change of each reconnect [Ray]
> >     - Remove a duplicated declaration of BmRepairAllControllers() in
> >       InternalBm.h.
> >
> >  MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h     | 19 ++++++++++---------
> >  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c         |  2 +-
> >  MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c | 18 +++++++++++++-----
> >  3 files changed, 24 insertions(+), 15 deletions(-)
> >
> >diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> >index 25a1d522fe84..21ecd8584d24 100644
> >--- a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> >+++ b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
> >@@ -108,6 +108,12 @@ CHAR16 *
> >  #define BM_OPTION_NAME_LEN                          sizeof ("PlatformRecovery####")
> >  extern CHAR16  *mBmLoadOptionName[];
> >+//
> >+// Maximum number of reconnect retry to repair controller; it is to limit the
> >+// number of recursive call of BmRepairAllControllers.
> >+//
> >+#define MAX_RECONNECT_REPAIR                        10
> >+
> >  /**
> >    Visitor function to be called by BmForEachVariable for each variable
> >    in variable storage.
> >@@ -145,10 +151,13 @@ typedef struct {
> >  /**
> >    Repair all the controllers according to the Driver Health status queried.
> >+
> >+  @param ReconnectRepairCount     To record the number of recursive call of
> >+                                  this function itself.
> >  **/
> >  VOID
> >  BmRepairAllControllers (
> >-  VOID
> >+  UINTN       ReconnectRepairCount
> >    );
> >  #define BM_HOTKEY_SIGNATURE SIGNATURE_32 ('b', 'm', 'h', 'k')
> >@@ -328,14 +337,6 @@ BmDelPartMatchInstance (
> >    );
> >  /**
> >-  Repair all the controllers according to the Driver Health status queried.
> >-**/
> >-VOID
> >-BmRepairAllControllers (
> >-  VOID
> >-  );
> >-
> >-/**
> >    Print the device path info.
> >    @param DevicePath           The device path need to print.
> >diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> >index ce19ae400660..b842d5824aed 100644
> >--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> >+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> >@@ -1767,7 +1767,7 @@ EfiBootManagerBoot (
> >      //
> >      // 4. Repair system through DriverHealth protocol
> >      //
> >-    BmRepairAllControllers ();
> >+    BmRepairAllControllers (0);
> >    }
> >    PERF_START_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber);
> >diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c
> >index ddcee8b0676f..db2f859ae73d 100644
> >--- a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c
> >+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c
> >@@ -423,10 +423,13 @@ EfiBootManagerFreeDriverHealthInfo (
> >  /**
> >    Repair all the controllers according to the Driver Health status queried.
> >+
> >+  @param ReconnectRepairCount     To record the number of recursive call of
> >+                                  this function itself.
> >  **/
> >  VOID
> >  BmRepairAllControllers (
> >-  VOID
> >+  UINTN       ReconnectRepairCount
> >    )
> >  {
> >    EFI_STATUS                          Status;
> >@@ -548,10 +551,6 @@ BmRepairAllControllers (
> >    EfiBootManagerFreeDriverHealthInfo (DriverHealthInfo, Count);
> >-  if (ReconnectRequired) {
> >-    BmRepairAllControllers ();
> >-  }
> >-
> >    DEBUG_CODE (
> >      CHAR16 *ControllerName;
> >@@ -576,6 +575,15 @@ BmRepairAllControllers (
> >      EfiBootManagerFreeDriverHealthInfo (DriverHealthInfo, Count);
> >      );
> >+  if (ReconnectRequired) {
> >+    if (ReconnectRepairCount < MAX_RECONNECT_REPAIR) {
> >+      BmRepairAllControllers (ReconnectRepairCount + 1);
> >+    } else {
> >+      DEBUG ((DEBUG_ERROR, "[%a:%d] Repair failed after %d retries.\n",
> >+        __FUNCTION__, __LINE__, ReconnectRepairCount));
> >+    }
> >+  }
> >+
> >    if (RebootRequired) {
> >      DEBUG ((EFI_D_INFO, "[BDS] One of the Driver Health instances requires rebooting.\n"));
> >      gRT->ResetSystem (EfiResetWarm, EFI_SUCCESS, 0, NULL);
> >
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> 
> -- 
> Thanks,
> Ray