[U-Boot] [PATCH 2/3] efi_loader: add checking for incorrect use of EFI_ENTRY/EXIT

[Rob Clark]

On Wed, Jul 26, 2017 at 11:25 AM, Alexander Graf <agraf at suse.de> wrote:
>
>
> On 26.07.17 15:55, Rob Clark wrote:
>>
>> Missing an EFI_ENTRY() or doubling up EFI_EXIT() leads to non-obvious
>> crashes.  Let's add some error checking.
>>
>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>> ---
>>   include/efi_loader.h          |  5 +++++
>>   lib/efi_loader/efi_boottime.c | 13 +++++++++++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 09bab7dbc6..4b49fac84b 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -15,11 +15,13 @@
>>     #include <linux/list.h>
>>   +int __efi_check_nesting(int delta);
>>   /*
>>    * Enter the u-boot world from UEFI:
>>    */
>>   #define EFI_ENTRY(format, ...) do { \
>>         efi_restore_gd(); \
>> +       assert(__efi_check_nesting(+1)); \
>>         debug("EFI: Entry %s(" format ")\n", __func__, ##__VA_ARGS__); \
>>         } while(0)
>>   @@ -28,6 +30,7 @@
>>    */
>>   #define EFI_EXIT(ret) ({ \
>>         debug("EFI: Exit: %s: %u\n", __func__, (u32)((ret) &
>> ~EFI_ERROR_MASK)); \
>> +       assert(__efi_check_nesting(-1)); \
>>         efi_exit_func(ret); \
>>         })
>>   @@ -36,10 +39,12 @@
>>    */
>>   #define EFI_CALL(exp) do { \
>>         debug("EFI: Call: %s\n", #exp); \
>> +       assert(__efi_check_nesting(-1)); \
>>         efi_exit_func(EFI_SUCCESS); \
>>         exp; \
>>         efi_restore_gd(); \
>>         debug("EFI: Return From: %s\n", #exp); \
>> +       assert(__efi_check_nesting(+1)); \
>>         } while(0)
>>     extern struct efi_runtime_services efi_runtime_services;
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 76cafffc1d..b21df7bd5d 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -57,6 +57,17 @@ void efi_save_gd(void)
>>   #endif
>>   }
>>   +/* Check for incorrect use of EFI_ENTRY() / EFI_EXIT() */
>> +int __efi_check_nesting(int delta)
>> +{
>> +       static int entry_count;
>
>
> I'd prefer if we marked globals as such (read: somewhere at the top of a .c
> file).

hmm, well that does increase the scope unnecessarily, which is why I
made it static inside the fxn.  But if you can prefer I guess I can
put it just above __efi_check_nesting().

> Also I think this function would be better off as a static inline in a
> header, as it should get compressed quite well.

That would mean making entry_count also non-static (ie. broader than
function or file level scope), otherwise you end up with different
copies of it in each .o file.  (Which would mostly work, but it would
fall apart in confusing ways in some edge cases..)

>> +       /* post-increment, pre-decrement: */
>> +       if (delta > 0)
>> +               return entry_count++ == 0;
>> +       else
>> +               return --entry_count == 0;
>
>
> I have a hard time to wrap my head around the logic. At least, couldn't you
> split this into 2 functions? :)

Would that make the logic more clear, or just move it far enough apart
that you don't notice it is confusing ;-)

BR,
-R

> Alex
>
>
>> +}
>> +
>>   /* Called on every callback entry */
>>   void efi_restore_gd(void)
>>   {
>> @@ -716,7 +727,9 @@ static efi_status_t EFIAPI
>> efi_start_image(efi_handle_t image_handle,
>>                 return EFI_EXIT(info->exit_status);
>>         }
>>   +     __efi_check_nesting(-1);
>>         entry(image_handle, &systab);
>> +       __efi_check_nesting(+1);
>>         /* Should usually never get here */
>>         return EFI_EXIT(EFI_SUCCESS);
>>
>