[edk2,1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth

Message ID 1519633779-130687-1-git-send-email-heyi.guo@linaro.org
State New
Headers show
Series
  • [edk2,1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth
Related show

Commit Message

gary guo Feb. 26, 2018, 8:29 a.m.
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(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Wang, Sunny (HPS SW) Feb. 26, 2018, 8:56 a.m. | #1
Hi Heyi, 
Just a suggestion. 
Is it better to use a PCD instead of a define for MAX_RECONNECT_REPAIR? So that we can easily override it in our platform dsc file.

Regards,
Sunny Wang

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Heyi Guo

Sent: Monday, February 26, 2018 4:30 PM
To: edk2-devel@lists.01.org
Cc: Ruiyu Ni <ruiyu.ni@intel.com>; Heyi Guo <heyi.guo@linaro.org>; Eric Dong <eric.dong@intel.com>; Star Zeng <star.zeng@intel.com>
Subject: [edk2] [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo Feb. 26, 2018, 11:34 a.m. | #2
Hi Sunny,

I didn't consider it as a value necessary for platform override, for the retry
count should only have some impact on boot performance and it only happens when
there is something wrong.

May I know what value you will use for your platform and why?

Thanks and regards,

Gary (Heyi Guo)

On Mon, Feb 26, 2018 at 08:56:50AM +0000, Wang, Sunny (HPS SW) wrote:
> Hi Heyi, 

> Just a suggestion. 

> Is it better to use a PCD instead of a define for MAX_RECONNECT_REPAIR? So that we can easily override it in our platform dsc file.

> 

> Regards,

> Sunny Wang

> 

> -----Original Message-----

> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Heyi Guo

> Sent: Monday, February 26, 2018 4:30 PM

> To: edk2-devel@lists.01.org

> Cc: Ruiyu Ni <ruiyu.ni@intel.com>; Heyi Guo <heyi.guo@linaro.org>; Eric Dong <eric.dong@intel.com>; Star Zeng <star.zeng@intel.com>

> Subject: [edk2] [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth

> 

> 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

> 

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Feb. 26, 2018, 4:23 p.m. | #3
On 02/26/18 09:29, 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.


Not really my place to comment on this, but how about removing the
recursion entirely, and turning the logic into a normal (iterative) loop
instead?

(1) Rename the current function to:

STATIC
VOID
BmRepairAllControllersWorker (
  OUT BOOLEAN *ReconnectRequired,
  OUT BOOLEAN *RebootRequired
  );


(2) The worker function should end right before

  if (ReconnectRequired) {
    BmRepairAllControllers ();
  }


(3) The worker function should not contain

  RebootRequired    = FALSE;
  ReconnectRequired = FALSE;

Such initialization should be left to the caller.


(4) The worker function should be called in a loop from a new
BmRepairAllControllers() function, like this:

  Reconnect = 0;
  RebootRequired = FALSE;
  do {
    ReconnectRequired = FALSE;
    BmRepairAllControllersWorker (&ReconnectRequired, &RebootRequired);
    ++Reconnect;
  } while (ReconnectRequired && Reconnect < MAX_RECONNECT_REPAIR);

  DEBUG_CODE (...);

  if (RebootRequired) {
    ...
  }


In addition to eliminating the shoddy recursive call (and the shoddier
global counter, ewww :) ), this would fix the following other warts with
the code:

- When a nested call chain is unwound, we currently dump a series of
"driver health info" lists (assuming no reboot is required), in the
DEBUG_CODE section. I would argue that we should do that only once, at
the end. (Even if we have to do it multiple times, it can be moved into
the worker function, to the end.)

- It seems to be sufficient to accumulate RebootRequired into one
variable (i.e. not multiple instances of the same local variable on the
stack) and to act upon it at the very end.


Feel free to ignore my comments -- I just think we should be moving in
the opposite direction; that is, away from recursion (especially from
recursion combined with global variables -- that's one difficult pattern
to reason about).

Thanks
Laszlo

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

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo Feb. 27, 2018, 12:48 a.m. | #4
Hi Laszlo,

I agree the current patch makes the code ugly, and turning the logic into a
normal loop should be the perfect solution. If Ray also agrees on it, I can try
to do that.

Thanks and regards,

Heyi

On Mon, Feb 26, 2018 at 05:23:29PM +0100, Laszlo Ersek wrote:
> On 02/26/18 09:29, 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.

> 

> Not really my place to comment on this, but how about removing the

> recursion entirely, and turning the logic into a normal (iterative) loop

> instead?

> 

> (1) Rename the current function to:

> 

> STATIC

> VOID

> BmRepairAllControllersWorker (

>   OUT BOOLEAN *ReconnectRequired,

>   OUT BOOLEAN *RebootRequired

>   );

> 

> 

> (2) The worker function should end right before

> 

>   if (ReconnectRequired) {

>     BmRepairAllControllers ();

>   }

> 

> 

> (3) The worker function should not contain

> 

>   RebootRequired    = FALSE;

>   ReconnectRequired = FALSE;

> 

> Such initialization should be left to the caller.

> 

> 

> (4) The worker function should be called in a loop from a new

> BmRepairAllControllers() function, like this:

> 

>   Reconnect = 0;

>   RebootRequired = FALSE;

>   do {

>     ReconnectRequired = FALSE;

>     BmRepairAllControllersWorker (&ReconnectRequired, &RebootRequired);

>     ++Reconnect;

>   } while (ReconnectRequired && Reconnect < MAX_RECONNECT_REPAIR);

> 

>   DEBUG_CODE (...);

> 

>   if (RebootRequired) {

>     ...

>   }

> 

> 

> In addition to eliminating the shoddy recursive call (and the shoddier

> global counter, ewww :) ), this would fix the following other warts with

> the code:

> 

> - When a nested call chain is unwound, we currently dump a series of

> "driver health info" lists (assuming no reboot is required), in the

> DEBUG_CODE section. I would argue that we should do that only once, at

> the end. (Even if we have to do it multiple times, it can be moved into

> the worker function, to the end.)

> 

> - It seems to be sufficient to accumulate RebootRequired into one

> variable (i.e. not multiple instances of the same local variable on the

> stack) and to act upon it at the very end.

> 

> 

> Feel free to ignore my comments -- I just think we should be moving in

> the opposite direction; that is, away from recursion (especially from

> recursion combined with global variables -- that's one difficult pattern

> to reason about).

> 

> Thanks

> Laszlo

> 

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

> > 

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Wang, Sunny (HPS SW) Feb. 27, 2018, 2:47 a.m. | #5
Hi Heyi, 
Thanks for looking into my suggestion. You already mentioned the value I thought. :) The value is to get better boot performance by setting the MAX_RECONNECT_REPAIR to a smaller number. 10 times reconnect may be suitable for consumer products like laptop. However, it may be not suitable for server. For example, user installs a problematic NIC 4-port card and its OPROM produced 4 driver handles to manage 4 ports. For this case, 10 times reconnect may take much longer time.  
If you think it's still not worth adding a PCD for this or have other concern about adding a PCD, I'm fine with dropping this suggestion.   

Regards,
Sunny Wang

-----Original Message-----
From: Guo Heyi [mailto:heyi.guo@linaro.org] 

Sent: Monday, February 26, 2018 7:34 PM
To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>
Cc: Heyi Guo <heyi.guo@linaro.org>; edk2-devel@lists.01.org; Ruiyu Ni <ruiyu.ni@intel.com>; Eric Dong <eric.dong@intel.com>; Star Zeng <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth
Importance: High

Hi Sunny,

I didn't consider it as a value necessary for platform override, for the retry count should only have some impact on boot performance and it only happens when there is something wrong.

May I know what value you will use for your platform and why?

Thanks and regards,

Gary (Heyi Guo)

On Mon, Feb 26, 2018 at 08:56:50AM +0000, Wang, Sunny (HPS SW) wrote:
> Hi Heyi,

> Just a suggestion. 

> Is it better to use a PCD instead of a define for MAX_RECONNECT_REPAIR? So that we can easily override it in our platform dsc file.

> 

> Regards,

> Sunny Wang

> 

> -----Original Message-----

> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 

> Heyi Guo

> Sent: Monday, February 26, 2018 4:30 PM

> To: edk2-devel@lists.01.org

> Cc: Ruiyu Ni <ruiyu.ni@intel.com>; Heyi Guo <heyi.guo@linaro.org>; 

> Eric Dong <eric.dong@intel.com>; Star Zeng <star.zeng@intel.com>

> Subject: [edk2] [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit 

> recursive call depth

> 

> 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

> 

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ni, Ruiyu Feb. 27, 2018, 5:48 a.m. | #6
On 2/27/2018 8:48 AM, Guo Heyi wrote:
> Hi Laszlo,

> 

> I agree the current patch makes the code ugly, and turning the logic into a

> normal loop should be the perfect solution. If Ray also agrees on it, I can try

> to do that.

> 

> Thanks and regards,

> 

> Heyi

> 

> On Mon, Feb 26, 2018 at 05:23:29PM +0100, Laszlo Ersek wrote:

>> On 02/26/18 09:29, 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.

>>

>> Not really my place to comment on this, but how about removing the

>> recursion entirely, and turning the logic into a normal (iterative) loop

>> instead?

>>

>> (1) Rename the current function to:

>>

>> STATIC

>> VOID

>> BmRepairAllControllersWorker (

>>    OUT BOOLEAN *ReconnectRequired,

>>    OUT BOOLEAN *RebootRequired

>>    );

>>

>>

>> (2) The worker function should end right before

>>

>>    if (ReconnectRequired) {

>>      BmRepairAllControllers ();

>>    }

>>

>>

>> (3) The worker function should not contain

>>

>>    RebootRequired    = FALSE;

>>    ReconnectRequired = FALSE;

>>

>> Such initialization should be left to the caller.

>>

>>

>> (4) The worker function should be called in a loop from a new

>> BmRepairAllControllers() function, like this:

>>

>>    Reconnect = 0;

>>    RebootRequired = FALSE;

>>    do {

>>      ReconnectRequired = FALSE;

>>      BmRepairAllControllersWorker (&ReconnectRequired, &RebootRequired);

>>      ++Reconnect;

>>    } while (ReconnectRequired && Reconnect < MAX_RECONNECT_REPAIR);

>>

>>    DEBUG_CODE (...);

>>

>>    if (RebootRequired) {

>>      ...

>>    }

>>

>>

>> In addition to eliminating the shoddy recursive call (and the shoddier

>> global counter, ewww :) ), this would fix the following other warts with

>> the code:

>>

>> - When a nested call chain is unwound, we currently dump a series of

>> "driver health info" lists (assuming no reboot is required), in the

>> DEBUG_CODE section. I would argue that we should do that only once, at

>> the end. (Even if we have to do it multiple times, it can be moved into

>> the worker function, to the end.)

>>

>> - It seems to be sufficient to accumulate RebootRequired into one

>> variable (i.e. not multiple instances of the same local variable on the

>> stack) and to act upon it at the very end.

>>

>>

>> Feel free to ignore my comments -- I just think we should be moving in

>> the opposite direction; that is, away from recursion (especially from

>> recursion combined with global variables -- that's one difficult pattern

>> to reason about).


How about to just remove the global variable?
I prefer to change BmRepairAllControllers in the following prototype:
VOID
BmRepairAllControllers (
   UINTN  ReconnectRepairCount
   );
And start to call this like BmRepairAllControllers (0).

I am neutral between recursive call and while loop.
But I am afraid such a big change may introduce some bugs.
And I also like to move the DEBUG_CODE to before:
if (ReconnectRequired) {
   BmRepairAllControllers (ReconnectRepairCount + 1);
}
So that we can dump the health info for every reconnect repair.



>>

>> Thanks

>> Laszlo

>>

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

>>>

>>

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

> 



-- 
Thanks,
Ray
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Feb. 27, 2018, 10:29 a.m. | #7
On 02/27/18 06:48, Ni, Ruiyu wrote:
> On 2/27/2018 8:48 AM, Guo Heyi wrote:
>> Hi Laszlo,
>>
>> I agree the current patch makes the code ugly, and turning the logic
>> into a
>> normal loop should be the perfect solution. If Ray also agrees on it,
>> I can try
>> to do that.
>>
>> Thanks and regards,
>>
>> Heyi
>>
>> On Mon, Feb 26, 2018 at 05:23:29PM +0100, Laszlo Ersek wrote:
>>> On 02/26/18 09:29, 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.
>>>
>>> Not really my place to comment on this, but how about removing the
>>> recursion entirely, and turning the logic into a normal (iterative) loop
>>> instead?
>>>
>>> (1) Rename the current function to:
>>>
>>> STATIC
>>> VOID
>>> BmRepairAllControllersWorker (
>>>    OUT BOOLEAN *ReconnectRequired,
>>>    OUT BOOLEAN *RebootRequired
>>>    );
>>>
>>>
>>> (2) The worker function should end right before
>>>
>>>    if (ReconnectRequired) {
>>>      BmRepairAllControllers ();
>>>    }
>>>
>>>
>>> (3) The worker function should not contain
>>>
>>>    RebootRequired    = FALSE;
>>>    ReconnectRequired = FALSE;
>>>
>>> Such initialization should be left to the caller.
>>>
>>>
>>> (4) The worker function should be called in a loop from a new
>>> BmRepairAllControllers() function, like this:
>>>
>>>    Reconnect = 0;
>>>    RebootRequired = FALSE;
>>>    do {
>>>      ReconnectRequired = FALSE;
>>>      BmRepairAllControllersWorker (&ReconnectRequired, &RebootRequired);
>>>      ++Reconnect;
>>>    } while (ReconnectRequired && Reconnect < MAX_RECONNECT_REPAIR);
>>>
>>>    DEBUG_CODE (...);
>>>
>>>    if (RebootRequired) {
>>>      ...
>>>    }
>>>
>>>
>>> In addition to eliminating the shoddy recursive call (and the shoddier
>>> global counter, ewww :) ), this would fix the following other warts with
>>> the code:
>>>
>>> - When a nested call chain is unwound, we currently dump a series of
>>> "driver health info" lists (assuming no reboot is required), in the
>>> DEBUG_CODE section. I would argue that we should do that only once, at
>>> the end. (Even if we have to do it multiple times, it can be moved into
>>> the worker function, to the end.)
>>>
>>> - It seems to be sufficient to accumulate RebootRequired into one
>>> variable (i.e. not multiple instances of the same local variable on the
>>> stack) and to act upon it at the very end.
>>>
>>>
>>> Feel free to ignore my comments -- I just think we should be moving in
>>> the opposite direction; that is, away from recursion (especially from
>>> recursion combined with global variables -- that's one difficult pattern
>>> to reason about).
> 
> How about to just remove the global variable?
> I prefer to change BmRepairAllControllers in the following prototype:
> VOID
> BmRepairAllControllers (
>   UINTN  ReconnectRepairCount
>   );
> And start to call this like BmRepairAllControllers (0).
> 
> I am neutral between recursive call and while loop.
> But I am afraid such a big change may introduce some bugs.
> And I also like to move the DEBUG_CODE to before:
> if (ReconnectRequired) {
>   BmRepairAllControllers (ReconnectRepairCount + 1);
> }
> So that we can dump the health info for every reconnect repair.

Sure, that too works for me.

Thanks!
Laszlo
gary guo Feb. 27, 2018, 10:39 a.m. | #8
Thanks Ray and Laszlo, I will create v2 according to your comments.

Regards,

Heyi

On Tue, Feb 27, 2018 at 11:29:18AM +0100, Laszlo Ersek wrote:
> On 02/27/18 06:48, Ni, Ruiyu wrote:

> > On 2/27/2018 8:48 AM, Guo Heyi wrote:

> >> Hi Laszlo,

> >>

> >> I agree the current patch makes the code ugly, and turning the logic

> >> into a

> >> normal loop should be the perfect solution. If Ray also agrees on it,

> >> I can try

> >> to do that.

> >>

> >> Thanks and regards,

> >>

> >> Heyi

> >>

> >> On Mon, Feb 26, 2018 at 05:23:29PM +0100, Laszlo Ersek wrote:

> >>> On 02/26/18 09:29, 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.

> >>>

> >>> Not really my place to comment on this, but how about removing the

> >>> recursion entirely, and turning the logic into a normal (iterative) loop

> >>> instead?

> >>>

> >>> (1) Rename the current function to:

> >>>

> >>> STATIC

> >>> VOID

> >>> BmRepairAllControllersWorker (

> >>>    OUT BOOLEAN *ReconnectRequired,

> >>>    OUT BOOLEAN *RebootRequired

> >>>    );

> >>>

> >>>

> >>> (2) The worker function should end right before

> >>>

> >>>    if (ReconnectRequired) {

> >>>      BmRepairAllControllers ();

> >>>    }

> >>>

> >>>

> >>> (3) The worker function should not contain

> >>>

> >>>    RebootRequired    = FALSE;

> >>>    ReconnectRequired = FALSE;

> >>>

> >>> Such initialization should be left to the caller.

> >>>

> >>>

> >>> (4) The worker function should be called in a loop from a new

> >>> BmRepairAllControllers() function, like this:

> >>>

> >>>    Reconnect = 0;

> >>>    RebootRequired = FALSE;

> >>>    do {

> >>>      ReconnectRequired = FALSE;

> >>>      BmRepairAllControllersWorker (&ReconnectRequired, &RebootRequired);

> >>>      ++Reconnect;

> >>>    } while (ReconnectRequired && Reconnect < MAX_RECONNECT_REPAIR);

> >>>

> >>>    DEBUG_CODE (...);

> >>>

> >>>    if (RebootRequired) {

> >>>      ...

> >>>    }

> >>>

> >>>

> >>> In addition to eliminating the shoddy recursive call (and the shoddier

> >>> global counter, ewww :) ), this would fix the following other warts with

> >>> the code:

> >>>

> >>> - When a nested call chain is unwound, we currently dump a series of

> >>> "driver health info" lists (assuming no reboot is required), in the

> >>> DEBUG_CODE section. I would argue that we should do that only once, at

> >>> the end. (Even if we have to do it multiple times, it can be moved into

> >>> the worker function, to the end.)

> >>>

> >>> - It seems to be sufficient to accumulate RebootRequired into one

> >>> variable (i.e. not multiple instances of the same local variable on the

> >>> stack) and to act upon it at the very end.

> >>>

> >>>

> >>> Feel free to ignore my comments -- I just think we should be moving in

> >>> the opposite direction; that is, away from recursion (especially from

> >>> recursion combined with global variables -- that's one difficult pattern

> >>> to reason about).

> > 

> > How about to just remove the global variable?

> > I prefer to change BmRepairAllControllers in the following prototype:

> > VOID

> > BmRepairAllControllers (

> >   UINTN  ReconnectRepairCount

> >   );

> > And start to call this like BmRepairAllControllers (0).

> > 

> > I am neutral between recursive call and while loop.

> > But I am afraid such a big change may introduce some bugs.

> > And I also like to move the DEBUG_CODE to before:

> > if (ReconnectRequired) {

> >   BmRepairAllControllers (ReconnectRepairCount + 1);

> > }

> > So that we can dump the health info for every reconnect repair.

> 

> Sure, that too works for me.

> 

> Thanks!

> Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

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 (