On Thu, Jun 15, 2023 at 7:25 AM Eduard Zingerman <eddyz87@xxxxxxxxx> 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.
>
> [1] https://lore.kernel.org/bpf/03bdf90f-f374-1e67-69d6-76dd9c8318a4@xxxxxxxx/
> [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
>
