On Tue, Jun 20, 2023 at 11:27 AM Jose E. Marchesi <jemarch@xxxxxxx> wrote:
>
>
> > 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.
>
> 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@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'''
Let's add an extensive comment here,
so that people looking at bpf_helper_defs.h don't need
to search clang documentation on what this attr is doing.
I bet even compiler experts won't be able to explain 'why'
after reading the doc:
https://clang.llvm.org/docs/AttributeReference.html#nomerge
The example from the commit log should probably be in the comment too.
Other than that the approach makes sense to me.
