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

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

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