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

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 3/7/2018 9:54 AM, Guo Heyi wrote:
> 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
Done in 72208a9a90b8c6cd5011ddf174ad01e567b67454.

-- 
Thanks,
Ray