bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eduard Zingerman <eddyz87@gmail.com>
To: bpf@vger.kernel.org, ast@kernel.org
Cc: andrii@kernel.org, daniel@iogearbox.net, martin.lau@linux.dev,
	kernel-team@fb.com, yhs@fb.com, jemarch@gnu.org,
	david.faust@oracle.com, dzq.aishenghu0@gmail.com,
	Eduard Zingerman <eddyz87@gmail.com>
Subject: [RFC bpf-next] bpf: generate 'nomerge' for map helpers in bpf_helper_defs.h
Date: Thu, 15 Jun 2023 17:25:20 +0300	[thread overview]
Message-ID: <20230615142520.10280-1-eddyz87@gmail.com> (raw)

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


             reply	other threads:[~2023-06-15 14:25 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-06-15 14:25 Eduard Zingerman [this message]
2023-06-16 17:03 ` [RFC bpf-next] bpf: generate 'nomerge' for map helpers in bpf_helper_defs.h Andrii Nakryiko
2023-06-20 18:27   ` Jose E. Marchesi
2023-06-22 21:35     ` Alexei Starovoitov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230615142520.10280-1-eddyz87@gmail.com \
    --to=eddyz87@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=david.faust@oracle.com \
    --cc=dzq.aishenghu0@gmail.com \
    --cc=jemarch@gnu.org \
    --cc=kernel-team@fb.com \
    --cc=martin.lau@linux.dev \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).