public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth
@ 2018-03-01  2:39 Heyi Guo
  2018-03-01  2:43 ` Guo Heyi
  2018-03-01  4:46 ` Ni, Ruiyu
  0 siblings, 2 replies; 14+ messages in thread
From: Heyi Guo @ 2018-03-01  2:39 UTC (permalink / raw)
  To: edk2-devel; +Cc: Heyi Guo, Star Zeng, Eric Dong, Ruiyu Ni, Laszlo Ersek

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);
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth
@ 2018-02-26  8:29 Heyi Guo
  2018-02-26  8:56 ` Wang, Sunny (HPS SW)
  2018-02-26 16:23 ` Laszlo Ersek
  0 siblings, 2 replies; 14+ messages in thread
From: Heyi Guo @ 2018-02-26  8:29 UTC (permalink / raw)
  To: edk2-devel; +Cc: Heyi Guo, Star Zeng, Eric Dong, Ruiyu Ni

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.

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>
---
 MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h     |  6 ++++++
 MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c | 17 ++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
index 25a1d522fe84..9aa86b096525 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.
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c
index ddcee8b0676f..30d70f32af84 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmDriverHealth.c
@@ -26,6 +26,12 @@ GLOBAL_REMOVE_IF_UNREFERENCED
     L"Reboot Required"
     };
 
+//
+// Counter of reconnect retry to repair controller; it is to limit the
+// number of recursive call of BmRepairAllControllers.
+//
+STATIC UINTN mReconnectRepairCount;
+
 /**
   Return the controller name.
 
@@ -549,7 +555,16 @@ BmRepairAllControllers (
 
 
   if (ReconnectRequired) {
-    BmRepairAllControllers ();
+    if (mReconnectRepairCount < MAX_RECONNECT_REPAIR) {
+      mReconnectRepairCount++;
+      BmRepairAllControllers ();
+    } else {
+      DEBUG ((DEBUG_ERROR, "[%a:%d] Repair failed after %d retries.\n",
+        __FUNCTION__, __LINE__, mReconnectRepairCount));
+      // Reset counter so that it will not affect calling
+      // BmRepairAllControllers() somewhere else
+      mReconnectRepairCount = 0;
+    }
   }
 
   DEBUG_CODE (
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-03-08  2:47 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-01  2:39 [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth Heyi Guo
2018-03-01  2:43 ` Guo Heyi
2018-03-01  4:46 ` Ni, Ruiyu
2018-03-07  1:54   ` Guo Heyi
2018-03-08  2:53     ` Ni, Ruiyu
  -- strict thread matches above, loose matches on Subject: below --
2018-02-26  8:29 Heyi Guo
2018-02-26  8:56 ` Wang, Sunny (HPS SW)
2018-02-26 11:34   ` Guo Heyi
2018-02-27  2:47     ` Wang, Sunny (HPS SW)
2018-02-26 16:23 ` Laszlo Ersek
2018-02-27  0:48   ` Guo Heyi
2018-02-27  5:48     ` Ni, Ruiyu
2018-02-27 10:29       ` Laszlo Ersek
2018-02-27 10:39         ` Guo Heyi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox