[PATCHv3 bpf-next 0/9] bpf: Move kernel test kfuncs into bpf_testmod

[PATCHv3 bpf-next 0/9] bpf: Move kernel test kfuncs into bpf_testmod

[Jiri Olsa]

hi,
I noticed several times in discussions that we should move test kfuncs
into kernel module, now perhaps even more pressing with all the kfunc
effort. This patchset moves all the test kfuncs into bpf_testmod.

I added bpf_testmod/bpf_testmod_kfunc.h header that is shared between
bpf_testmod kernel module and BPF programs, which brings some difficulties
with __ksym define. But I'm not sure having separate headers for BPF
programs and for kernel module would be better.

This patchset also needs:
  74bc3a5acc82 bpf: Add missing btf_put to register_btf_id_dtor_kfuncs
which is only in bpf/master now.

v3 changes:
  - added acks [David]
  - added bpf_testmod.ko make dependency for bpf test progs [David]
  - better handling of __ksym and refcount_t in bpf_testmod_kfunc.h [David]
  - removed 'extern' from kfuncs declarations [David]
  - typo in header guard macro [David]
  - use only stdout in un/load_bpf_testmod

v2 changes:
  - add 74bc3a5acc82 into bpf-next/master CI, so the test would pass
    https://github.com/kernel-patches/vmtest/pull/192
  - remove extra externs [Artem]
  - using un/load_bpf_testmod in other tests
  - rebased

thanks,
jirka


---
Jiri Olsa (9):
      selftests/bpf: Move kfunc exports to bpf_testmod/bpf_testmod_kfunc.h
      selftests/bpf: Move test_progs helpers to testing_helpers object
      selftests/bpf: Use only stdout in un/load_bpf_testmod functions
      selftests/bpf: Do not unload bpf_testmod in load_bpf_testmod
      selftests/bpf: Use un/load_bpf_testmod functions in tests
      selftests/bpf: Load bpf_testmod for verifier test
      selftests/bpf: Allow to use kfunc from testmod.ko in test_verifier
      selftests/bpf: Remove extern from kfuncs declarations
      bpf: Move kernel test kfuncs to bpf_testmod

 net/bpf/test_run.c                                          | 271 +------------------------------------------------------------------------------------------
 tools/testing/selftests/bpf/Makefile                        |   1 +
 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c       | 206 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h | 102 ++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/prog_tests/bpf_mod_race.c       |  34 ++----------
 tools/testing/selftests/bpf/prog_tests/module_attach.c      |  12 ++--
 tools/testing/selftests/bpf/progs/cb_refs.c                 |   4 +-
 tools/testing/selftests/bpf/progs/jit_probe_mem.c           |   4 +-
 tools/testing/selftests/bpf/progs/kfunc_call_destructive.c  |   3 +-
 tools/testing/selftests/bpf/progs/kfunc_call_fail.c         |   9 +--
 tools/testing/selftests/bpf/progs/kfunc_call_race.c         |   3 +-
 tools/testing/selftests/bpf/progs/kfunc_call_test.c         |  17 +-----
 tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c |   9 +--
 tools/testing/selftests/bpf/progs/map_kptr.c                |   6 +-
 tools/testing/selftests/bpf/progs/map_kptr_fail.c           |   5 +-
 tools/testing/selftests/bpf/test_progs.c                    |  76 +++-----------------------
 tools/testing/selftests/bpf/test_progs.h                    |   1 -
 tools/testing/selftests/bpf/test_verifier.c                 | 170 +++++++++++++++++++++++++++++++++++++++++++++++++--------
 tools/testing/selftests/bpf/testing_helpers.c               |  61 +++++++++++++++++++++
 tools/testing/selftests/bpf/testing_helpers.h               |  10 ++++
 20 files changed, 556 insertions(+), 448 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h

[PATCHv3 bpf-next 1/9] selftests/bpf: Move kfunc exports to bpf_testmod/bpf_testmod_kfunc.h

[Jiri Olsa]

Move all kfunc exports into separate bpf_testmod_kfunc.h header file
and include it in tests that need it.

We will move all test kfuncs into bpf_testmod in following change,
so it's convenient to have declarations in single place.

The bpf_testmod_kfunc.h is included by both bpf_testmod and bpf
programs that use test kfuncs.

As suggested by David, the bpf_testmod_kfunc.h includes vmlinux.h
and bpf/bpf_helpers.h for bpf programs build, so the declarations
have proper __ksym attribute and we can resolve all the structs.

Note in kfunc_call_test_subprog.c we can no longer use the sk_state
define from bpf_tcp_helpers.h (because it clashed with vmlinux.h)
and we need to address __sk_common.skc_state field directly.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../bpf/bpf_testmod/bpf_testmod_kfunc.h       | 41 +++++++++++++++++++
 tools/testing/selftests/bpf/progs/cb_refs.c   |  4 +-
 .../selftests/bpf/progs/jit_probe_mem.c       |  4 +-
 .../bpf/progs/kfunc_call_destructive.c        |  3 +-
 .../selftests/bpf/progs/kfunc_call_fail.c     |  9 +---
 .../selftests/bpf/progs/kfunc_call_race.c     |  3 +-
 .../selftests/bpf/progs/kfunc_call_test.c     | 17 +-------
 .../bpf/progs/kfunc_call_test_subprog.c       |  9 +---
 tools/testing/selftests/bpf/progs/map_kptr.c  |  6 +--
 .../selftests/bpf/progs/map_kptr_fail.c       |  5 +--
 10 files changed, 51 insertions(+), 50 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
new file mode 100644
index 000000000000..86d94257716a
--- /dev/null
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _BPF_TESTMOD_KFUNC_H
+#define _BPF_TESTMOD_KFUNC_H
+
+#ifndef __KERNEL__
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#else
+#define __ksym
+#endif
+
+extern struct prog_test_ref_kfunc *
+bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr) __ksym;
+extern struct prog_test_ref_kfunc *
+bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
+extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
+
+extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
+extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
+extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
+extern int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
+extern void bpf_kfunc_call_int_mem_release(int *p) __ksym;
+extern u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused) __ksym;
+
+extern void bpf_testmod_test_mod_kfunc(int i) __ksym;
+
+extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
+				__u32 c, __u64 d) __ksym;
+extern int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
+extern struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
+extern long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
+
+extern void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb) __ksym;
+extern void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
+extern void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
+extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
+
+extern void bpf_kfunc_call_test_destructive(void) __ksym;
+
+#endif /* _BPF_TESTMOD_KFUNC_H */
diff --git a/tools/testing/selftests/bpf/progs/cb_refs.c b/tools/testing/selftests/bpf/progs/cb_refs.c
index 7653df1bc787..823901c1b839 100644
--- a/tools/testing/selftests/bpf/progs/cb_refs.c
+++ b/tools/testing/selftests/bpf/progs/cb_refs.c
@@ -2,6 +2,7 @@
 #include <vmlinux.h>
 #include <bpf/bpf_tracing.h>
 #include <bpf/bpf_helpers.h>
+#include "bpf_testmod/bpf_testmod_kfunc.h"
 
 struct map_value {
 	struct prog_test_ref_kfunc __kptr_ref *ptr;
@@ -14,9 +15,6 @@ struct {
 	__uint(max_entries, 16);
 } array_map SEC(".maps");
 
-extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
-extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
-
 static __noinline int cb1(void *map, void *key, void *value, void *ctx)
 {
 	void *p = *(void **)ctx;
diff --git a/tools/testing/selftests/bpf/progs/jit_probe_mem.c b/tools/testing/selftests/bpf/progs/jit_probe_mem.c
index 2d2e61470794..96207f126054 100644
--- a/tools/testing/selftests/bpf/progs/jit_probe_mem.c
+++ b/tools/testing/selftests/bpf/progs/jit_probe_mem.c
@@ -3,13 +3,11 @@
 #include <vmlinux.h>
 #include <bpf/bpf_tracing.h>
 #include <bpf/bpf_helpers.h>
+#include "bpf_testmod/bpf_testmod_kfunc.h"
 
 static struct prog_test_ref_kfunc __kptr_ref *v;
 long total_sum = -1;
 
-extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
-extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
-
 SEC("tc")
 int test_jit_probe_mem(struct __sk_buff *ctx)
 {
diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c b/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
index 767472bc5a97..6a9b13a79ae8 100644
--- a/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
@@ -1,8 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <vmlinux.h>
 #include <bpf/bpf_helpers.h>
-
-extern void bpf_kfunc_call_test_destructive(void) __ksym;
+#include "bpf_testmod/bpf_testmod_kfunc.h"
 
 SEC("tc")
 int kfunc_destructive_test(void)
diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_fail.c b/tools/testing/selftests/bpf/progs/kfunc_call_fail.c
index b98313d391c6..e857d1c4cf5b 100644
--- a/tools/testing/selftests/bpf/progs/kfunc_call_fail.c
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_fail.c
@@ -2,14 +2,7 @@
 /* Copyright (c) 2021 Facebook */
 #include <vmlinux.h>
 #include <bpf/bpf_helpers.h>
-
-extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
-extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
-extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
-extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
-extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
-extern int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
-extern void bpf_kfunc_call_int_mem_release(int *p) __ksym;
+#include "bpf_testmod/bpf_testmod_kfunc.h"
 
 struct syscall_test_args {
 	__u8 data[16];
diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_race.c b/tools/testing/selftests/bpf/progs/kfunc_call_race.c
index 4e8fed75a4e0..a9558e434611 100644
--- a/tools/testing/selftests/bpf/progs/kfunc_call_race.c
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_race.c
@@ -1,8 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <vmlinux.h>
 #include <bpf/bpf_helpers.h>
-
-extern void bpf_testmod_test_mod_kfunc(int i) __ksym;
+#include "bpf_testmod/bpf_testmod_kfunc.h"
 
 SEC("tc")
 int kfunc_call_fail(struct __sk_buff *ctx)
diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
index 7daa8f5720b9..169fe673bebf 100644
--- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
@@ -2,22 +2,7 @@
 /* Copyright (c) 2021 Facebook */
 #include <vmlinux.h>
 #include <bpf/bpf_helpers.h>
-
-extern long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
-extern int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
-extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
-				  __u32 c, __u64 d) __ksym;
-
-extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
-extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
-extern void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb) __ksym;
-extern void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
-extern void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
-extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
-extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
-extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
-extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
-extern u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused) __ksym;
+#include "bpf_testmod/bpf_testmod_kfunc.h"
 
 SEC("tc")
 int kfunc_call_test4(struct __sk_buff *skb)
diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c b/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c
index c1fdecabeabf..5bfc8d35f782 100644
--- a/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c
@@ -1,13 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2021 Facebook */
-#include <linux/bpf.h>
-#include <bpf/bpf_helpers.h>
-#include "bpf_tcp_helpers.h"
+#include "bpf_testmod/bpf_testmod_kfunc.h"
 
 extern const int bpf_prog_active __ksym;
-extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
-				  __u32 c, __u64 d) __ksym;
-extern struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
 int active_res = -1;
 int sk_state_res = -1;
 
@@ -28,7 +23,7 @@ int __noinline f1(struct __sk_buff *skb)
 	if (active)
 		active_res = *active;
 
-	sk_state_res = bpf_kfunc_call_test3((struct sock *)sk)->sk_state;
+	sk_state_res = bpf_kfunc_call_test3((struct sock *)sk)->__sk_common.skc_state;
 
 	return (__u32)bpf_kfunc_call_test1((struct sock *)sk, 1, 2, 3, 4);
 }
diff --git a/tools/testing/selftests/bpf/progs/map_kptr.c b/tools/testing/selftests/bpf/progs/map_kptr.c
index eb8217803493..d53474f5b05b 100644
--- a/tools/testing/selftests/bpf/progs/map_kptr.c
+++ b/tools/testing/selftests/bpf/progs/map_kptr.c
@@ -2,6 +2,7 @@
 #include <vmlinux.h>
 #include <bpf/bpf_tracing.h>
 #include <bpf/bpf_helpers.h>
+#include "bpf_testmod/bpf_testmod_kfunc.h"
 
 struct map_value {
 	struct prog_test_ref_kfunc __kptr *unref_ptr;
@@ -57,11 +58,6 @@ DEFINE_MAP_OF_MAP(BPF_MAP_TYPE_HASH_OF_MAPS, hash_map, hash_of_hash_maps);
 DEFINE_MAP_OF_MAP(BPF_MAP_TYPE_HASH_OF_MAPS, hash_malloc_map, hash_of_hash_malloc_maps);
 DEFINE_MAP_OF_MAP(BPF_MAP_TYPE_HASH_OF_MAPS, lru_hash_map, hash_of_lru_hash_maps);
 
-extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
-extern struct prog_test_ref_kfunc *
-bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
-extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
-
 static void test_kptr_unref(struct map_value *v)
 {
 	struct prog_test_ref_kfunc *p;
diff --git a/tools/testing/selftests/bpf/progs/map_kptr_fail.c b/tools/testing/selftests/bpf/progs/map_kptr_fail.c
index 760e41e1a632..1358a7c9e399 100644
--- a/tools/testing/selftests/bpf/progs/map_kptr_fail.c
+++ b/tools/testing/selftests/bpf/progs/map_kptr_fail.c
@@ -4,6 +4,7 @@
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_core_read.h>
 #include "bpf_misc.h"
+#include "bpf_testmod/bpf_testmod_kfunc.h"
 
 struct map_value {
 	char buf[8];
@@ -19,10 +20,6 @@ struct array_map {
 	__uint(max_entries, 1);
 } array_map SEC(".maps");
 
-extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
-extern struct prog_test_ref_kfunc *
-bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
-
 SEC("?tc")
 __failure __msg("kptr access size must be BPF_DW")
 int size_not_bpf_dw(struct __sk_buff *ctx)
-- 
2.39.1

[PATCHv3 bpf-next 2/9] selftests/bpf: Move test_progs helpers to testing_helpers object

[Jiri Olsa]

Moving test_progs helpers to testing_helpers object so they can be
used from test_verifier in following changes.

Also adding missing ifndef header guard to testing_helpers.h header.

Using stderr instead of env.stderr because un/load_bpf_testmod helpers
will be used outside test_progs. Also at the point of calling them
in test_progs the std files are not hijacked yet and stderr is the
same as env.stderr.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/test_progs.c      | 67 +------------------
 tools/testing/selftests/bpf/test_progs.h      |  1 -
 tools/testing/selftests/bpf/testing_helpers.c | 63 +++++++++++++++++
 tools/testing/selftests/bpf/testing_helpers.h | 10 +++
 4 files changed, 75 insertions(+), 66 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 6d5e3022c75f..39ceb6a1bfc6 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -11,7 +11,6 @@
 #include <signal.h>
 #include <string.h>
 #include <execinfo.h> /* backtrace */
-#include <linux/membarrier.h>
 #include <sys/sysinfo.h> /* get_nprocs */
 #include <netinet/in.h>
 #include <sys/select.h>
@@ -616,68 +615,6 @@ int extract_build_id(char *build_id, size_t size)
 	return -1;
 }
 
-static int finit_module(int fd, const char *param_values, int flags)
-{
-	return syscall(__NR_finit_module, fd, param_values, flags);
-}
-
-static int delete_module(const char *name, int flags)
-{
-	return syscall(__NR_delete_module, name, flags);
-}
-
-/*
- * Trigger synchronize_rcu() in kernel.
- */
-int kern_sync_rcu(void)
-{
-	return syscall(__NR_membarrier, MEMBARRIER_CMD_SHARED, 0, 0);
-}
-
-static void unload_bpf_testmod(void)
-{
-	if (kern_sync_rcu())
-		fprintf(env.stderr, "Failed to trigger kernel-side RCU sync!\n");
-	if (delete_module("bpf_testmod", 0)) {
-		if (errno == ENOENT) {
-			if (verbose())
-				fprintf(stdout, "bpf_testmod.ko is already unloaded.\n");
-			return;
-		}
-		fprintf(env.stderr, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
-		return;
-	}
-	if (verbose())
-		fprintf(stdout, "Successfully unloaded bpf_testmod.ko.\n");
-}
-
-static int load_bpf_testmod(void)
-{
-	int fd;
-
-	/* ensure previous instance of the module is unloaded */
-	unload_bpf_testmod();
-
-	if (verbose())
-		fprintf(stdout, "Loading bpf_testmod.ko...\n");
-
-	fd = open("bpf_testmod.ko", O_RDONLY);
-	if (fd < 0) {
-		fprintf(env.stderr, "Can't find bpf_testmod.ko kernel module: %d\n", -errno);
-		return -ENOENT;
-	}
-	if (finit_module(fd, "", 0)) {
-		fprintf(env.stderr, "Failed to load bpf_testmod.ko into the kernel: %d\n", -errno);
-		close(fd);
-		return -EINVAL;
-	}
-	close(fd);
-
-	if (verbose())
-		fprintf(stdout, "Successfully loaded bpf_testmod.ko.\n");
-	return 0;
-}
-
 /* extern declarations for test funcs */
 #define DEFINE_TEST(name)				\
 	extern void test_##name(void) __weak;		\
@@ -1655,7 +1592,7 @@ int main(int argc, char **argv)
 	env.stderr = stderr;
 
 	env.has_testmod = true;
-	if (!env.list_test_names && load_bpf_testmod()) {
+	if (!env.list_test_names && load_bpf_testmod(verbose())) {
 		fprintf(env.stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n");
 		env.has_testmod = false;
 	}
@@ -1754,7 +1691,7 @@ int main(int argc, char **argv)
 	close(env.saved_netns_fd);
 out:
 	if (!env.list_test_names && env.has_testmod)
-		unload_bpf_testmod();
+		unload_bpf_testmod(verbose());
 
 	free_test_selector(&env.test_selector);
 	free_test_selector(&env.subtest_selector);
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index d5d51ec97ec8..b9dac3c32712 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -390,7 +390,6 @@ int bpf_find_map(const char *test, struct bpf_object *obj, const char *name);
 int compare_map_keys(int map1_fd, int map2_fd);
 int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len);
 int extract_build_id(char *build_id, size_t size);
-int kern_sync_rcu(void);
 int trigger_module_test_read(int read_sz);
 int trigger_module_test_write(int write_sz);
 int write_sysctl(const char *sysctl, const char *value);
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index 9695318e8132..3a9e7e8e5b14 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -8,6 +8,7 @@
 #include <bpf/libbpf.h>
 #include "test_progs.h"
 #include "testing_helpers.h"
+#include <linux/membarrier.h>
 
 int parse_num_list(const char *s, bool **num_set, int *num_set_len)
 {
@@ -229,3 +230,65 @@ int bpf_test_load_program(enum bpf_prog_type type, const struct bpf_insn *insns,
 
 	return bpf_prog_load(type, NULL, license, insns, insns_cnt, &opts);
 }
+
+static int finit_module(int fd, const char *param_values, int flags)
+{
+	return syscall(__NR_finit_module, fd, param_values, flags);
+}
+
+static int delete_module(const char *name, int flags)
+{
+	return syscall(__NR_delete_module, name, flags);
+}
+
+void unload_bpf_testmod(bool verbose)
+{
+	if (kern_sync_rcu())
+		fprintf(stderr, "Failed to trigger kernel-side RCU sync!\n");
+	if (delete_module("bpf_testmod", 0)) {
+		if (errno == ENOENT) {
+			if (verbose)
+				fprintf(stdout, "bpf_testmod.ko is already unloaded.\n");
+			return;
+		}
+		fprintf(stderr, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
+		return;
+	}
+	if (verbose)
+		fprintf(stdout, "Successfully unloaded bpf_testmod.ko.\n");
+}
+
+int load_bpf_testmod(bool verbose)
+{
+	int fd;
+
+	/* ensure previous instance of the module is unloaded */
+	unload_bpf_testmod(verbose);
+
+	if (verbose)
+		fprintf(stdout, "Loading bpf_testmod.ko...\n");
+
+	fd = open("bpf_testmod.ko", O_RDONLY);
+	if (fd < 0) {
+		fprintf(stderr, "Can't find bpf_testmod.ko kernel module: %d\n", -errno);
+		return -ENOENT;
+	}
+	if (finit_module(fd, "", 0)) {
+		fprintf(stderr, "Failed to load bpf_testmod.ko into the kernel: %d\n", -errno);
+		close(fd);
+		return -EINVAL;
+	}
+	close(fd);
+
+	if (verbose)
+		fprintf(stdout, "Successfully loaded bpf_testmod.ko.\n");
+	return 0;
+}
+
+/*
+ * Trigger synchronize_rcu() in kernel.
+ */
+int kern_sync_rcu(void)
+{
+	return syscall(__NR_membarrier, MEMBARRIER_CMD_SHARED, 0, 0);
+}
diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
index 6ec00bf79cb5..7356474def27 100644
--- a/tools/testing/selftests/bpf/testing_helpers.h
+++ b/tools/testing/selftests/bpf/testing_helpers.h
@@ -1,5 +1,9 @@
 /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
 /* Copyright (C) 2020 Facebook, Inc. */
+
+#ifndef __TESTING_HELPERS_H
+#define __TESTING_HELPERS_H
+
 #include <stdbool.h>
 #include <bpf/bpf.h>
 #include <bpf/libbpf.h>
@@ -20,3 +24,9 @@ struct test_filter_set;
 int parse_test_list(const char *s,
 		    struct test_filter_set *test_set,
 		    bool is_glob_pattern);
+
+int load_bpf_testmod(bool verbose);
+void unload_bpf_testmod(bool verbose);
+int kern_sync_rcu(void);
+
+#endif /* __TESTING_HELPERS_H */
-- 
2.39.1

[PATCHv3 bpf-next 3/9] selftests/bpf: Use only stdout in un/load_bpf_testmod functions

[Jiri Olsa]

We are about to use un/load_bpf_testmod functions in couple tests
and it's better  to print output to stdout,  so it's aligned with
tests ASSERT macros output, which use stdout as well.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/testing_helpers.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index 3a9e7e8e5b14..fd22c64646fc 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -244,14 +244,14 @@ static int delete_module(const char *name, int flags)
 void unload_bpf_testmod(bool verbose)
 {
 	if (kern_sync_rcu())
-		fprintf(stderr, "Failed to trigger kernel-side RCU sync!\n");
+		fprintf(stdout, "Failed to trigger kernel-side RCU sync!\n");
 	if (delete_module("bpf_testmod", 0)) {
 		if (errno == ENOENT) {
 			if (verbose)
 				fprintf(stdout, "bpf_testmod.ko is already unloaded.\n");
 			return;
 		}
-		fprintf(stderr, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
+		fprintf(stdout, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
 		return;
 	}
 	if (verbose)
@@ -270,11 +270,11 @@ int load_bpf_testmod(bool verbose)
 
 	fd = open("bpf_testmod.ko", O_RDONLY);
 	if (fd < 0) {
-		fprintf(stderr, "Can't find bpf_testmod.ko kernel module: %d\n", -errno);
+		fprintf(stdout, "Can't find bpf_testmod.ko kernel module: %d\n", -errno);
 		return -ENOENT;
 	}
 	if (finit_module(fd, "", 0)) {
-		fprintf(stderr, "Failed to load bpf_testmod.ko into the kernel: %d\n", -errno);
+		fprintf(stdout, "Failed to load bpf_testmod.ko into the kernel: %d\n", -errno);
 		close(fd);
 		return -EINVAL;
 	}
-- 
2.39.1

[PATCHv3 bpf-next 4/9] selftests/bpf: Do not unload bpf_testmod in load_bpf_testmod

[Jiri Olsa]

Do not unload bpf_testmod in load_bpf_testmod, instead call
unload_bpf_testmod separatelly.

This way we will be able use un/load_bpf_testmod functions
in other tests that un/load bpf_testmod module.

Acked-by: David Vernet <void@manifault.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/test_progs.c      | 11 ++++++++---
 tools/testing/selftests/bpf/testing_helpers.c |  3 ---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 39ceb6a1bfc6..0b4925cf6331 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -1592,9 +1592,14 @@ int main(int argc, char **argv)
 	env.stderr = stderr;
 
 	env.has_testmod = true;
-	if (!env.list_test_names && load_bpf_testmod(verbose())) {
-		fprintf(env.stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n");
-		env.has_testmod = false;
+	if (!env.list_test_names) {
+		/* ensure previous instance of the module is unloaded */
+		unload_bpf_testmod(verbose());
+
+		if (load_bpf_testmod(verbose())) {
+			fprintf(env.stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n");
+			env.has_testmod = false;
+		}
 	}
 
 	/* initializing tests */
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index fd22c64646fc..3872c36c17d4 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -262,9 +262,6 @@ int load_bpf_testmod(bool verbose)
 {
 	int fd;
 
-	/* ensure previous instance of the module is unloaded */
-	unload_bpf_testmod(verbose);
-
 	if (verbose)
 		fprintf(stdout, "Loading bpf_testmod.ko...\n");
 
-- 
2.39.1

[PATCHv3 bpf-next 5/9] selftests/bpf: Use un/load_bpf_testmod functions in tests

[Jiri Olsa]

Now that we have un/load_bpf_testmod helpers in testing_helpers.h,
we can use it in other tests and save some lines.

Acked-by: David Vernet <void@manifault.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../selftests/bpf/prog_tests/bpf_mod_race.c   | 34 +++----------------
 .../selftests/bpf/prog_tests/module_attach.c  | 12 +++----
 tools/testing/selftests/bpf/testing_helpers.c |  7 ++--
 tools/testing/selftests/bpf/testing_helpers.h |  2 +-
 4 files changed, 14 insertions(+), 41 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_mod_race.c b/tools/testing/selftests/bpf/prog_tests/bpf_mod_race.c
index a4d0cc9d3367..fe2c502e5089 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_mod_race.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_mod_race.c
@@ -11,6 +11,7 @@
 #include "ksym_race.skel.h"
 #include "bpf_mod_race.skel.h"
 #include "kfunc_call_race.skel.h"
+#include "testing_helpers.h"
 
 /* This test crafts a race between btf_try_get_module and do_init_module, and
  * checks whether btf_try_get_module handles the invocation for a well-formed
@@ -44,35 +45,10 @@ enum bpf_test_state {
 
 static _Atomic enum bpf_test_state state = _TS_INVALID;
 
-static int sys_finit_module(int fd, const char *param_values, int flags)
-{
-	return syscall(__NR_finit_module, fd, param_values, flags);
-}
-
-static int sys_delete_module(const char *name, unsigned int flags)
-{
-	return syscall(__NR_delete_module, name, flags);
-}
-
-static int load_module(const char *mod)
-{
-	int ret, fd;
-
-	fd = open("bpf_testmod.ko", O_RDONLY);
-	if (fd < 0)
-		return fd;
-
-	ret = sys_finit_module(fd, "", 0);
-	close(fd);
-	if (ret < 0)
-		return ret;
-	return 0;
-}
-
 static void *load_module_thread(void *p)
 {
 
-	if (!ASSERT_NEQ(load_module("bpf_testmod.ko"), 0, "load_module_thread must fail"))
+	if (!ASSERT_NEQ(load_bpf_testmod(false), 0, "load_module_thread must fail"))
 		atomic_store(&state, TS_MODULE_LOAD);
 	else
 		atomic_store(&state, TS_MODULE_LOAD_FAIL);
@@ -124,7 +100,7 @@ static void test_bpf_mod_race_config(const struct test_config *config)
 	if (!ASSERT_NEQ(fault_addr, MAP_FAILED, "mmap for uffd registration"))
 		return;
 
-	if (!ASSERT_OK(sys_delete_module("bpf_testmod", 0), "unload bpf_testmod"))
+	if (!ASSERT_OK(unload_bpf_testmod(false), "unload bpf_testmod"))
 		goto end_mmap;
 
 	skel = bpf_mod_race__open();
@@ -202,8 +178,8 @@ static void test_bpf_mod_race_config(const struct test_config *config)
 	bpf_mod_race__destroy(skel);
 	ASSERT_OK(kern_sync_rcu(), "kern_sync_rcu");
 end_module:
-	sys_delete_module("bpf_testmod", 0);
-	ASSERT_OK(load_module("bpf_testmod.ko"), "restore bpf_testmod");
+	unload_bpf_testmod(false);
+	ASSERT_OK(load_bpf_testmod(false), "restore bpf_testmod");
 end_mmap:
 	munmap(fault_addr, 4096);
 	atomic_store(&state, _TS_INVALID);
diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c
index 7fc01ff490db..f53d658ed080 100644
--- a/tools/testing/selftests/bpf/prog_tests/module_attach.c
+++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c
@@ -4,6 +4,7 @@
 #include <test_progs.h>
 #include <stdbool.h>
 #include "test_module_attach.skel.h"
+#include "testing_helpers.h"
 
 static int duration;
 
@@ -32,11 +33,6 @@ static int trigger_module_test_writable(int *val)
 	return 0;
 }
 
-static int delete_module(const char *name, int flags)
-{
-	return syscall(__NR_delete_module, name, flags);
-}
-
 void test_module_attach(void)
 {
 	const int READ_SZ = 456;
@@ -93,21 +89,21 @@ void test_module_attach(void)
 	if (!ASSERT_OK_PTR(link, "attach_fentry"))
 		goto cleanup;
 
-	ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
+	ASSERT_ERR(unload_bpf_testmod(false), "unload_bpf_testmod");
 	bpf_link__destroy(link);
 
 	link = bpf_program__attach(skel->progs.handle_fexit);
 	if (!ASSERT_OK_PTR(link, "attach_fexit"))
 		goto cleanup;
 
-	ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
+	ASSERT_ERR(unload_bpf_testmod(false), "unload_bpf_testmod");
 	bpf_link__destroy(link);
 
 	link = bpf_program__attach(skel->progs.kprobe_multi);
 	if (!ASSERT_OK_PTR(link, "attach_kprobe_multi"))
 		goto cleanup;
 
-	ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
+	ASSERT_ERR(unload_bpf_testmod(false), "unload_bpf_testmod");
 	bpf_link__destroy(link);
 
 cleanup:
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index 3872c36c17d4..030ed157954e 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -241,7 +241,7 @@ static int delete_module(const char *name, int flags)
 	return syscall(__NR_delete_module, name, flags);
 }
 
-void unload_bpf_testmod(bool verbose)
+int unload_bpf_testmod(bool verbose)
 {
 	if (kern_sync_rcu())
 		fprintf(stdout, "Failed to trigger kernel-side RCU sync!\n");
@@ -249,13 +249,14 @@ void unload_bpf_testmod(bool verbose)
 		if (errno == ENOENT) {
 			if (verbose)
 				fprintf(stdout, "bpf_testmod.ko is already unloaded.\n");
-			return;
+			return -1;
 		}
 		fprintf(stdout, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
-		return;
+		return -1;
 	}
 	if (verbose)
 		fprintf(stdout, "Successfully unloaded bpf_testmod.ko.\n");
+	return 0;
 }
 
 int load_bpf_testmod(bool verbose)
diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
index 7356474def27..713f8e37163d 100644
--- a/tools/testing/selftests/bpf/testing_helpers.h
+++ b/tools/testing/selftests/bpf/testing_helpers.h
@@ -26,7 +26,7 @@ int parse_test_list(const char *s,
 		    bool is_glob_pattern);
 
 int load_bpf_testmod(bool verbose);
-void unload_bpf_testmod(bool verbose);
+int unload_bpf_testmod(bool verbose);
 int kern_sync_rcu(void);
 
 #endif /* __TESTING_HELPERS_H */
-- 
2.39.1

[PATCHv3 bpf-next 6/9] selftests/bpf: Load bpf_testmod for verifier test

[Jiri Olsa]

Loading bpf_testmod kernel module for verifier test. We will
move all the tests kfuncs into bpf_testmod in following change.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/test_verifier.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 887c49dc5abd..14f11f2dfbce 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -45,6 +45,7 @@
 #include "bpf_util.h"
 #include "test_btf.h"
 #include "../../../include/linux/filter.h"
+#include "testing_helpers.h"
 
 #ifndef ENOTSUPP
 #define ENOTSUPP 524
@@ -1705,6 +1706,12 @@ static int do_test(bool unpriv, unsigned int from, unsigned int to)
 {
 	int i, passes = 0, errors = 0;
 
+	/* ensure previous instance of the module is unloaded */
+	unload_bpf_testmod(verbose);
+
+	if (load_bpf_testmod(verbose))
+		return EXIT_FAILURE;
+
 	for (i = from; i < to; i++) {
 		struct bpf_test *test = &tests[i];
 
@@ -1732,6 +1739,8 @@ static int do_test(bool unpriv, unsigned int from, unsigned int to)
 		}
 	}
 
+	unload_bpf_testmod(verbose);
+
 	printf("Summary: %d PASSED, %d SKIPPED, %d FAILED\n", passes,
 	       skips, errors);
 	return errors ? EXIT_FAILURE : EXIT_SUCCESS;
-- 
2.39.1

[PATCHv3 bpf-next 7/9] selftests/bpf: Allow to use kfunc from testmod.ko in test_verifier

[Jiri Olsa]

Currently the test_verifier allows test to specify kfunc symbol
and search for it in the kernel BTF.

Adding the possibility to search for kfunc also in bpf_testmod
module when it's not found in kernel BTF.

To find bpf_testmod btf we need to get back SYS_ADMIN cap.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/test_verifier.c | 161 +++++++++++++++++---
 1 file changed, 139 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 14f11f2dfbce..0a570195be37 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -879,8 +879,140 @@ static int create_map_kptr(void)
 	return fd;
 }
 
+static void set_root(bool set)
+{
+	__u64 caps;
+
+	if (set) {
+		if (cap_enable_effective(1ULL << CAP_SYS_ADMIN, &caps))
+			perror("cap_disable_effective(CAP_SYS_ADMIN)");
+	} else {
+		if (cap_disable_effective(1ULL << CAP_SYS_ADMIN, &caps))
+			perror("cap_disable_effective(CAP_SYS_ADMIN)");
+	}
+}
+
+static inline __u64 ptr_to_u64(const void *ptr)
+{
+	return (__u64) (unsigned long) ptr;
+}
+
+static struct btf *btf__load_testmod_btf(struct btf *vmlinux)
+{
+	struct bpf_btf_info info;
+	__u32 len = sizeof(info);
+	struct btf *btf = NULL;
+	char name[64];
+	__u32 id = 0;
+	int err, fd;
+
+	/* Iterate all loaded BTF objects and find bpf_testmod,
+	 * we need SYS_ADMIN cap for that.
+	 */
+	set_root(true);
+
+	while (true) {
+		err = bpf_btf_get_next_id(id, &id);
+		if (err) {
+			if (errno == ENOENT)
+				break;
+			perror("bpf_btf_get_next_id failed");
+			break;
+		}
+
+		fd = bpf_btf_get_fd_by_id(id);
+		if (fd < 0) {
+			if (errno == ENOENT)
+				continue;
+			perror("bpf_btf_get_fd_by_id failed");
+			break;
+		}
+
+		memset(&info, 0, sizeof(info));
+		info.name_len = sizeof(name);
+		info.name = ptr_to_u64(name);
+		len = sizeof(info);
+
+		err = bpf_obj_get_info_by_fd(fd, &info, &len);
+		if (err) {
+			close(fd);
+			perror("bpf_obj_get_info_by_fd failed");
+			break;
+		}
+
+		if (strcmp("bpf_testmod", name)) {
+			close(fd);
+			continue;
+		}
+
+		btf = btf__load_from_kernel_by_id_split(id, vmlinux);
+		if (!btf) {
+			close(fd);
+			break;
+		}
+
+		/* We need the fd to stay open so it can be used in fd_array.
+		 * The final cleanup call to btf__free will free btf object
+		 * and close the file descriptor.
+		 */
+		btf__set_fd(btf, fd);
+		break;
+	}
+
+	set_root(false);
+	return btf;
+}
+
+static struct btf *testmod_btf;
+static struct btf *vmlinux_btf;
+
+static void kfuncs_cleanup(void)
+{
+	btf__free(testmod_btf);
+	btf__free(vmlinux_btf);
+}
+
+static void fixup_prog_kfuncs(struct bpf_insn *prog, int *fd_array,
+			      struct kfunc_btf_id_pair *fixup_kfunc_btf_id)
+{
+	/* Patch in kfunc BTF IDs */
+	while (fixup_kfunc_btf_id->kfunc) {
+		int btf_id = 0;
+
+		/* try to find kfunc in kernel BTF */
+		vmlinux_btf = vmlinux_btf ?: btf__load_vmlinux_btf();
+		if (vmlinux_btf) {
+			btf_id = btf__find_by_name_kind(vmlinux_btf,
+							fixup_kfunc_btf_id->kfunc,
+							BTF_KIND_FUNC);
+			btf_id = btf_id < 0 ? 0 : btf_id;
+		}
+
+		/* kfunc not found in kernel BTF, try bpf_testmod BTF */
+		if (!btf_id) {
+			testmod_btf = testmod_btf ?: btf__load_testmod_btf(vmlinux_btf);
+			if (testmod_btf) {
+				btf_id = btf__find_by_name_kind(testmod_btf,
+								fixup_kfunc_btf_id->kfunc,
+								BTF_KIND_FUNC);
+				btf_id = btf_id < 0 ? 0 : btf_id;
+				if (btf_id) {
+					/* We put bpf_testmod module fd into fd_array
+					 * and its index 1 into instruction 'off'.
+					 */
+					*fd_array = btf__fd(testmod_btf);
+					prog[fixup_kfunc_btf_id->insn_idx].off = 1;
+				}
+			}
+		}
+
+		prog[fixup_kfunc_btf_id->insn_idx].imm = btf_id;
+		fixup_kfunc_btf_id++;
+	}
+}
+
 static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
-			  struct bpf_insn *prog, int *map_fds)
+			  struct bpf_insn *prog, int *map_fds, int *fd_array)
 {
 	int *fixup_map_hash_8b = test->fixup_map_hash_8b;
 	int *fixup_map_hash_48b = test->fixup_map_hash_48b;
@@ -905,7 +1037,6 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
 	int *fixup_map_ringbuf = test->fixup_map_ringbuf;
 	int *fixup_map_timer = test->fixup_map_timer;
 	int *fixup_map_kptr = test->fixup_map_kptr;
-	struct kfunc_btf_id_pair *fixup_kfunc_btf_id = test->fixup_kfunc_btf_id;
 
 	if (test->fill_helper) {
 		test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
@@ -1106,25 +1237,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
 		} while (*fixup_map_kptr);
 	}
 
-	/* Patch in kfunc BTF IDs */
-	if (fixup_kfunc_btf_id->kfunc) {
-		struct btf *btf;
-		int btf_id;
-
-		do {
-			btf_id = 0;
-			btf = btf__load_vmlinux_btf();
-			if (btf) {
-				btf_id = btf__find_by_name_kind(btf,
-								fixup_kfunc_btf_id->kfunc,
-								BTF_KIND_FUNC);
-				btf_id = btf_id < 0 ? 0 : btf_id;
-			}
-			btf__free(btf);
-			prog[fixup_kfunc_btf_id->insn_idx].imm = btf_id;
-			fixup_kfunc_btf_id++;
-		} while (fixup_kfunc_btf_id->kfunc);
-	}
+	fixup_prog_kfuncs(prog, fd_array, test->fixup_kfunc_btf_id);
 }
 
 struct libcap {
@@ -1451,6 +1564,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 	int run_errs, run_successes;
 	int map_fds[MAX_NR_MAPS];
 	const char *expected_err;
+	int fd_array[2] = { -1, -1 };
 	int saved_errno;
 	int fixup_skips;
 	__u32 pflags;
@@ -1464,7 +1578,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 	if (!prog_type)
 		prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
 	fixup_skips = skips;
-	do_test_fixup(test, prog_type, prog, map_fds);
+	do_test_fixup(test, prog_type, prog, map_fds, &fd_array[1]);
 	if (test->fill_insns) {
 		prog = test->fill_insns;
 		prog_len = test->prog_len;
@@ -1498,6 +1612,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 	else
 		opts.log_level = DEFAULT_LIBBPF_LOG_LEVEL;
 	opts.prog_flags = pflags;
+	if (fd_array[1] != -1)
+		opts.fd_array = &fd_array[0];
 
 	if ((prog_type == BPF_PROG_TYPE_TRACING ||
 	     prog_type == BPF_PROG_TYPE_LSM) && test->kfunc) {
@@ -1740,6 +1856,7 @@ static int do_test(bool unpriv, unsigned int from, unsigned int to)
 	}
 
 	unload_bpf_testmod(verbose);
+	kfuncs_cleanup();
 
 	printf("Summary: %d PASSED, %d SKIPPED, %d FAILED\n", passes,
 	       skips, errors);
-- 
2.39.1

[PATCHv3 bpf-next 8/9] selftests/bpf: Remove extern from kfuncs declarations

[Jiri Olsa]

There's no need to keep the extern in kfuncs declarations.

Suggested-by: David Vernet <void@manifault.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../bpf/bpf_testmod/bpf_testmod_kfunc.h       | 38 +++++++++----------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
index 86d94257716a..27d4494444c8 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
@@ -10,32 +10,32 @@
 #define __ksym
 #endif
 
-extern struct prog_test_ref_kfunc *
+struct prog_test_ref_kfunc *
 bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr) __ksym;
-extern struct prog_test_ref_kfunc *
+struct prog_test_ref_kfunc *
 bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
-extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
+void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
 
-extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
-extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
-extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
-extern int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
-extern void bpf_kfunc_call_int_mem_release(int *p) __ksym;
-extern u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused) __ksym;
+void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
+int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
+int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
+int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
+void bpf_kfunc_call_int_mem_release(int *p) __ksym;
+u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused) __ksym;
 
-extern void bpf_testmod_test_mod_kfunc(int i) __ksym;
+void bpf_testmod_test_mod_kfunc(int i) __ksym;
 
-extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
+__u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
 				__u32 c, __u64 d) __ksym;
-extern int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
-extern struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
-extern long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
+int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
+struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
+long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
 
-extern void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb) __ksym;
-extern void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
-extern void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
-extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
+void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb) __ksym;
+void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
+void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
+void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
 
-extern void bpf_kfunc_call_test_destructive(void) __ksym;
+void bpf_kfunc_call_test_destructive(void) __ksym;
 
 #endif /* _BPF_TESTMOD_KFUNC_H */
-- 
2.39.1

[PATCHv3 bpf-next 9/9] bpf: Move kernel test kfuncs to bpf_testmod

[Jiri Olsa]

Moving kernel test kfuncs into bpf_testmod kernel module,
and adding necessary init calls and BTF IDs records.

As suggested by David adding bpf_testmod.ko make dependency for
bpf programs, so they are rebuilt if we change the bpf_testmod.ko
module.

Also adding missing __bpf_kfunc to bpf_kfunc_call_test4 functions.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 net/bpf/test_run.c                            | 271 +-----------------
 tools/testing/selftests/bpf/Makefile          |   1 +
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 206 ++++++++++++-
 .../bpf/bpf_testmod/bpf_testmod_kfunc.h       |  61 ++++
 4 files changed, 268 insertions(+), 271 deletions(-)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index e6f773d12045..88fead83d8d3 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -535,217 +535,6 @@ __bpf_kfunc int bpf_modify_return_test(int a, int *b)
 	return a + *b;
 }
 
-__bpf_kfunc u64 bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
-{
-	return a + b + c + d;
-}
-
-__bpf_kfunc int bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
-{
-	return a + b;
-}
-
-__bpf_kfunc struct sock *bpf_kfunc_call_test3(struct sock *sk)
-{
-	return sk;
-}
-
-long noinline bpf_kfunc_call_test4(signed char a, short b, int c, long d)
-{
-	/* Provoke the compiler to assume that the caller has sign-extended a,
-	 * b and c on platforms where this is required (e.g. s390x).
-	 */
-	return (long)a + (long)b + (long)c + d;
-}
-
-struct prog_test_member1 {
-	int a;
-};
-
-struct prog_test_member {
-	struct prog_test_member1 m;
-	int c;
-};
-
-struct prog_test_ref_kfunc {
-	int a;
-	int b;
-	struct prog_test_member memb;
-	struct prog_test_ref_kfunc *next;
-	refcount_t cnt;
-};
-
-static struct prog_test_ref_kfunc prog_test_struct = {
-	.a = 42,
-	.b = 108,
-	.next = &prog_test_struct,
-	.cnt = REFCOUNT_INIT(1),
-};
-
-__bpf_kfunc struct prog_test_ref_kfunc *
-bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr)
-{
-	refcount_inc(&prog_test_struct.cnt);
-	return &prog_test_struct;
-}
-
-__bpf_kfunc struct prog_test_member *
-bpf_kfunc_call_memb_acquire(void)
-{
-	WARN_ON_ONCE(1);
-	return NULL;
-}
-
-__bpf_kfunc void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
-{
-	if (!p)
-		return;
-
-	refcount_dec(&p->cnt);
-}
-
-__bpf_kfunc void bpf_kfunc_call_memb_release(struct prog_test_member *p)
-{
-}
-
-__bpf_kfunc void bpf_kfunc_call_memb1_release(struct prog_test_member1 *p)
-{
-	WARN_ON_ONCE(1);
-}
-
-static int *__bpf_kfunc_call_test_get_mem(struct prog_test_ref_kfunc *p, const int size)
-{
-	if (size > 2 * sizeof(int))
-		return NULL;
-
-	return (int *)p;
-}
-
-__bpf_kfunc int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p,
-						  const int rdwr_buf_size)
-{
-	return __bpf_kfunc_call_test_get_mem(p, rdwr_buf_size);
-}
-
-__bpf_kfunc int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p,
-						    const int rdonly_buf_size)
-{
-	return __bpf_kfunc_call_test_get_mem(p, rdonly_buf_size);
-}
-
-/* the next 2 ones can't be really used for testing expect to ensure
- * that the verifier rejects the call.
- * Acquire functions must return struct pointers, so these ones are
- * failing.
- */
-__bpf_kfunc int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p,
-						    const int rdonly_buf_size)
-{
-	return __bpf_kfunc_call_test_get_mem(p, rdonly_buf_size);
-}
-
-__bpf_kfunc void bpf_kfunc_call_int_mem_release(int *p)
-{
-}
-
-__bpf_kfunc struct prog_test_ref_kfunc *
-bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **pp, int a, int b)
-{
-	struct prog_test_ref_kfunc *p = READ_ONCE(*pp);
-
-	if (!p)
-		return NULL;
-	refcount_inc(&p->cnt);
-	return p;
-}
-
-struct prog_test_pass1 {
-	int x0;
-	struct {
-		int x1;
-		struct {
-			int x2;
-			struct {
-				int x3;
-			};
-		};
-	};
-};
-
-struct prog_test_pass2 {
-	int len;
-	short arr1[4];
-	struct {
-		char arr2[4];
-		unsigned long arr3[8];
-	} x;
-};
-
-struct prog_test_fail1 {
-	void *p;
-	int x;
-};
-
-struct prog_test_fail2 {
-	int x8;
-	struct prog_test_pass1 x;
-};
-
-struct prog_test_fail3 {
-	int len;
-	char arr1[2];
-	char arr2[];
-};
-
-__bpf_kfunc void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb)
-{
-}
-
-__bpf_kfunc void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p)
-{
-}
-
-__bpf_kfunc void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p)
-{
-}
-
-__bpf_kfunc void bpf_kfunc_call_test_fail1(struct prog_test_fail1 *p)
-{
-}
-
-__bpf_kfunc void bpf_kfunc_call_test_fail2(struct prog_test_fail2 *p)
-{
-}
-
-__bpf_kfunc void bpf_kfunc_call_test_fail3(struct prog_test_fail3 *p)
-{
-}
-
-__bpf_kfunc void bpf_kfunc_call_test_mem_len_pass1(void *mem, int mem__sz)
-{
-}
-
-__bpf_kfunc void bpf_kfunc_call_test_mem_len_fail1(void *mem, int len)
-{
-}
-
-__bpf_kfunc void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len)
-{
-}
-
-__bpf_kfunc void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p)
-{
-}
-
-__bpf_kfunc void bpf_kfunc_call_test_destructive(void)
-{
-}
-
-__bpf_kfunc static u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused)
-{
-	return arg;
-}
-
 __diag_pop();
 
 BTF_SET8_START(bpf_test_modify_return_ids)
@@ -758,35 +547,6 @@ static const struct btf_kfunc_id_set bpf_test_modify_return_set = {
 	.set   = &bpf_test_modify_return_ids,
 };
 
-BTF_SET8_START(test_sk_check_kfunc_ids)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test2)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test3)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test4)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_acquire, KF_ACQUIRE | KF_RET_NULL)
-BTF_ID_FLAGS(func, bpf_kfunc_call_memb_acquire, KF_ACQUIRE | KF_RET_NULL)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_release, KF_RELEASE)
-BTF_ID_FLAGS(func, bpf_kfunc_call_memb_release, KF_RELEASE)
-BTF_ID_FLAGS(func, bpf_kfunc_call_memb1_release, KF_RELEASE)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_get_rdwr_mem, KF_RET_NULL)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_get_rdonly_mem, KF_RET_NULL)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_acq_rdonly_mem, KF_ACQUIRE | KF_RET_NULL)
-BTF_ID_FLAGS(func, bpf_kfunc_call_int_mem_release, KF_RELEASE)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_kptr_get, KF_ACQUIRE | KF_RET_NULL | KF_KPTR_GET)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_pass_ctx)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_pass1)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_pass2)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail1)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail2)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
-BTF_ID_FLAGS(func, bpf_kfunc_call_test_static_unused_arg)
-BTF_SET8_END(test_sk_check_kfunc_ids)
-
 static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
 			   u32 size, u32 headroom, u32 tailroom)
 {
@@ -1669,37 +1429,8 @@ int bpf_prog_test_run_syscall(struct bpf_prog *prog,
 	return err;
 }
 
-static const struct btf_kfunc_id_set bpf_prog_test_kfunc_set = {
-	.owner = THIS_MODULE,
-	.set   = &test_sk_check_kfunc_ids,
-};
-
-BTF_ID_LIST(bpf_prog_test_dtor_kfunc_ids)
-BTF_ID(struct, prog_test_ref_kfunc)
-BTF_ID(func, bpf_kfunc_call_test_release)
-BTF_ID(struct, prog_test_member)
-BTF_ID(func, bpf_kfunc_call_memb_release)
-
 static int __init bpf_prog_test_run_init(void)
 {
-	const struct btf_id_dtor_kfunc bpf_prog_test_dtor_kfunc[] = {
-		{
-		  .btf_id       = bpf_prog_test_dtor_kfunc_ids[0],
-		  .kfunc_btf_id = bpf_prog_test_dtor_kfunc_ids[1]
-		},
-		{
-		  .btf_id	= bpf_prog_test_dtor_kfunc_ids[2],
-		  .kfunc_btf_id = bpf_prog_test_dtor_kfunc_ids[3],
-		},
-	};
-	int ret;
-
-	ret = register_btf_fmodret_id_set(&bpf_test_modify_return_set);
-	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_prog_test_kfunc_set);
-	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_prog_test_kfunc_set);
-	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_prog_test_kfunc_set);
-	return ret ?: register_btf_id_dtor_kfuncs(bpf_prog_test_dtor_kfunc,
-						  ARRAY_SIZE(bpf_prog_test_dtor_kfunc),
-						  THIS_MODULE);
+	return register_btf_fmodret_id_set(&bpf_test_modify_return_set);
 }
 late_initcall(bpf_prog_test_run_init);
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index b2eb3201b85a..7008e49015e0 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -441,6 +441,7 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o:				\
 		     $(TRUNNER_BPF_PROGS_DIR)/%.c			\
 		     $(TRUNNER_BPF_PROGS_DIR)/*.h			\
 		     $$(INCLUDE_DIR)/vmlinux.h				\
+		     $(OUTPUT)/bpf_testmod.ko				\
 		     $(wildcard $(BPFDIR)/bpf_*.h)			\
 		     $(wildcard $(BPFDIR)/*.bpf.h)			\
 		     | $(TRUNNER_OUTPUT) $$(BPFOBJ)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 46500636d8cd..437e449d7c5c 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -9,6 +9,7 @@
 #include <linux/sysfs.h>
 #include <linux/tracepoint.h>
 #include "bpf_testmod.h"
+#include "bpf_testmod_kfunc.h"
 
 #define CREATE_TRACE_POINTS
 #include "bpf_testmod-events.h"
@@ -220,7 +221,189 @@ static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = {
 	.write = bpf_testmod_test_write,
 };
 
+__bpf_kfunc u64 bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
+{
+	return a + b + c + d;
+}
+
+__bpf_kfunc int bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
+{
+	return a + b;
+}
+
+__bpf_kfunc struct sock *bpf_kfunc_call_test3(struct sock *sk)
+{
+	return sk;
+}
+
+__bpf_kfunc long bpf_kfunc_call_test4(signed char a, short b, int c, long d)
+{
+	/* Provoke the compiler to assume that the caller has sign-extended a,
+	 * b and c on platforms where this is required (e.g. s390x).
+	 */
+	return (long)a + (long)b + (long)c + d;
+}
+
+static struct prog_test_ref_kfunc prog_test_struct = {
+	.a = 42,
+	.b = 108,
+	.next = &prog_test_struct,
+	.cnt = REFCOUNT_INIT(1),
+};
+
+__bpf_kfunc struct prog_test_ref_kfunc *
+bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr)
+{
+	refcount_inc(&prog_test_struct.cnt);
+	return &prog_test_struct;
+}
+
+__bpf_kfunc struct prog_test_member *
+bpf_kfunc_call_memb_acquire(void)
+{
+	WARN_ON_ONCE(1);
+	return NULL;
+}
+
+__bpf_kfunc void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
+{
+	if (!p)
+		return;
+
+	refcount_dec(&p->cnt);
+}
+
+__bpf_kfunc void bpf_kfunc_call_memb_release(struct prog_test_member *p)
+{
+}
+
+__bpf_kfunc void bpf_kfunc_call_memb1_release(struct prog_test_member1 *p)
+{
+	WARN_ON_ONCE(1);
+}
+
+static int *__bpf_kfunc_call_test_get_mem(struct prog_test_ref_kfunc *p, const int size)
+{
+	if (size > 2 * sizeof(int))
+		return NULL;
+
+	return (int *)p;
+}
+
+__bpf_kfunc int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p,
+						  const int rdwr_buf_size)
+{
+	return __bpf_kfunc_call_test_get_mem(p, rdwr_buf_size);
+}
+
+__bpf_kfunc int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p,
+						    const int rdonly_buf_size)
+{
+	return __bpf_kfunc_call_test_get_mem(p, rdonly_buf_size);
+}
+
+/* the next 2 ones can't be really used for testing expect to ensure
+ * that the verifier rejects the call.
+ * Acquire functions must return struct pointers, so these ones are
+ * failing.
+ */
+__bpf_kfunc int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p,
+						    const int rdonly_buf_size)
+{
+	return __bpf_kfunc_call_test_get_mem(p, rdonly_buf_size);
+}
+
+__bpf_kfunc void bpf_kfunc_call_int_mem_release(int *p)
+{
+}
+
+__bpf_kfunc struct prog_test_ref_kfunc *
+bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **pp, int a, int b)
+{
+	struct prog_test_ref_kfunc *p = READ_ONCE(*pp);
+
+	if (!p)
+		return NULL;
+	refcount_inc(&p->cnt);
+	return p;
+}
+
+__bpf_kfunc void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb)
+{
+}
+
+__bpf_kfunc void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p)
+{
+}
+
+__bpf_kfunc void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p)
+{
+}
+
+__bpf_kfunc void bpf_kfunc_call_test_fail1(struct prog_test_fail1 *p)
+{
+}
+
+__bpf_kfunc void bpf_kfunc_call_test_fail2(struct prog_test_fail2 *p)
+{
+}
+
+__bpf_kfunc void bpf_kfunc_call_test_fail3(struct prog_test_fail3 *p)
+{
+}
+
+__bpf_kfunc void bpf_kfunc_call_test_mem_len_pass1(void *mem, int mem__sz)
+{
+}
+
+__bpf_kfunc void bpf_kfunc_call_test_mem_len_fail1(void *mem, int len)
+{
+}
+
+__bpf_kfunc void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len)
+{
+}
+
+__bpf_kfunc void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p)
+{
+}
+
+__bpf_kfunc void bpf_kfunc_call_test_destructive(void)
+{
+}
+
+__bpf_kfunc static u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused)
+{
+	return arg;
+}
+
 BTF_SET8_START(bpf_testmod_check_kfunc_ids)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test2)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test3)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test4)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_acquire, KF_ACQUIRE | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_kfunc_call_memb_acquire, KF_ACQUIRE | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_release, KF_RELEASE)
+BTF_ID_FLAGS(func, bpf_kfunc_call_memb_release, KF_RELEASE)
+BTF_ID_FLAGS(func, bpf_kfunc_call_memb1_release, KF_RELEASE)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_get_rdwr_mem, KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_get_rdonly_mem, KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_acq_rdonly_mem, KF_ACQUIRE | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_kfunc_call_int_mem_release, KF_RELEASE)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_kptr_get, KF_ACQUIRE | KF_RET_NULL | KF_KPTR_GET)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_pass_ctx)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_pass1)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_pass2)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail1)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail2)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_static_unused_arg)
 BTF_ID_FLAGS(func, bpf_testmod_test_mod_kfunc)
 BTF_SET8_END(bpf_testmod_check_kfunc_ids)
 
@@ -229,13 +412,34 @@ static const struct btf_kfunc_id_set bpf_testmod_kfunc_set = {
 	.set   = &bpf_testmod_check_kfunc_ids,
 };
 
+BTF_ID_LIST(bpf_prog_test_dtor_kfunc_ids)
+BTF_ID(struct, prog_test_ref_kfunc)
+BTF_ID(func, bpf_kfunc_call_test_release)
+BTF_ID(struct, prog_test_member)
+BTF_ID(func, bpf_kfunc_call_memb_release)
+
 extern int bpf_fentry_test1(int a);
 
-static int bpf_testmod_init(void)
+static int __init bpf_testmod_init(void)
 {
+	const struct btf_id_dtor_kfunc bpf_prog_test_dtor_kfunc[] = {
+		{
+		  .btf_id       = bpf_prog_test_dtor_kfunc_ids[0],
+		  .kfunc_btf_id = bpf_prog_test_dtor_kfunc_ids[1]
+		},
+		{
+		  .btf_id	= bpf_prog_test_dtor_kfunc_ids[2],
+		  .kfunc_btf_id = bpf_prog_test_dtor_kfunc_ids[3],
+		},
+	};
 	int ret;
 
 	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_testmod_kfunc_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_testmod_kfunc_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_testmod_kfunc_set);
+	ret = ret ?: register_btf_id_dtor_kfuncs(bpf_prog_test_dtor_kfunc,
+						  ARRAY_SIZE(bpf_prog_test_dtor_kfunc),
+						  THIS_MODULE);
 	if (ret < 0)
 		return ret;
 	if (bpf_fentry_test1(0) < 0)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
index 27d4494444c8..c8bf054b1ba3 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
@@ -10,6 +10,61 @@
 #define __ksym
 #endif
 
+struct prog_test_pass1 {
+	int x0;
+	struct {
+		int x1;
+		struct {
+			int x2;
+			struct {
+				int x3;
+			};
+		};
+	};
+};
+
+struct prog_test_pass2 {
+	int len;
+	short arr1[4];
+	struct {
+		char arr2[4];
+		unsigned long arr3[8];
+	} x;
+};
+
+struct prog_test_fail1 {
+	void *p;
+	int x;
+};
+
+struct prog_test_fail2 {
+	int x8;
+	struct prog_test_pass1 x;
+};
+
+struct prog_test_fail3 {
+	int len;
+	char arr1[2];
+	char arr2[];
+};
+
+struct prog_test_member1 {
+	int a;
+};
+
+struct prog_test_member {
+	struct prog_test_member1 m;
+	int c;
+};
+
+struct prog_test_ref_kfunc {
+	int a;
+	int b;
+	struct prog_test_member memb;
+	struct prog_test_ref_kfunc *next;
+	refcount_t cnt;
+};
+
 struct prog_test_ref_kfunc *
 bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr) __ksym;
 struct prog_test_ref_kfunc *
@@ -21,7 +76,13 @@ int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int r
 int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
 int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
 void bpf_kfunc_call_int_mem_release(int *p) __ksym;
+
+/* The bpf_kfunc_call_test_static_unused_arg is defined as static,
+ * but bpf program compilation needs to see it as global symbol.
+ */
+#ifndef __KERNEL__
 u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused) __ksym;
+#endif
 
 void bpf_testmod_test_mod_kfunc(int i) __ksym;
 
-- 
2.39.1

Re: [PATCHv3 bpf-next 0/9] bpf: Move kernel test kfuncs into bpf_testmod

[Alexei Starovoitov]

On Fri, Feb 3, 2023 at 8:23 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> hi,
> I noticed several times in discussions that we should move test kfuncs
> into kernel module, now perhaps even more pressing with all the kfunc
> effort. This patchset moves all the test kfuncs into bpf_testmod.
>
> I added bpf_testmod/bpf_testmod_kfunc.h header that is shared between
> bpf_testmod kernel module and BPF programs, which brings some difficulties
> with __ksym define. But I'm not sure having separate headers for BPF
> programs and for kernel module would be better.
>
> This patchset also needs:
>   74bc3a5acc82 bpf: Add missing btf_put to register_btf_id_dtor_kfuncs
> which is only in bpf/master now.

I thought you've added this patch to CI,
but cb_refs is still failing on s390...

Re: [PATCHv3 bpf-next 0/9] bpf: Move kernel test kfuncs into bpf_testmod

[Jiri Olsa]

On Sat, Feb 04, 2023 at 01:21:13AM -0800, Alexei Starovoitov wrote:
> On Fri, Feb 3, 2023 at 8:23 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > hi,
> > I noticed several times in discussions that we should move test kfuncs
> > into kernel module, now perhaps even more pressing with all the kfunc
> > effort. This patchset moves all the test kfuncs into bpf_testmod.
> >
> > I added bpf_testmod/bpf_testmod_kfunc.h header that is shared between
> > bpf_testmod kernel module and BPF programs, which brings some difficulties
> > with __ksym define. But I'm not sure having separate headers for BPF
> > programs and for kernel module would be better.
> >
> > This patchset also needs:
> >   74bc3a5acc82 bpf: Add missing btf_put to register_btf_id_dtor_kfuncs
> > which is only in bpf/master now.
> 
> I thought you've added this patch to CI,
> but cb_refs is still failing on s390...

the CI now fails for s390 with messages like:
   2023-02-04T07:04:32.5185267Z    RES: address of kernel function bpf_kfunc_call_test_fail1 is out of range

so now that we have test kfuncs in the module, the 's32 imm' value of
the bpf call instructions can overflow when the offset between module
and kernel is greater than 2GB ... as explained in the commit that
added the verifier check:

  8cbf062a250e bpf: Reject kfunc calls that overflow insn->imm

not sure we can do anything about that on bpf side.. cc-ing s390 list
and Ilya for ideas/thoughts

maybe we could make bpf_testmod in-tree module and compile it as module
just for some archs

thoughts?

thanks,
jirka

Re: [PATCHv3 bpf-next 0/9] bpf: Move kernel test kfuncs into bpf_testmod

[Ilya Leoshkevich]

On Sun, 2023-02-05 at 19:17 +0100, Jiri Olsa wrote:
> On Sat, Feb 04, 2023 at 01:21:13AM -0800, Alexei Starovoitov wrote:
> > On Fri, Feb 3, 2023 at 8:23 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > 
> > > hi,
> > > I noticed several times in discussions that we should move test
> > > kfuncs
> > > into kernel module, now perhaps even more pressing with all the
> > > kfunc
> > > effort. This patchset moves all the test kfuncs into bpf_testmod.
> > > 
> > > I added bpf_testmod/bpf_testmod_kfunc.h header that is shared
> > > between
> > > bpf_testmod kernel module and BPF programs, which brings some
> > > difficulties
> > > with __ksym define. But I'm not sure having separate headers for
> > > BPF
> > > programs and for kernel module would be better.
> > > 
> > > This patchset also needs:
> > >   74bc3a5acc82 bpf: Add missing btf_put to
> > > register_btf_id_dtor_kfuncs
> > > which is only in bpf/master now.
> > 
> > I thought you've added this patch to CI,
> > but cb_refs is still failing on s390...
> 
> the CI now fails for s390 with messages like:
>    2023-02-04T07:04:32.5185267Z    RES: address of kernel function
> bpf_kfunc_call_test_fail1 is out of range
> 
> so now that we have test kfuncs in the module, the 's32 imm' value of
> the bpf call instructions can overflow when the offset between module
> and kernel is greater than 2GB ... as explained in the commit that
> added the verifier check:
> 
>   8cbf062a250e bpf: Reject kfunc calls that overflow insn->imm
> 
> not sure we can do anything about that on bpf side.. cc-ing s390 list
> and Ilya for ideas/thoughts
> 
> maybe we could make bpf_testmod in-tree module and compile it as
> module
> just for some archs
> 
> thoughts?

Hi,

I'd rather have this fixed - I guess the problem can affect the users.
The ksyms_module test is already denylisted because of that.
Unfortunately getting the kernel and the modules close together on
s390x is unlikely to happen in the foreseeable future.

What do you think about keeping the BTF ID inside the insn->imm field
and putting the 64-bit delta into bpf_insn_aux_data, replacing the
call_imm field that we already have there?

Best regards,
Ilya

> 
> thanks,
> jirka

Re: [PATCHv3 bpf-next 0/9] bpf: Move kernel test kfuncs into bpf_testmod

[Jiri Olsa]

On Sun, Feb 05, 2023 at 07:36:14PM +0100, Ilya Leoshkevich wrote:
> On Sun, 2023-02-05 at 19:17 +0100, Jiri Olsa wrote:
> > On Sat, Feb 04, 2023 at 01:21:13AM -0800, Alexei Starovoitov wrote:
> > > On Fri, Feb 3, 2023 at 8:23 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > 
> > > > hi,
> > > > I noticed several times in discussions that we should move test
> > > > kfuncs
> > > > into kernel module, now perhaps even more pressing with all the
> > > > kfunc
> > > > effort. This patchset moves all the test kfuncs into bpf_testmod.
> > > > 
> > > > I added bpf_testmod/bpf_testmod_kfunc.h header that is shared
> > > > between
> > > > bpf_testmod kernel module and BPF programs, which brings some
> > > > difficulties
> > > > with __ksym define. But I'm not sure having separate headers for
> > > > BPF
> > > > programs and for kernel module would be better.
> > > > 
> > > > This patchset also needs:
> > > >   74bc3a5acc82 bpf: Add missing btf_put to
> > > > register_btf_id_dtor_kfuncs
> > > > which is only in bpf/master now.
> > > 
> > > I thought you've added this patch to CI,
> > > but cb_refs is still failing on s390...
> > 
> > the CI now fails for s390 with messages like:
> >    2023-02-04T07:04:32.5185267Z    RES: address of kernel function
> > bpf_kfunc_call_test_fail1 is out of range
> > 
> > so now that we have test kfuncs in the module, the 's32 imm' value of
> > the bpf call instructions can overflow when the offset between module
> > and kernel is greater than 2GB ... as explained in the commit that
> > added the verifier check:
> > 
> >   8cbf062a250e bpf: Reject kfunc calls that overflow insn->imm
> > 
> > not sure we can do anything about that on bpf side.. cc-ing s390 list
> > and Ilya for ideas/thoughts
> > 
> > maybe we could make bpf_testmod in-tree module and compile it as
> > module
> > just for some archs
> > 
> > thoughts?
> 
> Hi,
> 
> I'd rather have this fixed - I guess the problem can affect the users.
> The ksyms_module test is already denylisted because of that.
> Unfortunately getting the kernel and the modules close together on
> s390x is unlikely to happen in the foreseeable future.
> 
> What do you think about keeping the BTF ID inside the insn->imm field
> and putting the 64-bit delta into bpf_insn_aux_data, replacing the
> call_imm field that we already have there?

seems tricky wrt other archs.. how about saving address of the kfunc
in bpf_insn_aux_data and use that in s390 jit code instead of the
'__bpf_call_base + imm' calculation

jirka

Re: [PATCHv3 bpf-next 1/9] selftests/bpf: Move kfunc exports to bpf_testmod/bpf_testmod_kfunc.h

[David Vernet]

On Fri, Feb 03, 2023 at 05:23:28PM +0100, Jiri Olsa wrote:
> Move all kfunc exports into separate bpf_testmod_kfunc.h header file
> and include it in tests that need it.
> 
> We will move all test kfuncs into bpf_testmod in following change,
> so it's convenient to have declarations in single place.
> 
> The bpf_testmod_kfunc.h is included by both bpf_testmod and bpf
> programs that use test kfuncs.
> 
> As suggested by David, the bpf_testmod_kfunc.h includes vmlinux.h
> and bpf/bpf_helpers.h for bpf programs build, so the declarations
> have proper __ksym attribute and we can resolve all the structs.
> 
> Note in kfunc_call_test_subprog.c we can no longer use the sk_state
> define from bpf_tcp_helpers.h (because it clashed with vmlinux.h)
> and we need to address __sk_common.skc_state field directly.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../bpf/bpf_testmod/bpf_testmod_kfunc.h       | 41 +++++++++++++++++++
>  tools/testing/selftests/bpf/progs/cb_refs.c   |  4 +-
>  .../selftests/bpf/progs/jit_probe_mem.c       |  4 +-
>  .../bpf/progs/kfunc_call_destructive.c        |  3 +-
>  .../selftests/bpf/progs/kfunc_call_fail.c     |  9 +---
>  .../selftests/bpf/progs/kfunc_call_race.c     |  3 +-
>  .../selftests/bpf/progs/kfunc_call_test.c     | 17 +-------
>  .../bpf/progs/kfunc_call_test_subprog.c       |  9 +---
>  tools/testing/selftests/bpf/progs/map_kptr.c  |  6 +--
>  .../selftests/bpf/progs/map_kptr_fail.c       |  5 +--
>  10 files changed, 51 insertions(+), 50 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> 
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> new file mode 100644
> index 000000000000..86d94257716a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _BPF_TESTMOD_KFUNC_H
> +#define _BPF_TESTMOD_KFUNC_H
> +
> +#ifndef __KERNEL__
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#else
> +#define __ksym
> +#endif
> +
> +extern struct prog_test_ref_kfunc *
> +bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr) __ksym;
> +extern struct prog_test_ref_kfunc *
> +bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
> +extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> +
> +extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
> +extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
> +extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> +extern int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> +extern void bpf_kfunc_call_int_mem_release(int *p) __ksym;
> +extern u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused) __ksym;
> +
> +extern void bpf_testmod_test_mod_kfunc(int i) __ksym;
> +
> +extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
> +				__u32 c, __u64 d) __ksym;
> +extern int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
> +extern struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
> +extern long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
> +
> +extern void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb) __ksym;
> +extern void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
> +extern void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
> +extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
> +
> +extern void bpf_kfunc_call_test_destructive(void) __ksym;
> +
> +#endif /* _BPF_TESTMOD_KFUNC_H */
> diff --git a/tools/testing/selftests/bpf/progs/cb_refs.c b/tools/testing/selftests/bpf/progs/cb_refs.c
> index 7653df1bc787..823901c1b839 100644
> --- a/tools/testing/selftests/bpf/progs/cb_refs.c
> +++ b/tools/testing/selftests/bpf/progs/cb_refs.c
> @@ -2,6 +2,7 @@
>  #include <vmlinux.h>
>  #include <bpf/bpf_tracing.h>
>  #include <bpf/bpf_helpers.h>
> +#include "bpf_testmod/bpf_testmod_kfunc.h"

Feel free to ignore if you disagree, but here and elsewhere, should we
do this:

#include <bpf_testmod/bpf_testmod_kfunc.h>

rather than using #include "bpf_testmod/bpf_testmod_kfunc.h". Doesn't
matter much, but IMO it's just slightly more readable to use the <> to
show that we're relying on -I rather than expecting
bpf_testmod/bpf_testmod_kfunc.h to be found at a path relative to the
progs. #include "bpf_misc.h" makes more sense because it really is
located in the progs/ directory.

Either way:

Acked-by: David Vernet <void@manifault.com>

>  
>  struct map_value {
>  	struct prog_test_ref_kfunc __kptr_ref *ptr;
> @@ -14,9 +15,6 @@ struct {
>  	__uint(max_entries, 16);
>  } array_map SEC(".maps");
>  
> -extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
> -extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> -
>  static __noinline int cb1(void *map, void *key, void *value, void *ctx)
>  {
>  	void *p = *(void **)ctx;
> diff --git a/tools/testing/selftests/bpf/progs/jit_probe_mem.c b/tools/testing/selftests/bpf/progs/jit_probe_mem.c
> index 2d2e61470794..96207f126054 100644
> --- a/tools/testing/selftests/bpf/progs/jit_probe_mem.c
> +++ b/tools/testing/selftests/bpf/progs/jit_probe_mem.c
> @@ -3,13 +3,11 @@
>  #include <vmlinux.h>
>  #include <bpf/bpf_tracing.h>
>  #include <bpf/bpf_helpers.h>
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>  
>  static struct prog_test_ref_kfunc __kptr_ref *v;
>  long total_sum = -1;
>  
> -extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
> -extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> -
>  SEC("tc")
>  int test_jit_probe_mem(struct __sk_buff *ctx)
>  {
> diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c b/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
> index 767472bc5a97..6a9b13a79ae8 100644
> --- a/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
> +++ b/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
> @@ -1,8 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <vmlinux.h>
>  #include <bpf/bpf_helpers.h>
> -
> -extern void bpf_kfunc_call_test_destructive(void) __ksym;
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>  
>  SEC("tc")
>  int kfunc_destructive_test(void)
> diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_fail.c b/tools/testing/selftests/bpf/progs/kfunc_call_fail.c
> index b98313d391c6..e857d1c4cf5b 100644
> --- a/tools/testing/selftests/bpf/progs/kfunc_call_fail.c
> +++ b/tools/testing/selftests/bpf/progs/kfunc_call_fail.c
> @@ -2,14 +2,7 @@
>  /* Copyright (c) 2021 Facebook */
>  #include <vmlinux.h>
>  #include <bpf/bpf_helpers.h>
> -
> -extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
> -extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> -extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
> -extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
> -extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> -extern int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> -extern void bpf_kfunc_call_int_mem_release(int *p) __ksym;
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>  
>  struct syscall_test_args {
>  	__u8 data[16];
> diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_race.c b/tools/testing/selftests/bpf/progs/kfunc_call_race.c
> index 4e8fed75a4e0..a9558e434611 100644
> --- a/tools/testing/selftests/bpf/progs/kfunc_call_race.c
> +++ b/tools/testing/selftests/bpf/progs/kfunc_call_race.c
> @@ -1,8 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <vmlinux.h>
>  #include <bpf/bpf_helpers.h>
> -
> -extern void bpf_testmod_test_mod_kfunc(int i) __ksym;
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>  
>  SEC("tc")
>  int kfunc_call_fail(struct __sk_buff *ctx)
> diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
> index 7daa8f5720b9..169fe673bebf 100644
> --- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c
> +++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
> @@ -2,22 +2,7 @@
>  /* Copyright (c) 2021 Facebook */
>  #include <vmlinux.h>
>  #include <bpf/bpf_helpers.h>
> -
> -extern long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
> -extern int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
> -extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
> -				  __u32 c, __u64 d) __ksym;
> -
> -extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
> -extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> -extern void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb) __ksym;
> -extern void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
> -extern void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
> -extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
> -extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
> -extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
> -extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> -extern u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused) __ksym;
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>  
>  SEC("tc")
>  int kfunc_call_test4(struct __sk_buff *skb)
> diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c b/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c
> index c1fdecabeabf..5bfc8d35f782 100644
> --- a/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c
> +++ b/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c
> @@ -1,13 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Copyright (c) 2021 Facebook */
> -#include <linux/bpf.h>
> -#include <bpf/bpf_helpers.h>
> -#include "bpf_tcp_helpers.h"
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>  
>  extern const int bpf_prog_active __ksym;
> -extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
> -				  __u32 c, __u64 d) __ksym;
> -extern struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
>  int active_res = -1;
>  int sk_state_res = -1;
>  
> @@ -28,7 +23,7 @@ int __noinline f1(struct __sk_buff *skb)
>  	if (active)
>  		active_res = *active;
>  
> -	sk_state_res = bpf_kfunc_call_test3((struct sock *)sk)->sk_state;
> +	sk_state_res = bpf_kfunc_call_test3((struct sock *)sk)->__sk_common.skc_state;
>  
>  	return (__u32)bpf_kfunc_call_test1((struct sock *)sk, 1, 2, 3, 4);
>  }
> diff --git a/tools/testing/selftests/bpf/progs/map_kptr.c b/tools/testing/selftests/bpf/progs/map_kptr.c
> index eb8217803493..d53474f5b05b 100644
> --- a/tools/testing/selftests/bpf/progs/map_kptr.c
> +++ b/tools/testing/selftests/bpf/progs/map_kptr.c
> @@ -2,6 +2,7 @@
>  #include <vmlinux.h>
>  #include <bpf/bpf_tracing.h>
>  #include <bpf/bpf_helpers.h>
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>  
>  struct map_value {
>  	struct prog_test_ref_kfunc __kptr *unref_ptr;
> @@ -57,11 +58,6 @@ DEFINE_MAP_OF_MAP(BPF_MAP_TYPE_HASH_OF_MAPS, hash_map, hash_of_hash_maps);
>  DEFINE_MAP_OF_MAP(BPF_MAP_TYPE_HASH_OF_MAPS, hash_malloc_map, hash_of_hash_malloc_maps);
>  DEFINE_MAP_OF_MAP(BPF_MAP_TYPE_HASH_OF_MAPS, lru_hash_map, hash_of_lru_hash_maps);
>  
> -extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
> -extern struct prog_test_ref_kfunc *
> -bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
> -extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> -
>  static void test_kptr_unref(struct map_value *v)
>  {
>  	struct prog_test_ref_kfunc *p;
> diff --git a/tools/testing/selftests/bpf/progs/map_kptr_fail.c b/tools/testing/selftests/bpf/progs/map_kptr_fail.c
> index 760e41e1a632..1358a7c9e399 100644
> --- a/tools/testing/selftests/bpf/progs/map_kptr_fail.c
> +++ b/tools/testing/selftests/bpf/progs/map_kptr_fail.c
> @@ -4,6 +4,7 @@
>  #include <bpf/bpf_helpers.h>
>  #include <bpf/bpf_core_read.h>
>  #include "bpf_misc.h"
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>  
>  struct map_value {
>  	char buf[8];
> @@ -19,10 +20,6 @@ struct array_map {
>  	__uint(max_entries, 1);
>  } array_map SEC(".maps");
>  
> -extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
> -extern struct prog_test_ref_kfunc *
> -bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
> -
>  SEC("?tc")
>  __failure __msg("kptr access size must be BPF_DW")
>  int size_not_bpf_dw(struct __sk_buff *ctx)
> -- 
> 2.39.1
> 

Re: [PATCHv3 bpf-next 2/9] selftests/bpf: Move test_progs helpers to testing_helpers object

[David Vernet]

On Fri, Feb 03, 2023 at 05:23:29PM +0100, Jiri Olsa wrote:
> Moving test_progs helpers to testing_helpers object so they can be
> used from test_verifier in following changes.
> 
> Also adding missing ifndef header guard to testing_helpers.h header.
> 
> Using stderr instead of env.stderr because un/load_bpf_testmod helpers
> will be used outside test_progs. Also at the point of calling them
> in test_progs the std files are not hijacked yet and stderr is the
> same as env.stderr.

Makes sense. Possibly something to clean up at another time but given
that we were being inconsistent with env.stdout and env.stderr in
load_bpf_testmod() in the first place, this seems totally fine.

Acked-by: David Vernet <void@manifault.com>

Left one question about kern_sync_rcu() below that need not block this
patch series, and can be addressed in a follow-up if it's even relevant.

> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/testing/selftests/bpf/test_progs.c      | 67 +------------------
>  tools/testing/selftests/bpf/test_progs.h      |  1 -
>  tools/testing/selftests/bpf/testing_helpers.c | 63 +++++++++++++++++
>  tools/testing/selftests/bpf/testing_helpers.h | 10 +++
>  4 files changed, 75 insertions(+), 66 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 6d5e3022c75f..39ceb6a1bfc6 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -11,7 +11,6 @@
>  #include <signal.h>
>  #include <string.h>
>  #include <execinfo.h> /* backtrace */
> -#include <linux/membarrier.h>
>  #include <sys/sysinfo.h> /* get_nprocs */
>  #include <netinet/in.h>
>  #include <sys/select.h>
> @@ -616,68 +615,6 @@ int extract_build_id(char *build_id, size_t size)
>  	return -1;
>  }
>  
> -static int finit_module(int fd, const char *param_values, int flags)
> -{
> -	return syscall(__NR_finit_module, fd, param_values, flags);
> -}
> -
> -static int delete_module(const char *name, int flags)
> -{
> -	return syscall(__NR_delete_module, name, flags);
> -}
> -
> -/*
> - * Trigger synchronize_rcu() in kernel.
> - */
> -int kern_sync_rcu(void)
> -{
> -	return syscall(__NR_membarrier, MEMBARRIER_CMD_SHARED, 0, 0);
> -}
> -
> -static void unload_bpf_testmod(void)
> -{
> -	if (kern_sync_rcu())
> -		fprintf(env.stderr, "Failed to trigger kernel-side RCU sync!\n");
> -	if (delete_module("bpf_testmod", 0)) {
> -		if (errno == ENOENT) {
> -			if (verbose())
> -				fprintf(stdout, "bpf_testmod.ko is already unloaded.\n");
> -			return;
> -		}
> -		fprintf(env.stderr, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
> -		return;
> -	}
> -	if (verbose())
> -		fprintf(stdout, "Successfully unloaded bpf_testmod.ko.\n");
> -}
> -
> -static int load_bpf_testmod(void)
> -{
> -	int fd;
> -
> -	/* ensure previous instance of the module is unloaded */
> -	unload_bpf_testmod();
> -
> -	if (verbose())
> -		fprintf(stdout, "Loading bpf_testmod.ko...\n");
> -
> -	fd = open("bpf_testmod.ko", O_RDONLY);
> -	if (fd < 0) {
> -		fprintf(env.stderr, "Can't find bpf_testmod.ko kernel module: %d\n", -errno);
> -		return -ENOENT;
> -	}
> -	if (finit_module(fd, "", 0)) {
> -		fprintf(env.stderr, "Failed to load bpf_testmod.ko into the kernel: %d\n", -errno);
> -		close(fd);
> -		return -EINVAL;
> -	}
> -	close(fd);
> -
> -	if (verbose())
> -		fprintf(stdout, "Successfully loaded bpf_testmod.ko.\n");
> -	return 0;
> -}
> -
>  /* extern declarations for test funcs */
>  #define DEFINE_TEST(name)				\
>  	extern void test_##name(void) __weak;		\
> @@ -1655,7 +1592,7 @@ int main(int argc, char **argv)
>  	env.stderr = stderr;
>  
>  	env.has_testmod = true;
> -	if (!env.list_test_names && load_bpf_testmod()) {
> +	if (!env.list_test_names && load_bpf_testmod(verbose())) {
>  		fprintf(env.stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n");
>  		env.has_testmod = false;
>  	}
> @@ -1754,7 +1691,7 @@ int main(int argc, char **argv)
>  	close(env.saved_netns_fd);
>  out:
>  	if (!env.list_test_names && env.has_testmod)
> -		unload_bpf_testmod();
> +		unload_bpf_testmod(verbose());
>  
>  	free_test_selector(&env.test_selector);
>  	free_test_selector(&env.subtest_selector);
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index d5d51ec97ec8..b9dac3c32712 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -390,7 +390,6 @@ int bpf_find_map(const char *test, struct bpf_object *obj, const char *name);
>  int compare_map_keys(int map1_fd, int map2_fd);
>  int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len);
>  int extract_build_id(char *build_id, size_t size);
> -int kern_sync_rcu(void);
>  int trigger_module_test_read(int read_sz);
>  int trigger_module_test_write(int write_sz);
>  int write_sysctl(const char *sysctl, const char *value);
> diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
> index 9695318e8132..3a9e7e8e5b14 100644
> --- a/tools/testing/selftests/bpf/testing_helpers.c
> +++ b/tools/testing/selftests/bpf/testing_helpers.c
> @@ -8,6 +8,7 @@
>  #include <bpf/libbpf.h>
>  #include "test_progs.h"
>  #include "testing_helpers.h"
> +#include <linux/membarrier.h>
>  
>  int parse_num_list(const char *s, bool **num_set, int *num_set_len)
>  {
> @@ -229,3 +230,65 @@ int bpf_test_load_program(enum bpf_prog_type type, const struct bpf_insn *insns,
>  
>  	return bpf_prog_load(type, NULL, license, insns, insns_cnt, &opts);
>  }
> +
> +static int finit_module(int fd, const char *param_values, int flags)
> +{
> +	return syscall(__NR_finit_module, fd, param_values, flags);
> +}
> +
> +static int delete_module(const char *name, int flags)
> +{
> +	return syscall(__NR_delete_module, name, flags);
> +}
> +
> +void unload_bpf_testmod(bool verbose)
> +{
> +	if (kern_sync_rcu())
> +		fprintf(stderr, "Failed to trigger kernel-side RCU sync!\n");

I realize there's no behavior change here, but out of curiosity, do you
know why we need a synchronize_rcu() here? In general this feels kind of
sketchy, and like something we should just put in bpf_testmod_exit() if
it's really needed for something in the kernel.

> +	if (delete_module("bpf_testmod", 0)) {
> +		if (errno == ENOENT) {
> +			if (verbose)
> +				fprintf(stdout, "bpf_testmod.ko is already unloaded.\n");
> +			return;
> +		}
> +		fprintf(stderr, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
> +		return;
> +	}
> +	if (verbose)
> +		fprintf(stdout, "Successfully unloaded bpf_testmod.ko.\n");
> +}
> +
> +int load_bpf_testmod(bool verbose)
> +{
> +	int fd;
> +
> +	/* ensure previous instance of the module is unloaded */
> +	unload_bpf_testmod(verbose);
> +
> +	if (verbose)
> +		fprintf(stdout, "Loading bpf_testmod.ko...\n");
> +
> +	fd = open("bpf_testmod.ko", O_RDONLY);
> +	if (fd < 0) {
> +		fprintf(stderr, "Can't find bpf_testmod.ko kernel module: %d\n", -errno);
> +		return -ENOENT;
> +	}
> +	if (finit_module(fd, "", 0)) {
> +		fprintf(stderr, "Failed to load bpf_testmod.ko into the kernel: %d\n", -errno);
> +		close(fd);
> +		return -EINVAL;
> +	}
> +	close(fd);
> +
> +	if (verbose)
> +		fprintf(stdout, "Successfully loaded bpf_testmod.ko.\n");
> +	return 0;
> +}
> +
> +/*
> + * Trigger synchronize_rcu() in kernel.
> + */
> +int kern_sync_rcu(void)
> +{
> +	return syscall(__NR_membarrier, MEMBARRIER_CMD_SHARED, 0, 0);
> +}
> diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
> index 6ec00bf79cb5..7356474def27 100644
> --- a/tools/testing/selftests/bpf/testing_helpers.h
> +++ b/tools/testing/selftests/bpf/testing_helpers.h
> @@ -1,5 +1,9 @@
>  /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
>  /* Copyright (C) 2020 Facebook, Inc. */
> +
> +#ifndef __TESTING_HELPERS_H
> +#define __TESTING_HELPERS_H
> +
>  #include <stdbool.h>
>  #include <bpf/bpf.h>
>  #include <bpf/libbpf.h>
> @@ -20,3 +24,9 @@ struct test_filter_set;
>  int parse_test_list(const char *s,
>  		    struct test_filter_set *test_set,
>  		    bool is_glob_pattern);
> +
> +int load_bpf_testmod(bool verbose);
> +void unload_bpf_testmod(bool verbose);
> +int kern_sync_rcu(void);
> +
> +#endif /* __TESTING_HELPERS_H */
> -- 
> 2.39.1
> 

Re: [PATCHv3 bpf-next 3/9] selftests/bpf: Use only stdout in un/load_bpf_testmod functions

[David Vernet]

On Fri, Feb 03, 2023 at 05:23:30PM +0100, Jiri Olsa wrote:
> We are about to use un/load_bpf_testmod functions in couple tests
> and it's better  to print output to stdout,  so it's aligned with
> tests ASSERT macros output, which use stdout as well.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: David Vernet <void@manifault.com>

Should we remove FILE *stderr from struct test_env? Seems like it might
be prudent if using it can actually cause a mismatch between testcase
output and the test runner?

> ---
>  tools/testing/selftests/bpf/testing_helpers.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
> index 3a9e7e8e5b14..fd22c64646fc 100644
> --- a/tools/testing/selftests/bpf/testing_helpers.c
> +++ b/tools/testing/selftests/bpf/testing_helpers.c
> @@ -244,14 +244,14 @@ static int delete_module(const char *name, int flags)
>  void unload_bpf_testmod(bool verbose)
>  {
>  	if (kern_sync_rcu())
> -		fprintf(stderr, "Failed to trigger kernel-side RCU sync!\n");
> +		fprintf(stdout, "Failed to trigger kernel-side RCU sync!\n");
>  	if (delete_module("bpf_testmod", 0)) {
>  		if (errno == ENOENT) {
>  			if (verbose)
>  				fprintf(stdout, "bpf_testmod.ko is already unloaded.\n");
>  			return;
>  		}
> -		fprintf(stderr, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
> +		fprintf(stdout, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
>  		return;
>  	}
>  	if (verbose)
> @@ -270,11 +270,11 @@ int load_bpf_testmod(bool verbose)
>  
>  	fd = open("bpf_testmod.ko", O_RDONLY);
>  	if (fd < 0) {
> -		fprintf(stderr, "Can't find bpf_testmod.ko kernel module: %d\n", -errno);
> +		fprintf(stdout, "Can't find bpf_testmod.ko kernel module: %d\n", -errno);
>  		return -ENOENT;
>  	}
>  	if (finit_module(fd, "", 0)) {
> -		fprintf(stderr, "Failed to load bpf_testmod.ko into the kernel: %d\n", -errno);
> +		fprintf(stdout, "Failed to load bpf_testmod.ko into the kernel: %d\n", -errno);
>  		close(fd);
>  		return -EINVAL;
>  	}
> -- 
> 2.39.1
> 

Re: [PATCHv3 bpf-next 5/9] selftests/bpf: Use un/load_bpf_testmod functions in tests

[David Vernet]

On Fri, Feb 03, 2023 at 05:23:32PM +0100, Jiri Olsa wrote:
> Now that we have un/load_bpf_testmod helpers in testing_helpers.h,
> we can use it in other tests and save some lines.
> 
> Acked-by: David Vernet <void@manifault.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../selftests/bpf/prog_tests/bpf_mod_race.c   | 34 +++----------------
>  .../selftests/bpf/prog_tests/module_attach.c  | 12 +++----
>  tools/testing/selftests/bpf/testing_helpers.c |  7 ++--
>  tools/testing/selftests/bpf/testing_helpers.h |  2 +-
>  4 files changed, 14 insertions(+), 41 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_mod_race.c b/tools/testing/selftests/bpf/prog_tests/bpf_mod_race.c
> index a4d0cc9d3367..fe2c502e5089 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_mod_race.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_mod_race.c
> @@ -11,6 +11,7 @@
>  #include "ksym_race.skel.h"
>  #include "bpf_mod_race.skel.h"
>  #include "kfunc_call_race.skel.h"
> +#include "testing_helpers.h"
>  
>  /* This test crafts a race between btf_try_get_module and do_init_module, and
>   * checks whether btf_try_get_module handles the invocation for a well-formed
> @@ -44,35 +45,10 @@ enum bpf_test_state {
>  
>  static _Atomic enum bpf_test_state state = _TS_INVALID;
>  
> -static int sys_finit_module(int fd, const char *param_values, int flags)
> -{
> -	return syscall(__NR_finit_module, fd, param_values, flags);
> -}
> -
> -static int sys_delete_module(const char *name, unsigned int flags)
> -{
> -	return syscall(__NR_delete_module, name, flags);
> -}
> -
> -static int load_module(const char *mod)
> -{
> -	int ret, fd;
> -
> -	fd = open("bpf_testmod.ko", O_RDONLY);
> -	if (fd < 0)
> -		return fd;
> -
> -	ret = sys_finit_module(fd, "", 0);
> -	close(fd);
> -	if (ret < 0)
> -		return ret;
> -	return 0;
> -}
> -
>  static void *load_module_thread(void *p)
>  {
>  
> -	if (!ASSERT_NEQ(load_module("bpf_testmod.ko"), 0, "load_module_thread must fail"))
> +	if (!ASSERT_NEQ(load_bpf_testmod(false), 0, "load_module_thread must fail"))
>  		atomic_store(&state, TS_MODULE_LOAD);
>  	else
>  		atomic_store(&state, TS_MODULE_LOAD_FAIL);
> @@ -124,7 +100,7 @@ static void test_bpf_mod_race_config(const struct test_config *config)
>  	if (!ASSERT_NEQ(fault_addr, MAP_FAILED, "mmap for uffd registration"))
>  		return;
>  
> -	if (!ASSERT_OK(sys_delete_module("bpf_testmod", 0), "unload bpf_testmod"))
> +	if (!ASSERT_OK(unload_bpf_testmod(false), "unload bpf_testmod"))
>  		goto end_mmap;
>  
>  	skel = bpf_mod_race__open();
> @@ -202,8 +178,8 @@ static void test_bpf_mod_race_config(const struct test_config *config)
>  	bpf_mod_race__destroy(skel);
>  	ASSERT_OK(kern_sync_rcu(), "kern_sync_rcu");
>  end_module:
> -	sys_delete_module("bpf_testmod", 0);
> -	ASSERT_OK(load_module("bpf_testmod.ko"), "restore bpf_testmod");
> +	unload_bpf_testmod(false);
> +	ASSERT_OK(load_bpf_testmod(false), "restore bpf_testmod");
>  end_mmap:
>  	munmap(fault_addr, 4096);
>  	atomic_store(&state, _TS_INVALID);
> diff --git a/tools/testing/selftests/bpf/prog_tests/module_attach.c b/tools/testing/selftests/bpf/prog_tests/module_attach.c
> index 7fc01ff490db..f53d658ed080 100644
> --- a/tools/testing/selftests/bpf/prog_tests/module_attach.c
> +++ b/tools/testing/selftests/bpf/prog_tests/module_attach.c
> @@ -4,6 +4,7 @@
>  #include <test_progs.h>
>  #include <stdbool.h>
>  #include "test_module_attach.skel.h"
> +#include "testing_helpers.h"
>  
>  static int duration;
>  
> @@ -32,11 +33,6 @@ static int trigger_module_test_writable(int *val)
>  	return 0;
>  }
>  
> -static int delete_module(const char *name, int flags)
> -{
> -	return syscall(__NR_delete_module, name, flags);
> -}
> -
>  void test_module_attach(void)
>  {
>  	const int READ_SZ = 456;
> @@ -93,21 +89,21 @@ void test_module_attach(void)
>  	if (!ASSERT_OK_PTR(link, "attach_fentry"))
>  		goto cleanup;
>  
> -	ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
> +	ASSERT_ERR(unload_bpf_testmod(false), "unload_bpf_testmod");
>  	bpf_link__destroy(link);
>  
>  	link = bpf_program__attach(skel->progs.handle_fexit);
>  	if (!ASSERT_OK_PTR(link, "attach_fexit"))
>  		goto cleanup;
>  
> -	ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
> +	ASSERT_ERR(unload_bpf_testmod(false), "unload_bpf_testmod");
>  	bpf_link__destroy(link);
>  
>  	link = bpf_program__attach(skel->progs.kprobe_multi);
>  	if (!ASSERT_OK_PTR(link, "attach_kprobe_multi"))
>  		goto cleanup;
>  
> -	ASSERT_ERR(delete_module("bpf_testmod", 0), "delete_module");
> +	ASSERT_ERR(unload_bpf_testmod(false), "unload_bpf_testmod");
>  	bpf_link__destroy(link);
>  
>  cleanup:
> diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
> index 3872c36c17d4..030ed157954e 100644
> --- a/tools/testing/selftests/bpf/testing_helpers.c
> +++ b/tools/testing/selftests/bpf/testing_helpers.c
> @@ -241,7 +241,7 @@ static int delete_module(const char *name, int flags)
>  	return syscall(__NR_delete_module, name, flags);
>  }
>  
> -void unload_bpf_testmod(bool verbose)
> +int unload_bpf_testmod(bool verbose)
>  {
>  	if (kern_sync_rcu())
>  		fprintf(stdout, "Failed to trigger kernel-side RCU sync!\n");

Per my question in [0], I'd be curious to know what the deal is with
this synchronize_rcu(). If it's actually important, it seems like we
should also return an error here if it fails. Otherwise, it should
probably just live in bpf_testmod_exit(). A comment explaining why it's
necessary seems useful regardless of where it is as well.

[0]: https://lore.kernel.org/bpf/Y+JiUQFyalc0aV6M@maniforge.lan/

> @@ -249,13 +249,14 @@ void unload_bpf_testmod(bool verbose)
>  		if (errno == ENOENT) {
>  			if (verbose)
>  				fprintf(stdout, "bpf_testmod.ko is already unloaded.\n");
> -			return;
> +			return -1;
>  		}
>  		fprintf(stdout, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
> -		return;
> +		return -1;
>  	}
>  	if (verbose)
>  		fprintf(stdout, "Successfully unloaded bpf_testmod.ko.\n");
> +	return 0;
>  }
>  
>  int load_bpf_testmod(bool verbose)
> diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
> index 7356474def27..713f8e37163d 100644
> --- a/tools/testing/selftests/bpf/testing_helpers.h
> +++ b/tools/testing/selftests/bpf/testing_helpers.h
> @@ -26,7 +26,7 @@ int parse_test_list(const char *s,
>  		    bool is_glob_pattern);
>  
>  int load_bpf_testmod(bool verbose);
> -void unload_bpf_testmod(bool verbose);
> +int unload_bpf_testmod(bool verbose);
>  int kern_sync_rcu(void);
>  
>  #endif /* __TESTING_HELPERS_H */
> -- 
> 2.39.1
> 

Re: [PATCHv3 bpf-next 6/9] selftests/bpf: Load bpf_testmod for verifier test

[David Vernet]

On Fri, Feb 03, 2023 at 05:23:33PM +0100, Jiri Olsa wrote:
> Loading bpf_testmod kernel module for verifier test. We will
> move all the tests kfuncs into bpf_testmod in following change.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: David Vernet <void@manifault.com>

> ---
>  tools/testing/selftests/bpf/test_verifier.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 887c49dc5abd..14f11f2dfbce 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -45,6 +45,7 @@
>  #include "bpf_util.h"
>  #include "test_btf.h"
>  #include "../../../include/linux/filter.h"
> +#include "testing_helpers.h"
>  
>  #ifndef ENOTSUPP
>  #define ENOTSUPP 524
> @@ -1705,6 +1706,12 @@ static int do_test(bool unpriv, unsigned int from, unsigned int to)
>  {
>  	int i, passes = 0, errors = 0;
>  
> +	/* ensure previous instance of the module is unloaded */
> +	unload_bpf_testmod(verbose);
> +
> +	if (load_bpf_testmod(verbose))
> +		return EXIT_FAILURE;
> +
>  	for (i = from; i < to; i++) {
>  		struct bpf_test *test = &tests[i];
>  
> @@ -1732,6 +1739,8 @@ static int do_test(bool unpriv, unsigned int from, unsigned int to)
>  		}
>  	}
>  
> +	unload_bpf_testmod(verbose);
> +
>  	printf("Summary: %d PASSED, %d SKIPPED, %d FAILED\n", passes,
>  	       skips, errors);
>  	return errors ? EXIT_FAILURE : EXIT_SUCCESS;
> -- 
> 2.39.1
> 

Re: [PATCHv3 bpf-next 7/9] selftests/bpf: Allow to use kfunc from testmod.ko in test_verifier

[David Vernet]

On Fri, Feb 03, 2023 at 05:23:34PM +0100, Jiri Olsa wrote:
> Currently the test_verifier allows test to specify kfunc symbol
> and search for it in the kernel BTF.
> 
> Adding the possibility to search for kfunc also in bpf_testmod
> module when it's not found in kernel BTF.
> 
> To find bpf_testmod btf we need to get back SYS_ADMIN cap.

This observation and any subsequent discussion is certainly outside the
scope of your patch set, but it feels like a bit of a weird /
inconsistent UX to force users to have SYS_ADMIN cap for loading kfuncs
from modules, but not from vmlinux BTF.

I realize that you need to have SYS_ADMIN cap for BPF_PROG_GET_FD_BY_ID,
BPF_MAP_GET_FD_BY_ID, etc, so the consistency makes sense there, but it
would be nice if we could eventually make the UX consistent for programs
linking against module kfuncs, because I don't really see the difference
in terms of permissions from the user's perspective.

> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

LGTM in general -- just left one comment below.

Acked-by: David Vernet <void@manifault.com>

> ---
>  tools/testing/selftests/bpf/test_verifier.c | 161 +++++++++++++++++---
>  1 file changed, 139 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 14f11f2dfbce..0a570195be37 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -879,8 +879,140 @@ static int create_map_kptr(void)
>  	return fd;
>  }
>  
> +static void set_root(bool set)
> +{
> +	__u64 caps;
> +
> +	if (set) {
> +		if (cap_enable_effective(1ULL << CAP_SYS_ADMIN, &caps))
> +			perror("cap_disable_effective(CAP_SYS_ADMIN)");
> +	} else {
> +		if (cap_disable_effective(1ULL << CAP_SYS_ADMIN, &caps))
> +			perror("cap_disable_effective(CAP_SYS_ADMIN)");
> +	}
> +}
> +
> +static inline __u64 ptr_to_u64(const void *ptr)
> +{
> +	return (__u64) (unsigned long) ptr;

Small nit / suggestion -- IMO this is slightly preferable just to keep
it a bit more in-line with the C-standard:

return (uintptr_t)ptr;

The standard of course doesn't dictate that you can do
ptr -> uintptr_t -> __u64 -> uintptr_t -> ptr, but it at least does dictate that you can do
ptr -> uintptr_t -> ptr, whereas it does not say the same for
ptr -> unsigned long -> ptr

Also, I don't think the 'inline' keyword is necessary. The compiler will
probably figure this out on its own.

> +}
> +
> +static struct btf *btf__load_testmod_btf(struct btf *vmlinux)

Would be nice if some of this code could be shared from libbpf at some
point, but ok, a cleanup for another time.

> +{
> +	struct bpf_btf_info info;
> +	__u32 len = sizeof(info);
> +	struct btf *btf = NULL;
> +	char name[64];
> +	__u32 id = 0;
> +	int err, fd;
> +
> +	/* Iterate all loaded BTF objects and find bpf_testmod,
> +	 * we need SYS_ADMIN cap for that.
> +	 */
> +	set_root(true);
> +
> +	while (true) {
> +		err = bpf_btf_get_next_id(id, &id);
> +		if (err) {
> +			if (errno == ENOENT)
> +				break;
> +			perror("bpf_btf_get_next_id failed");
> +			break;
> +		}
> +
> +		fd = bpf_btf_get_fd_by_id(id);
> +		if (fd < 0) {
> +			if (errno == ENOENT)
> +				continue;
> +			perror("bpf_btf_get_fd_by_id failed");
> +			break;
> +		}
> +
> +		memset(&info, 0, sizeof(info));
> +		info.name_len = sizeof(name);
> +		info.name = ptr_to_u64(name);
> +		len = sizeof(info);
> +
> +		err = bpf_obj_get_info_by_fd(fd, &info, &len);
> +		if (err) {
> +			close(fd);
> +			perror("bpf_obj_get_info_by_fd failed");
> +			break;
> +		}
> +
> +		if (strcmp("bpf_testmod", name)) {
> +			close(fd);
> +			continue;
> +		}
> +
> +		btf = btf__load_from_kernel_by_id_split(id, vmlinux);
> +		if (!btf) {
> +			close(fd);
> +			break;
> +		}
> +
> +		/* We need the fd to stay open so it can be used in fd_array.
> +		 * The final cleanup call to btf__free will free btf object
> +		 * and close the file descriptor.
> +		 */
> +		btf__set_fd(btf, fd);
> +		break;
> +	}
> +
> +	set_root(false);
> +	return btf;
> +}
> +
> +static struct btf *testmod_btf;
> +static struct btf *vmlinux_btf;
> +
> +static void kfuncs_cleanup(void)
> +{
> +	btf__free(testmod_btf);
> +	btf__free(vmlinux_btf);
> +}
> +
> +static void fixup_prog_kfuncs(struct bpf_insn *prog, int *fd_array,
> +			      struct kfunc_btf_id_pair *fixup_kfunc_btf_id)
> +{
> +	/* Patch in kfunc BTF IDs */
> +	while (fixup_kfunc_btf_id->kfunc) {
> +		int btf_id = 0;
> +
> +		/* try to find kfunc in kernel BTF */
> +		vmlinux_btf = vmlinux_btf ?: btf__load_vmlinux_btf();
> +		if (vmlinux_btf) {
> +			btf_id = btf__find_by_name_kind(vmlinux_btf,
> +							fixup_kfunc_btf_id->kfunc,
> +							BTF_KIND_FUNC);
> +			btf_id = btf_id < 0 ? 0 : btf_id;
> +		}
> +
> +		/* kfunc not found in kernel BTF, try bpf_testmod BTF */
> +		if (!btf_id) {
> +			testmod_btf = testmod_btf ?: btf__load_testmod_btf(vmlinux_btf);
> +			if (testmod_btf) {
> +				btf_id = btf__find_by_name_kind(testmod_btf,
> +								fixup_kfunc_btf_id->kfunc,
> +								BTF_KIND_FUNC);
> +				btf_id = btf_id < 0 ? 0 : btf_id;
> +				if (btf_id) {
> +					/* We put bpf_testmod module fd into fd_array
> +					 * and its index 1 into instruction 'off'.
> +					 */
> +					*fd_array = btf__fd(testmod_btf);
> +					prog[fixup_kfunc_btf_id->insn_idx].off = 1;
> +				}
> +			}
> +		}
> +
> +		prog[fixup_kfunc_btf_id->insn_idx].imm = btf_id;
> +		fixup_kfunc_btf_id++;
> +	}
> +}
> +
>  static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> -			  struct bpf_insn *prog, int *map_fds)
> +			  struct bpf_insn *prog, int *map_fds, int *fd_array)
>  {
>  	int *fixup_map_hash_8b = test->fixup_map_hash_8b;
>  	int *fixup_map_hash_48b = test->fixup_map_hash_48b;
> @@ -905,7 +1037,6 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
>  	int *fixup_map_ringbuf = test->fixup_map_ringbuf;
>  	int *fixup_map_timer = test->fixup_map_timer;
>  	int *fixup_map_kptr = test->fixup_map_kptr;
> -	struct kfunc_btf_id_pair *fixup_kfunc_btf_id = test->fixup_kfunc_btf_id;
>  
>  	if (test->fill_helper) {
>  		test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
> @@ -1106,25 +1237,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
>  		} while (*fixup_map_kptr);
>  	}
>  
> -	/* Patch in kfunc BTF IDs */
> -	if (fixup_kfunc_btf_id->kfunc) {
> -		struct btf *btf;
> -		int btf_id;
> -
> -		do {
> -			btf_id = 0;
> -			btf = btf__load_vmlinux_btf();
> -			if (btf) {
> -				btf_id = btf__find_by_name_kind(btf,
> -								fixup_kfunc_btf_id->kfunc,
> -								BTF_KIND_FUNC);
> -				btf_id = btf_id < 0 ? 0 : btf_id;
> -			}
> -			btf__free(btf);
> -			prog[fixup_kfunc_btf_id->insn_idx].imm = btf_id;
> -			fixup_kfunc_btf_id++;
> -		} while (fixup_kfunc_btf_id->kfunc);
> -	}
> +	fixup_prog_kfuncs(prog, fd_array, test->fixup_kfunc_btf_id);
>  }
>  
>  struct libcap {
> @@ -1451,6 +1564,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>  	int run_errs, run_successes;
>  	int map_fds[MAX_NR_MAPS];
>  	const char *expected_err;
> +	int fd_array[2] = { -1, -1 };
>  	int saved_errno;
>  	int fixup_skips;
>  	__u32 pflags;
> @@ -1464,7 +1578,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>  	if (!prog_type)
>  		prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
>  	fixup_skips = skips;
> -	do_test_fixup(test, prog_type, prog, map_fds);
> +	do_test_fixup(test, prog_type, prog, map_fds, &fd_array[1]);
>  	if (test->fill_insns) {
>  		prog = test->fill_insns;
>  		prog_len = test->prog_len;
> @@ -1498,6 +1612,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>  	else
>  		opts.log_level = DEFAULT_LIBBPF_LOG_LEVEL;
>  	opts.prog_flags = pflags;
> +	if (fd_array[1] != -1)
> +		opts.fd_array = &fd_array[0];
>  
>  	if ((prog_type == BPF_PROG_TYPE_TRACING ||
>  	     prog_type == BPF_PROG_TYPE_LSM) && test->kfunc) {
> @@ -1740,6 +1856,7 @@ static int do_test(bool unpriv, unsigned int from, unsigned int to)
>  	}
>  
>  	unload_bpf_testmod(verbose);
> +	kfuncs_cleanup();
>  
>  	printf("Summary: %d PASSED, %d SKIPPED, %d FAILED\n", passes,
>  	       skips, errors);
> -- 
> 2.39.1
> 

Re: [PATCHv3 bpf-next 8/9] selftests/bpf: Remove extern from kfuncs declarations

[David Vernet]

On Fri, Feb 03, 2023 at 05:23:35PM +0100, Jiri Olsa wrote:
> There's no need to keep the extern in kfuncs declarations.
> 
> Suggested-by: David Vernet <void@manifault.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

Acked-by: David Vernet <void@manifault.com>

>  .../bpf/bpf_testmod/bpf_testmod_kfunc.h       | 38 +++++++++----------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> index 86d94257716a..27d4494444c8 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> @@ -10,32 +10,32 @@
>  #define __ksym
>  #endif
>  
> -extern struct prog_test_ref_kfunc *
> +struct prog_test_ref_kfunc *
>  bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr) __ksym;
> -extern struct prog_test_ref_kfunc *
> +struct prog_test_ref_kfunc *
>  bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
> -extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> +void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
>  
> -extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
> -extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
> -extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> -extern int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> -extern void bpf_kfunc_call_int_mem_release(int *p) __ksym;
> -extern u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused) __ksym;
> +void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
> +int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
> +int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> +int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> +void bpf_kfunc_call_int_mem_release(int *p) __ksym;
> +u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused) __ksym;
>  
> -extern void bpf_testmod_test_mod_kfunc(int i) __ksym;
> +void bpf_testmod_test_mod_kfunc(int i) __ksym;
>  
> -extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
> +__u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
>  				__u32 c, __u64 d) __ksym;
> -extern int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
> -extern struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
> -extern long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
> +int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
> +struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
> +long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
>  
> -extern void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb) __ksym;
> -extern void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
> -extern void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
> -extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
> +void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb) __ksym;
> +void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
> +void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
> +void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
>  
> -extern void bpf_kfunc_call_test_destructive(void) __ksym;
> +void bpf_kfunc_call_test_destructive(void) __ksym;
>  
>  #endif /* _BPF_TESTMOD_KFUNC_H */
> -- 
> 2.39.1
> 

Re: [PATCHv3 bpf-next 2/9] selftests/bpf: Move test_progs helpers to testing_helpers object

[Jiri Olsa]

On Tue, Feb 07, 2023 at 08:38:09AM -0600, David Vernet wrote:
> On Fri, Feb 03, 2023 at 05:23:29PM +0100, Jiri Olsa wrote:
> > Moving test_progs helpers to testing_helpers object so they can be
> > used from test_verifier in following changes.
> > 
> > Also adding missing ifndef header guard to testing_helpers.h header.
> > 
> > Using stderr instead of env.stderr because un/load_bpf_testmod helpers
> > will be used outside test_progs. Also at the point of calling them
> > in test_progs the std files are not hijacked yet and stderr is the
> > same as env.stderr.
> 
> Makes sense. Possibly something to clean up at another time but given
> that we were being inconsistent with env.stdout and env.stderr in
> load_bpf_testmod() in the first place, this seems totally fine.

ok

> 
> Acked-by: David Vernet <void@manifault.com>
> 
> Left one question about kern_sync_rcu() below that need not block this
> patch series, and can be addressed in a follow-up if it's even relevant.

SNIP

> > +void unload_bpf_testmod(bool verbose)
> > +{
> > +	if (kern_sync_rcu())
> > +		fprintf(stderr, "Failed to trigger kernel-side RCU sync!\n");
> 
> I realize there's no behavior change here, but out of curiosity, do you
> know why we need a synchronize_rcu() here? In general this feels kind of
> sketchy, and like something we should just put in bpf_testmod_exit() if
> it's really needed for something in the kernel.

it's explained in here:

635599bace25 selftests/bpf: Sync RCU before unloading bpf_testmod

    If some of the subtests use module BTFs through ksyms, they will cause
    bpf_prog to take a refcount on bpf_testmod module, which will prevent it from
    successfully unloading. Module's refcnt is decremented when bpf_prog is freed,
    which generally happens in RCU callback. So we need to trigger
    syncronize_rcu() in the kernel, which can be achieved nicely with
    membarrier(MEMBARRIER_CMD_SHARED) or membarrier(MEMBARRIER_CMD_GLOBAL) syscall.
    So do that in kernel_sync_rcu() and make it available to other test inside the
    test_progs. This synchronize_rcu() is called before attempting to unload
    bpf_testmod.

jirka

> 
> > +	if (delete_module("bpf_testmod", 0)) {
> > +		if (errno == ENOENT) {
> > +			if (verbose)
> > +				fprintf(stdout, "bpf_testmod.ko is already unloaded.\n");
> > +			return;
> > +		}
> > +		fprintf(stderr, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
> > +		return;
> > +	}
> > +	if (verbose)
> > +		fprintf(stdout, "Successfully unloaded bpf_testmod.ko.\n");
> > +}
> > +

SNIP

Re: [PATCHv3 bpf-next 3/9] selftests/bpf: Use only stdout in un/load_bpf_testmod functions

[Jiri Olsa]

On Tue, Feb 07, 2023 at 08:41:38AM -0600, David Vernet wrote:
> On Fri, Feb 03, 2023 at 05:23:30PM +0100, Jiri Olsa wrote:
> > We are about to use un/load_bpf_testmod functions in couple tests
> > and it's better  to print output to stdout,  so it's aligned with
> > tests ASSERT macros output, which use stdout as well.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> Acked-by: David Vernet <void@manifault.com>
> 
> Should we remove FILE *stderr from struct test_env? Seems like it might
> be prudent if using it can actually cause a mismatch between testcase
> output and the test runner?

we still seem to use stderr in few places, not sure if it's
in the output hijack window.. I'll try to check

jirka

> 
> > ---
> >  tools/testing/selftests/bpf/testing_helpers.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
> > index 3a9e7e8e5b14..fd22c64646fc 100644
> > --- a/tools/testing/selftests/bpf/testing_helpers.c
> > +++ b/tools/testing/selftests/bpf/testing_helpers.c
> > @@ -244,14 +244,14 @@ static int delete_module(const char *name, int flags)
> >  void unload_bpf_testmod(bool verbose)
> >  {
> >  	if (kern_sync_rcu())
> > -		fprintf(stderr, "Failed to trigger kernel-side RCU sync!\n");
> > +		fprintf(stdout, "Failed to trigger kernel-side RCU sync!\n");
> >  	if (delete_module("bpf_testmod", 0)) {
> >  		if (errno == ENOENT) {
> >  			if (verbose)
> >  				fprintf(stdout, "bpf_testmod.ko is already unloaded.\n");
> >  			return;
> >  		}
> > -		fprintf(stderr, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
> > +		fprintf(stdout, "Failed to unload bpf_testmod.ko from kernel: %d\n", -errno);
> >  		return;
> >  	}
> >  	if (verbose)
> > @@ -270,11 +270,11 @@ int load_bpf_testmod(bool verbose)
> >  
> >  	fd = open("bpf_testmod.ko", O_RDONLY);
> >  	if (fd < 0) {
> > -		fprintf(stderr, "Can't find bpf_testmod.ko kernel module: %d\n", -errno);
> > +		fprintf(stdout, "Can't find bpf_testmod.ko kernel module: %d\n", -errno);
> >  		return -ENOENT;
> >  	}
> >  	if (finit_module(fd, "", 0)) {
> > -		fprintf(stderr, "Failed to load bpf_testmod.ko into the kernel: %d\n", -errno);
> > +		fprintf(stdout, "Failed to load bpf_testmod.ko into the kernel: %d\n", -errno);
> >  		close(fd);
> >  		return -EINVAL;
> >  	}
> > -- 
> > 2.39.1
> > 

Re: [PATCHv3 bpf-next 7/9] selftests/bpf: Allow to use kfunc from testmod.ko in test_verifier

[Jiri Olsa]

On Tue, Feb 07, 2023 at 09:34:26AM -0600, David Vernet wrote:
> On Fri, Feb 03, 2023 at 05:23:34PM +0100, Jiri Olsa wrote:
> > Currently the test_verifier allows test to specify kfunc symbol
> > and search for it in the kernel BTF.
> > 
> > Adding the possibility to search for kfunc also in bpf_testmod
> > module when it's not found in kernel BTF.
> > 
> > To find bpf_testmod btf we need to get back SYS_ADMIN cap.
> 
> This observation and any subsequent discussion is certainly outside the
> scope of your patch set, but it feels like a bit of a weird /
> inconsistent UX to force users to have SYS_ADMIN cap for loading kfuncs
> from modules, but not from vmlinux BTF.
> 
> I realize that you need to have SYS_ADMIN cap for BPF_PROG_GET_FD_BY_ID,
> BPF_MAP_GET_FD_BY_ID, etc, so the consistency makes sense there, but it
> would be nice if we could eventually make the UX consistent for programs
> linking against module kfuncs, because I don't really see the difference
> in terms of permissions from the user's perspective.

right, it's tricky.. I'm not sure if BPF_PROG_GET_FD_BY_ID could
work just with CAP_BPF.. will check

> 
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> LGTM in general -- just left one comment below.
> 
> Acked-by: David Vernet <void@manifault.com>
> 
> > ---
> >  tools/testing/selftests/bpf/test_verifier.c | 161 +++++++++++++++++---
> >  1 file changed, 139 insertions(+), 22 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index 14f11f2dfbce..0a570195be37 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -879,8 +879,140 @@ static int create_map_kptr(void)
> >  	return fd;
> >  }
> >  
> > +static void set_root(bool set)
> > +{
> > +	__u64 caps;
> > +
> > +	if (set) {
> > +		if (cap_enable_effective(1ULL << CAP_SYS_ADMIN, &caps))
> > +			perror("cap_disable_effective(CAP_SYS_ADMIN)");
> > +	} else {
> > +		if (cap_disable_effective(1ULL << CAP_SYS_ADMIN, &caps))
> > +			perror("cap_disable_effective(CAP_SYS_ADMIN)");
> > +	}
> > +}
> > +
> > +static inline __u64 ptr_to_u64(const void *ptr)
> > +{
> > +	return (__u64) (unsigned long) ptr;
> 
> Small nit / suggestion -- IMO this is slightly preferable just to keep
> it a bit more in-line with the C-standard:
> 
> return (uintptr_t)ptr;
> 
> The standard of course doesn't dictate that you can do
> ptr -> uintptr_t -> __u64 -> uintptr_t -> ptr, but it at least does dictate that you can do
> ptr -> uintptr_t -> ptr, whereas it does not say the same for
> ptr -> unsigned long -> ptr
> 
> Also, I don't think the 'inline' keyword is necessary. The compiler will
> probably figure this out on its own.

I copy&paste the ptr_to_u64 from some other test, sounds good, will check

> 
> > +}
> > +
> > +static struct btf *btf__load_testmod_btf(struct btf *vmlinux)
> 
> Would be nice if some of this code could be shared from libbpf at some
> point, but ok, a cleanup for another time.

ok

thanks,
jirka

> 
> > +{
> > +	struct bpf_btf_info info;
> > +	__u32 len = sizeof(info);
> > +	struct btf *btf = NULL;
> > +	char name[64];
> > +	__u32 id = 0;
> > +	int err, fd;
> > +
> > +	/* Iterate all loaded BTF objects and find bpf_testmod,
> > +	 * we need SYS_ADMIN cap for that.
> > +	 */
> > +	set_root(true);
> > +
> > +	while (true) {
> > +		err = bpf_btf_get_next_id(id, &id);
> > +		if (err) {
> > +			if (errno == ENOENT)
> > +				break;
> > +			perror("bpf_btf_get_next_id failed");
> > +			break;
> > +		}
> > +
> > +		fd = bpf_btf_get_fd_by_id(id);
> > +		if (fd < 0) {
> > +			if (errno == ENOENT)
> > +				continue;
> > +			perror("bpf_btf_get_fd_by_id failed");
> > +			break;
> > +		}
> > +
> > +		memset(&info, 0, sizeof(info));
> > +		info.name_len = sizeof(name);
> > +		info.name = ptr_to_u64(name);
> > +		len = sizeof(info);
> > +
> > +		err = bpf_obj_get_info_by_fd(fd, &info, &len);
> > +		if (err) {
> > +			close(fd);
> > +			perror("bpf_obj_get_info_by_fd failed");
> > +			break;
> > +		}
> > +
> > +		if (strcmp("bpf_testmod", name)) {
> > +			close(fd);
> > +			continue;
> > +		}
> > +
> > +		btf = btf__load_from_kernel_by_id_split(id, vmlinux);
> > +		if (!btf) {
> > +			close(fd);
> > +			break;
> > +		}
> > +
> > +		/* We need the fd to stay open so it can be used in fd_array.
> > +		 * The final cleanup call to btf__free will free btf object
> > +		 * and close the file descriptor.
> > +		 */
> > +		btf__set_fd(btf, fd);
> > +		break;
> > +	}
> > +
> > +	set_root(false);
> > +	return btf;
> > +}
> > +
> > +static struct btf *testmod_btf;
> > +static struct btf *vmlinux_btf;
> > +
> > +static void kfuncs_cleanup(void)
> > +{
> > +	btf__free(testmod_btf);
> > +	btf__free(vmlinux_btf);
> > +}
> > +
> > +static void fixup_prog_kfuncs(struct bpf_insn *prog, int *fd_array,
> > +			      struct kfunc_btf_id_pair *fixup_kfunc_btf_id)
> > +{
> > +	/* Patch in kfunc BTF IDs */
> > +	while (fixup_kfunc_btf_id->kfunc) {
> > +		int btf_id = 0;
> > +
> > +		/* try to find kfunc in kernel BTF */
> > +		vmlinux_btf = vmlinux_btf ?: btf__load_vmlinux_btf();
> > +		if (vmlinux_btf) {
> > +			btf_id = btf__find_by_name_kind(vmlinux_btf,
> > +							fixup_kfunc_btf_id->kfunc,
> > +							BTF_KIND_FUNC);
> > +			btf_id = btf_id < 0 ? 0 : btf_id;
> > +		}
> > +
> > +		/* kfunc not found in kernel BTF, try bpf_testmod BTF */
> > +		if (!btf_id) {
> > +			testmod_btf = testmod_btf ?: btf__load_testmod_btf(vmlinux_btf);
> > +			if (testmod_btf) {
> > +				btf_id = btf__find_by_name_kind(testmod_btf,
> > +								fixup_kfunc_btf_id->kfunc,
> > +								BTF_KIND_FUNC);
> > +				btf_id = btf_id < 0 ? 0 : btf_id;
> > +				if (btf_id) {
> > +					/* We put bpf_testmod module fd into fd_array
> > +					 * and its index 1 into instruction 'off'.
> > +					 */
> > +					*fd_array = btf__fd(testmod_btf);
> > +					prog[fixup_kfunc_btf_id->insn_idx].off = 1;
> > +				}
> > +			}
> > +		}
> > +
> > +		prog[fixup_kfunc_btf_id->insn_idx].imm = btf_id;
> > +		fixup_kfunc_btf_id++;
> > +	}
> > +}
> > +
> >  static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> > -			  struct bpf_insn *prog, int *map_fds)
> > +			  struct bpf_insn *prog, int *map_fds, int *fd_array)
> >  {
> >  	int *fixup_map_hash_8b = test->fixup_map_hash_8b;
> >  	int *fixup_map_hash_48b = test->fixup_map_hash_48b;
> > @@ -905,7 +1037,6 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> >  	int *fixup_map_ringbuf = test->fixup_map_ringbuf;
> >  	int *fixup_map_timer = test->fixup_map_timer;
> >  	int *fixup_map_kptr = test->fixup_map_kptr;
> > -	struct kfunc_btf_id_pair *fixup_kfunc_btf_id = test->fixup_kfunc_btf_id;
> >  
> >  	if (test->fill_helper) {
> >  		test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
> > @@ -1106,25 +1237,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> >  		} while (*fixup_map_kptr);
> >  	}
> >  
> > -	/* Patch in kfunc BTF IDs */
> > -	if (fixup_kfunc_btf_id->kfunc) {
> > -		struct btf *btf;
> > -		int btf_id;
> > -
> > -		do {
> > -			btf_id = 0;
> > -			btf = btf__load_vmlinux_btf();
> > -			if (btf) {
> > -				btf_id = btf__find_by_name_kind(btf,
> > -								fixup_kfunc_btf_id->kfunc,
> > -								BTF_KIND_FUNC);
> > -				btf_id = btf_id < 0 ? 0 : btf_id;
> > -			}
> > -			btf__free(btf);
> > -			prog[fixup_kfunc_btf_id->insn_idx].imm = btf_id;
> > -			fixup_kfunc_btf_id++;
> > -		} while (fixup_kfunc_btf_id->kfunc);
> > -	}
> > +	fixup_prog_kfuncs(prog, fd_array, test->fixup_kfunc_btf_id);
> >  }
> >  
> >  struct libcap {
> > @@ -1451,6 +1564,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >  	int run_errs, run_successes;
> >  	int map_fds[MAX_NR_MAPS];
> >  	const char *expected_err;
> > +	int fd_array[2] = { -1, -1 };
> >  	int saved_errno;
> >  	int fixup_skips;
> >  	__u32 pflags;
> > @@ -1464,7 +1578,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >  	if (!prog_type)
> >  		prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> >  	fixup_skips = skips;
> > -	do_test_fixup(test, prog_type, prog, map_fds);
> > +	do_test_fixup(test, prog_type, prog, map_fds, &fd_array[1]);
> >  	if (test->fill_insns) {
> >  		prog = test->fill_insns;
> >  		prog_len = test->prog_len;
> > @@ -1498,6 +1612,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >  	else
> >  		opts.log_level = DEFAULT_LIBBPF_LOG_LEVEL;
> >  	opts.prog_flags = pflags;
> > +	if (fd_array[1] != -1)
> > +		opts.fd_array = &fd_array[0];
> >  
> >  	if ((prog_type == BPF_PROG_TYPE_TRACING ||
> >  	     prog_type == BPF_PROG_TYPE_LSM) && test->kfunc) {
> > @@ -1740,6 +1856,7 @@ static int do_test(bool unpriv, unsigned int from, unsigned int to)
> >  	}
> >  
> >  	unload_bpf_testmod(verbose);
> > +	kfuncs_cleanup();
> >  
> >  	printf("Summary: %d PASSED, %d SKIPPED, %d FAILED\n", passes,
> >  	       skips, errors);
> > -- 
> > 2.39.1
> > 

Re: [PATCHv3 bpf-next 1/9] selftests/bpf: Move kfunc exports to bpf_testmod/bpf_testmod_kfunc.h

[Andrii Nakryiko]

On Tue, Feb 7, 2023 at 6:28 AM David Vernet <void@manifault.com> wrote:
>
> On Fri, Feb 03, 2023 at 05:23:28PM +0100, Jiri Olsa wrote:
> > Move all kfunc exports into separate bpf_testmod_kfunc.h header file
> > and include it in tests that need it.
> >
> > We will move all test kfuncs into bpf_testmod in following change,
> > so it's convenient to have declarations in single place.
> >
> > The bpf_testmod_kfunc.h is included by both bpf_testmod and bpf
> > programs that use test kfuncs.
> >
> > As suggested by David, the bpf_testmod_kfunc.h includes vmlinux.h
> > and bpf/bpf_helpers.h for bpf programs build, so the declarations
> > have proper __ksym attribute and we can resolve all the structs.
> >
> > Note in kfunc_call_test_subprog.c we can no longer use the sk_state
> > define from bpf_tcp_helpers.h (because it clashed with vmlinux.h)
> > and we need to address __sk_common.skc_state field directly.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../bpf/bpf_testmod/bpf_testmod_kfunc.h       | 41 +++++++++++++++++++
> >  tools/testing/selftests/bpf/progs/cb_refs.c   |  4 +-
> >  .../selftests/bpf/progs/jit_probe_mem.c       |  4 +-
> >  .../bpf/progs/kfunc_call_destructive.c        |  3 +-
> >  .../selftests/bpf/progs/kfunc_call_fail.c     |  9 +---
> >  .../selftests/bpf/progs/kfunc_call_race.c     |  3 +-
> >  .../selftests/bpf/progs/kfunc_call_test.c     | 17 +-------
> >  .../bpf/progs/kfunc_call_test_subprog.c       |  9 +---
> >  tools/testing/selftests/bpf/progs/map_kptr.c  |  6 +--
> >  .../selftests/bpf/progs/map_kptr_fail.c       |  5 +--
> >  10 files changed, 51 insertions(+), 50 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> >
> > diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> > new file mode 100644
> > index 000000000000..86d94257716a
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _BPF_TESTMOD_KFUNC_H
> > +#define _BPF_TESTMOD_KFUNC_H
> > +
> > +#ifndef __KERNEL__
> > +#include <vmlinux.h>
> > +#include <bpf/bpf_helpers.h>
> > +#else
> > +#define __ksym
> > +#endif
> > +
> > +extern struct prog_test_ref_kfunc *
> > +bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr) __ksym;
> > +extern struct prog_test_ref_kfunc *
> > +bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
> > +extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> > +
> > +extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
> > +extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
> > +extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> > +extern int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> > +extern void bpf_kfunc_call_int_mem_release(int *p) __ksym;
> > +extern u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused) __ksym;
> > +
> > +extern void bpf_testmod_test_mod_kfunc(int i) __ksym;
> > +
> > +extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
> > +                             __u32 c, __u64 d) __ksym;
> > +extern int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
> > +extern struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
> > +extern long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
> > +
> > +extern void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb) __ksym;
> > +extern void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
> > +extern void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
> > +extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
> > +
> > +extern void bpf_kfunc_call_test_destructive(void) __ksym;
> > +
> > +#endif /* _BPF_TESTMOD_KFUNC_H */
> > diff --git a/tools/testing/selftests/bpf/progs/cb_refs.c b/tools/testing/selftests/bpf/progs/cb_refs.c
> > index 7653df1bc787..823901c1b839 100644
> > --- a/tools/testing/selftests/bpf/progs/cb_refs.c
> > +++ b/tools/testing/selftests/bpf/progs/cb_refs.c
> > @@ -2,6 +2,7 @@
> >  #include <vmlinux.h>
> >  #include <bpf/bpf_tracing.h>
> >  #include <bpf/bpf_helpers.h>
> > +#include "bpf_testmod/bpf_testmod_kfunc.h"
>
> Feel free to ignore if you disagree, but here and elsewhere, should we
> do this:
>
> #include <bpf_testmod/bpf_testmod_kfunc.h>
>
> rather than using #include "bpf_testmod/bpf_testmod_kfunc.h". Doesn't
> matter much, but IMO it's just slightly more readable to use the <> to
> show that we're relying on -I rather than expecting
> bpf_testmod/bpf_testmod_kfunc.h to be found at a path relative to the
> progs. #include "bpf_misc.h" makes more sense because it really is
> located in the progs/ directory.

We do <> for headers that are expected to be installed in the system
(even if we cheat with -I sometimes). But in this case it's a local
header, so using "" makes more sense to me. But shouldn't it be
"../bpf_testmod/bpf_testmod_kfunc.h"?

>
> Either way:
>
> Acked-by: David Vernet <void@manifault.com>
>
> >

[...]

Re: [PATCHv3 bpf-next 1/9] selftests/bpf: Move kfunc exports to bpf_testmod/bpf_testmod_kfunc.h

[Jiri Olsa]

On Wed, Feb 08, 2023 at 04:20:13PM -0800, Andrii Nakryiko wrote:

SNIP

> > > diff --git a/tools/testing/selftests/bpf/progs/cb_refs.c b/tools/testing/selftests/bpf/progs/cb_refs.c
> > > index 7653df1bc787..823901c1b839 100644
> > > --- a/tools/testing/selftests/bpf/progs/cb_refs.c
> > > +++ b/tools/testing/selftests/bpf/progs/cb_refs.c
> > > @@ -2,6 +2,7 @@
> > >  #include <vmlinux.h>
> > >  #include <bpf/bpf_tracing.h>
> > >  #include <bpf/bpf_helpers.h>
> > > +#include "bpf_testmod/bpf_testmod_kfunc.h"
> >
> > Feel free to ignore if you disagree, but here and elsewhere, should we
> > do this:
> >
> > #include <bpf_testmod/bpf_testmod_kfunc.h>
> >
> > rather than using #include "bpf_testmod/bpf_testmod_kfunc.h". Doesn't
> > matter much, but IMO it's just slightly more readable to use the <> to
> > show that we're relying on -I rather than expecting
> > bpf_testmod/bpf_testmod_kfunc.h to be found at a path relative to the
> > progs. #include "bpf_misc.h" makes more sense because it really is
> > located in the progs/ directory.
> 
> We do <> for headers that are expected to be installed in the system
> (even if we cheat with -I sometimes). But in this case it's a local
> header, so using "" makes more sense to me. But shouldn't it be
> "../bpf_testmod/bpf_testmod_kfunc.h"?

I think we have -I<..selftests/bpf> so it works.. but right, we want
to show it's local header, so "../bpf_testmod/bpf_testmod_kfunc.h"
makes sense to me

jirka

Re: [PATCHv3 bpf-next 0/9] bpf: Move kernel test kfuncs into bpf_testmod

[Jiri Olsa]

On Mon, Feb 06, 2023 at 10:15:37AM +0100, Jiri Olsa wrote:
> On Sun, Feb 05, 2023 at 07:36:14PM +0100, Ilya Leoshkevich wrote:
> > On Sun, 2023-02-05 at 19:17 +0100, Jiri Olsa wrote:
> > > On Sat, Feb 04, 2023 at 01:21:13AM -0800, Alexei Starovoitov wrote:
> > > > On Fri, Feb 3, 2023 at 8:23 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > > 
> > > > > hi,
> > > > > I noticed several times in discussions that we should move test
> > > > > kfuncs
> > > > > into kernel module, now perhaps even more pressing with all the
> > > > > kfunc
> > > > > effort. This patchset moves all the test kfuncs into bpf_testmod.
> > > > > 
> > > > > I added bpf_testmod/bpf_testmod_kfunc.h header that is shared
> > > > > between
> > > > > bpf_testmod kernel module and BPF programs, which brings some
> > > > > difficulties
> > > > > with __ksym define. But I'm not sure having separate headers for
> > > > > BPF
> > > > > programs and for kernel module would be better.
> > > > > 
> > > > > This patchset also needs:
> > > > >   74bc3a5acc82 bpf: Add missing btf_put to
> > > > > register_btf_id_dtor_kfuncs
> > > > > which is only in bpf/master now.
> > > > 
> > > > I thought you've added this patch to CI,
> > > > but cb_refs is still failing on s390...
> > > 
> > > the CI now fails for s390 with messages like:
> > >    2023-02-04T07:04:32.5185267Z    RES: address of kernel function
> > > bpf_kfunc_call_test_fail1 is out of range
> > > 
> > > so now that we have test kfuncs in the module, the 's32 imm' value of
> > > the bpf call instructions can overflow when the offset between module
> > > and kernel is greater than 2GB ... as explained in the commit that
> > > added the verifier check:
> > > 
> > >   8cbf062a250e bpf: Reject kfunc calls that overflow insn->imm
> > > 
> > > not sure we can do anything about that on bpf side.. cc-ing s390 list
> > > and Ilya for ideas/thoughts
> > > 
> > > maybe we could make bpf_testmod in-tree module and compile it as
> > > module
> > > just for some archs
> > > 
> > > thoughts?
> > 
> > Hi,
> > 
> > I'd rather have this fixed - I guess the problem can affect the users.
> > The ksyms_module test is already denylisted because of that.
> > Unfortunately getting the kernel and the modules close together on
> > s390x is unlikely to happen in the foreseeable future.
> > 
> > What do you think about keeping the BTF ID inside the insn->imm field
> > and putting the 64-bit delta into bpf_insn_aux_data, replacing the
> > call_imm field that we already have there?
> 
> seems tricky wrt other archs.. how about saving address of the kfunc
> in bpf_insn_aux_data and use that in s390 jit code instead of the
> '__bpf_call_base + imm' calculation

any other ideas/thoughts on this?

I don't have s390 server available, so can't really fix/test that..
any chance you work on that?

thanks,
jirka

Re: [PATCHv3 bpf-next 0/9] bpf: Move kernel test kfuncs into bpf_testmod

[Ilya Leoshkevich]

On Thu, 2023-02-09 at 09:47 +0100, Jiri Olsa wrote:
> On Mon, Feb 06, 2023 at 10:15:37AM +0100, Jiri Olsa wrote:
> > On Sun, Feb 05, 2023 at 07:36:14PM +0100, Ilya Leoshkevich wrote:
> > > On Sun, 2023-02-05 at 19:17 +0100, Jiri Olsa wrote:
> > > > On Sat, Feb 04, 2023 at 01:21:13AM -0800, Alexei Starovoitov
> > > > wrote:
> > > > > On Fri, Feb 3, 2023 at 8:23 AM Jiri Olsa <jolsa@kernel.org>
> > > > > wrote:
> > > > > > 
> > > > > > hi,
> > > > > > I noticed several times in discussions that we should move
> > > > > > test
> > > > > > kfuncs
> > > > > > into kernel module, now perhaps even more pressing with all
> > > > > > the
> > > > > > kfunc
> > > > > > effort. This patchset moves all the test kfuncs into
> > > > > > bpf_testmod.
> > > > > > 
> > > > > > I added bpf_testmod/bpf_testmod_kfunc.h header that is
> > > > > > shared
> > > > > > between
> > > > > > bpf_testmod kernel module and BPF programs, which brings
> > > > > > some
> > > > > > difficulties
> > > > > > with __ksym define. But I'm not sure having separate
> > > > > > headers for
> > > > > > BPF
> > > > > > programs and for kernel module would be better.
> > > > > > 
> > > > > > This patchset also needs:
> > > > > >   74bc3a5acc82 bpf: Add missing btf_put to
> > > > > > register_btf_id_dtor_kfuncs
> > > > > > which is only in bpf/master now.
> > > > > 
> > > > > I thought you've added this patch to CI,
> > > > > but cb_refs is still failing on s390...
> > > > 
> > > > the CI now fails for s390 with messages like:
> > > >    2023-02-04T07:04:32.5185267Z    RES: address of kernel
> > > > function
> > > > bpf_kfunc_call_test_fail1 is out of range
> > > > 
> > > > so now that we have test kfuncs in the module, the 's32 imm'
> > > > value of
> > > > the bpf call instructions can overflow when the offset between
> > > > module
> > > > and kernel is greater than 2GB ... as explained in the commit
> > > > that
> > > > added the verifier check:
> > > > 
> > > >   8cbf062a250e bpf: Reject kfunc calls that overflow insn->imm
> > > > 
> > > > not sure we can do anything about that on bpf side.. cc-ing
> > > > s390 list
> > > > and Ilya for ideas/thoughts
> > > > 
> > > > maybe we could make bpf_testmod in-tree module and compile it
> > > > as
> > > > module
> > > > just for some archs
> > > > 
> > > > thoughts?
> > > 
> > > Hi,
> > > 
> > > I'd rather have this fixed - I guess the problem can affect the
> > > users.
> > > The ksyms_module test is already denylisted because of that.
> > > Unfortunately getting the kernel and the modules close together
> > > on
> > > s390x is unlikely to happen in the foreseeable future.
> > > 
> > > What do you think about keeping the BTF ID inside the insn->imm
> > > field
> > > and putting the 64-bit delta into bpf_insn_aux_data, replacing
> > > the
> > > call_imm field that we already have there?
> > 
> > seems tricky wrt other archs.. how about saving address of the
> > kfunc
> > in bpf_insn_aux_data and use that in s390 jit code instead of the
> > '__bpf_call_base + imm' calculation
> 
> any other ideas/thoughts on this?
> 
> I don't have s390 server available, so can't really fix/test that..
> any chance you work on that?

Hi Jiri,

sure, I'll give this a try.

Best regards,
Ilya

> 
> thanks,
> jirka