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

[Alexander Graf]

On 26.07.17 18:41, Rob Clark wrote:
> 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().

It's a matter of taste, but in general I find it very useful to know at 
a glimpse where .bss, .rodata and .data come from.

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

Ah, no, entry_count would still be in efi_boottime.c, but the function 
using it would be in a .h file. That way we don't need to do the 
register save/restore dance we need to do for full remote function calls.

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

I can't really tell yet, but I'd say it's worth a try :).


Alex