Re: [RFC bpf-next] bpf: generate 'nomerge' for map helpers in bpf_helper_defs.h

["Jose E. Marchesi" <jemarch@gnu.org>]

> On Thu, Jun 15, 2023 at 7:25 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>>
>> Update code generation for bpf_helper_defs.h by adding
>> __attribute__((nomerge)) for a set of helper functions to prevent some
>> verifier unfriendly compiler optimizations.
>>
>> This addresses a recent mailing list thread [1].
>> There Zhongqiu Duan and Yonghong Song discussed a C program as below:
>>
>>      if (data_end - data > 1024) {
>>          bpf_for_each_map_elem(&map1, cb, &cb_data, 0);
>>      } else {
>>          bpf_for_each_map_elem(&map2, cb, &cb_data, 0);
>>      }
>>
>> Which was converted by clang to something like this:
>>
>>      if (data_end - data > 1024)
>>        tmp = &map1;
>>      else
>>        tmp = &map2;
>>      bpf_for_each_map_elem(tmp, cb, &cb_data, 0);
>>
>> Which in turn triggered verification error, because
>> verifier.c:record_func_map() requires a single map address for each
>> bpf_for_each_map_elem() call.
>>
>> In fact, this is a requirement for the following helpers:
>> - bpf_tail_call
>> - bpf_map_lookup_elem
>> - bpf_map_update_elem
>> - bpf_map_delete_elem
>> - bpf_map_push_elem
>> - bpf_map_pop_elem
>> - bpf_map_peek_elem
>> - bpf_for_each_map_elem
>> - bpf_redirect_map
>> - bpf_map_lookup_percpu_elem
>>
>> I had an off-list discussion with Yonghong where we agreed that clang
>> attribute 'nomerge' (see [2]) could be used to prevent the
>> optimization hitting in [1]. However, currently 'nomerge' applies only
>> to functions and statements, hence I submitted change requests [3],
>> [4] to allow specifying 'nomerge' for function pointers as well.
>>
>> The patch below updates bpf_helper_defs.h generation by adding a
>> definition of __nomerge macro, and using this macro in definitions of
>> relevant helpers.
>>
>> The generated code looks as follows:
>>
>>     /* This is auto-generated file. See bpf_doc.py for details. */
>>
>>     #if __has_attribute(nomerge)
>>     #define __nomerge __attribute__((nomerge))
>>     #else
>>     #define __nomerge
>>     #endif
>>
>>     /* Forward declarations of BPF structs */
>>     ...
>>     static long (*bpf_for_each_map_elem)(void *map, ...) __nomerge = (void *) 164;
>>     ...
>>
>> (In non-RFC version the macro definition would have to be updated to
>>  check for supported clang version).
>>
>> Does community agree with such approach?
>
> Makes sense to me. Let's just be very careful to do proper detection
> of __nomerge "applicability" to ensure we don't cause compilation
> errors for unsupported Clang (which I'm sure you are well aware of)
> *and* make it compatible with GCC, so we don't fix it later.

GCC doesn't support the "nomerge" attribute at this point.  We will look
into adding it or find some other equivalent mechanism that can be
abstracted in the __nomerge macro.

>>
>> [1] https://lore.kernel.org/bpf/03bdf90f-f374-1e67-69d6-76dd9c8318a4@meta.com/
>> [2] https://clang.llvm.org/docs/AttributeReference.html#nomerge
>> [3] https://reviews.llvm.org/D152986
>> [4] https://reviews.llvm.org/D152987
>> ---
>>  scripts/bpf_doc.py | 37 ++++++++++++++++++++++++++++++-------
>>  1 file changed, 30 insertions(+), 7 deletions(-)
>>
>> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
>> index eaae2ce78381..dbd4893c793e 100755
>> --- a/scripts/bpf_doc.py
>> +++ b/scripts/bpf_doc.py
>> @@ -777,14 +777,33 @@ class PrinterHelpers(Printer):
>>          'bpf_get_socket_cookie',
>>          'bpf_sk_assign',
>>      ]
>> +    # Helpers that need __nomerge attribute
>> +    nomerge_helpers = set([
>> +       "bpf_tail_call",
>> +       "bpf_map_lookup_elem",
>> +       "bpf_map_update_elem",
>> +       "bpf_map_delete_elem",
>> +       "bpf_map_push_elem",
>> +       "bpf_map_pop_elem",
>> +       "bpf_map_peek_elem",
>> +       "bpf_for_each_map_elem",
>> +       "bpf_redirect_map",
>> +       "bpf_map_lookup_percpu_elem"
>> +    ])
>> +
>> +    macros = '''\
>> +#if __has_attribute(nomerge)
>> +#define __nomerge __attribute__((nomerge))
>> +#else
>> +#define __nomerge
>> +#endif'''
>>
>>      def print_header(self):
>> -        header = '''\
>> -/* This is auto-generated file. See bpf_doc.py for details. */
>> -
>> -/* Forward declarations of BPF structs */'''
>> -
>> -        print(header)
>> +        print('/* This is auto-generated file. See bpf_doc.py for details. */')
>> +        print()
>> +        print(self.macros)
>> +        print()
>> +        print('/* Forward declarations of BPF structs */')
>>          for fwd in self.type_fwds:
>>              print('%s;' % fwd)
>>          print('')
>> @@ -846,7 +865,11 @@ class PrinterHelpers(Printer):
>>              comma = ', '
>>              print(one_arg, end='')
>>
>> -        print(') = (void *) %d;' % helper.enum_val)
>> +        print(')', end='')
>> +        if proto['name'] in self.nomerge_helpers:
>> +            print(' __nomerge', end='')
>> +
>> +        print(' = (void *) %d;' % helper.enum_val)
>>          print('')
>>
>>  ###############################################################################
>> --
>> 2.40.1
>>