* [RFC bpf-next] bpf: generate 'nomerge' for map helpers in bpf_helper_defs.h
@ 2023-06-15 14:25 Eduard Zingerman
2023-06-16 17:03 ` Andrii Nakryiko
0 siblings, 1 reply; 4+ messages in thread
From: Eduard Zingerman @ 2023-06-15 14:25 UTC (permalink / raw)
To: bpf, ast
Cc: andrii, daniel, martin.lau, kernel-team, yhs, jemarch,
david.faust, dzq.aishenghu0, Eduard Zingerman
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?
[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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [RFC bpf-next] bpf: generate 'nomerge' for map helpers in bpf_helper_defs.h
2023-06-15 14:25 [RFC bpf-next] bpf: generate 'nomerge' for map helpers in bpf_helper_defs.h Eduard Zingerman
@ 2023-06-16 17:03 ` Andrii Nakryiko
2023-06-20 18:27 ` Jose E. Marchesi
0 siblings, 1 reply; 4+ messages in thread
From: Andrii Nakryiko @ 2023-06-16 17:03 UTC (permalink / raw)
To: Eduard Zingerman
Cc: bpf, ast, andrii, daniel, martin.lau, kernel-team, yhs, jemarch,
david.faust, dzq.aishenghu0
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.
>
> [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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC bpf-next] bpf: generate 'nomerge' for map helpers in bpf_helper_defs.h
2023-06-16 17:03 ` Andrii Nakryiko
@ 2023-06-20 18:27 ` Jose E. Marchesi
2023-06-22 21:35 ` Alexei Starovoitov
0 siblings, 1 reply; 4+ messages in thread
From: Jose E. Marchesi @ 2023-06-20 18:27 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Eduard Zingerman, bpf, ast, andrii, daniel, martin.lau,
kernel-team, yhs, david.faust, dzq.aishenghu0
> 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
>>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC bpf-next] bpf: generate 'nomerge' for map helpers in bpf_helper_defs.h
2023-06-20 18:27 ` Jose E. Marchesi
@ 2023-06-22 21:35 ` Alexei Starovoitov
0 siblings, 0 replies; 4+ messages in thread
From: Alexei Starovoitov @ 2023-06-22 21:35 UTC (permalink / raw)
To: Jose E. Marchesi
Cc: Andrii Nakryiko, Eduard Zingerman, bpf, Alexei Starovoitov,
Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau, Kernel Team,
Yonghong Song, David Faust, dzq.aishenghu0
On Tue, Jun 20, 2023 at 11:27 AM Jose E. Marchesi <jemarch@gnu.org> wrote:
>
>
> > 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'''
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-22 21:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-15 14:25 [RFC bpf-next] bpf: generate 'nomerge' for map helpers in bpf_helper_defs.h Eduard Zingerman
2023-06-16 17:03 ` Andrii Nakryiko
2023-06-20 18:27 ` Jose E. Marchesi
2023-06-22 21:35 ` Alexei Starovoitov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox