[PATCH bpf-next 00/24] Support bpf trampoline for s390x

[PATCH bpf-next 00/24] Support bpf trampoline for s390x

[Ilya Leoshkevich]

Hi,

This series implements poke, trampoline, kfunc, mixing subprogs and
tailcalls, and fixes a number of tests on s390x.

The following failures still remain:

#52      core_read_macros:FAIL
Uses BPF_PROBE_READ(), shouldn't there be BPF_PROBE_READ_KERNEL()?

#82      get_stack_raw_tp:FAIL
get_stack_print_output:FAIL:user_stack corrupted user stack
Known issue:
We cannot reliably unwind userspace on s390x without DWARF.

#101     ksyms_module:FAIL
address of kernel function bpf_testmod_test_mod_kfunc is out of range
Known issue:
Kernel and modules are too far away from each other on s390x.

#167     sk_assign:FAIL
Uses legacy map definitions in 'maps' section.

#190     stacktrace_build_id:FAIL
Known issue:
We cannot reliably unwind userspace on s390x without DWARF.

#211     test_bpffs:FAIL
iterators.bpf.c is broken on s390x, it uses BPF_CORE_READ(), shouldn't
there be BPF_CORE_READ_KERNEL()?

#218     test_profiler:FAIL
A lot of BPF_PROBE_READ() usages.

#281     xdp_metadata:FAIL
See patch 24.

#284     xdp_synproxy:FAIL
Verifier error:
; value = bpf_tcp_raw_gen_syncookie_ipv4(hdr->ipv4, hdr->tcp,
281: (79) r1 = *(u64 *)(r10 -80)      ; R1_w=pkt(off=14,r=74,imm=0) R10=fp0
282: (bf) r2 = r8                     ; R2_w=pkt(id=5,off=14,r=74,umax=60,var_off=(0x0; 0x3c)) R8=pkt(id=5,off=14,r=74,umax=60,var_off=(0x0; 0x3c))
283: (79) r3 = *(u64 *)(r10 -104)     ; R3_w=scalar(umax=60,var_off=(0x0; 0x3c)) R10=fp0
284: (85) call bpf_tcp_raw_gen_syncookie_ipv4#204
invalid access to packet, off=14 size=0, R2(id=5,off=14,r=74)
R2 offset is outside of the packet

None of these seem to be due to the new changes.

Best regards,
Ilya

Ilya Leoshkevich (24):
  selftests/bpf: Fix liburandom_read.so linker error
  selftests/bpf: Fix symlink creation error
  selftests/bpf: Fix fexit_stress on s390x
  selftests/bpf: Fix trampoline_count on s390x
  selftests/bpf: Fix kfree_skb on s390x
  selftests/bpf: Set errno when urand_spawn() fails
  selftests/bpf: Fix decap_sanity_ns cleanup
  selftests/bpf: Fix verify_pkcs7_sig on s390x
  selftests/bpf: Fix xdp_do_redirect on s390x
  selftests/bpf: Fix cgrp_local_storage on s390x
  selftests/bpf: Check stack_mprotect() return value
  selftests/bpf: Increase SIZEOF_BPF_LOCAL_STORAGE_ELEM on s390x
  selftests/bpf: Add a sign-extension test for kfuncs
  selftests/bpf: Fix test_lsm on s390x
  selftests/bpf: Fix test_xdp_adjust_tail_grow2 on s390x
  selftests/bpf: Fix vmlinux test on s390x
  libbpf: Read usdt arg spec with bpf_probe_read_kernel()
  s390/bpf: Fix a typo in a comment
  s390/bpf: Add expoline to tail calls
  s390/bpf: Implement bpf_arch_text_poke()
  bpf: btf: Add BTF_FMODEL_SIGNED_ARG flag
  s390/bpf: Implement arch_prepare_bpf_trampoline()
  s390/bpf: Implement bpf_jit_supports_subprog_tailcalls()
  s390/bpf: Implement bpf_jit_supports_kfunc_call()

 arch/s390/net/bpf_jit_comp.c                  | 708 +++++++++++++++++-
 include/linux/bpf.h                           |   8 +
 include/linux/btf.h                           |  15 +-
 kernel/bpf/btf.c                              |  16 +-
 net/bpf/test_run.c                            |   9 +
 tools/lib/bpf/usdt.bpf.h                      |  33 +-
 tools/testing/selftests/bpf/Makefile          |   7 +-
 tools/testing/selftests/bpf/netcnt_common.h   |   6 +-
 .../selftests/bpf/prog_tests/bpf_cookie.c     |   6 +-
 .../bpf/prog_tests/cgrp_local_storage.c       |   2 +-
 .../selftests/bpf/prog_tests/decap_sanity.c   |   2 +-
 .../selftests/bpf/prog_tests/fexit_stress.c   |   6 +-
 .../selftests/bpf/prog_tests/kfree_skb.c      |   2 +-
 .../selftests/bpf/prog_tests/kfunc_call.c     |   1 +
 .../selftests/bpf/prog_tests/test_lsm.c       |   3 +-
 .../bpf/prog_tests/trampoline_count.c         |   4 +
 tools/testing/selftests/bpf/prog_tests/usdt.c |   1 +
 .../bpf/prog_tests/verify_pkcs7_sig.c         |   9 +
 .../bpf/prog_tests/xdp_adjust_tail.c          |   7 +-
 .../bpf/prog_tests/xdp_do_redirect.c          |   4 +
 .../selftests/bpf/progs/kfunc_call_test.c     |  18 +
 tools/testing/selftests/bpf/progs/lsm.c       |   7 +-
 .../bpf/progs/test_verify_pkcs7_sig.c         |  12 +-
 .../selftests/bpf/progs/test_vmlinux.c        |   4 +-
 .../bpf/progs/test_xdp_adjust_tail_grow.c     |   8 +-
 25 files changed, 816 insertions(+), 82 deletions(-)

-- 
2.39.1

[PATCH bpf-next 01/24] selftests/bpf: Fix liburandom_read.so linker error

[Ilya Leoshkevich]

When building with O=, the following linker error occurs:

    clang: error: no such file or directory: 'liburandom_read.so'

Fix by adding $(OUTPUT) to the linker search path.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index c9b5ed59e1ed..43098eb15d31 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -189,9 +189,9 @@ $(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c
 $(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_read.so
 	$(call msg,BINARY,,$@)
 	$(Q)$(CLANG) $(filter-out -static,$(CFLAGS) $(LDFLAGS)) $(filter %.c,$^) \
-		     liburandom_read.so $(filter-out -static,$(LDLIBS))	     \
+		     $(filter-out -static,$(LDLIBS))	                       \
 		     -fuse-ld=$(LLD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
-		     -Wl,-rpath=. -o $@
+		     -Wl,-rpath=. -o $@ -lurandom_read -L$(OUTPUT)
 
 $(OUTPUT)/sign-file: ../../../../scripts/sign-file.c
 	$(call msg,SIGN-FILE,,$@)
-- 
2.39.1

Re: [PATCH bpf-next 01/24] selftests/bpf: Fix liburandom_read.so linker error

[Andrii Nakryiko]

On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> When building with O=, the following linker error occurs:
>
>     clang: error: no such file or directory: 'liburandom_read.so'
>
> Fix by adding $(OUTPUT) to the linker search path.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/testing/selftests/bpf/Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index c9b5ed59e1ed..43098eb15d31 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -189,9 +189,9 @@ $(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c
>  $(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_read.so
>         $(call msg,BINARY,,$@)
>         $(Q)$(CLANG) $(filter-out -static,$(CFLAGS) $(LDFLAGS)) $(filter %.c,$^) \
> -                    liburandom_read.so $(filter-out -static,$(LDLIBS))      \
> +                    $(filter-out -static,$(LDLIBS))                           \
>                      -fuse-ld=$(LLD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
> -                    -Wl,-rpath=. -o $@
> +                    -Wl,-rpath=. -o $@ -lurandom_read -L$(OUTPUT)

why moving to the end? it's nice in verbose logs when the last thing
is the resulting file ($@), so if possible, let's move it back?


>
>  $(OUTPUT)/sign-file: ../../../../scripts/sign-file.c
>         $(call msg,SIGN-FILE,,$@)
> --
> 2.39.1
>

Re: [PATCH bpf-next 01/24] selftests/bpf: Fix liburandom_read.so linker error

[Ilya Leoshkevich]

On Wed, 2023-01-25 at 17:07 -0800, Andrii Nakryiko wrote:
> On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > When building with O=, the following linker error occurs:
> > 
> >     clang: error: no such file or directory: 'liburandom_read.so'
> > 
> > Fix by adding $(OUTPUT) to the linker search path.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  tools/testing/selftests/bpf/Makefile | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/Makefile
> > b/tools/testing/selftests/bpf/Makefile
> > index c9b5ed59e1ed..43098eb15d31 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -189,9 +189,9 @@ $(OUTPUT)/liburandom_read.so:
> > urandom_read_lib1.c urandom_read_lib2.c
> >  $(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c
> > $(OUTPUT)/liburandom_read.so
> >         $(call msg,BINARY,,$@)
> >         $(Q)$(CLANG) $(filter-out -static,$(CFLAGS) $(LDFLAGS))
> > $(filter %.c,$^) \
> > -                    liburandom_read.so $(filter-out -
> > static,$(LDLIBS))      \
> > +                    $(filter-out -
> > static,$(LDLIBS))                           \
> >                      -fuse-ld=$(LLD) -Wl,-znoseparate-code -Wl,--
> > build-id=sha1 \
> > -                    -Wl,-rpath=. -o $@
> > +                    -Wl,-rpath=. -o $@ -lurandom_read -L$(OUTPUT)
> 
> why moving to the end? it's nice in verbose logs when the last thing
> is the resulting file ($@), so if possible, let's move it back?

You're right, I'm just used to having the libraries at the end, but
here we already have $(LDLIBS) in the middle. Will do in v2.

> 
> > 
> >  $(OUTPUT)/sign-file: ../../../../scripts/sign-file.c
> >         $(call msg,SIGN-FILE,,$@)
> > --
> > 2.39.1
> > 

[PATCH bpf-next 02/24] selftests/bpf: Fix symlink creation error

[Ilya Leoshkevich]

When building with O=, the following error occurs:

    ln: failed to create symbolic link 'no_alu32/bpftool': No such file or directory

Adjust the code to account for $(OUTPUT).

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 43098eb15d31..b6c31817755e 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -519,7 +519,8 @@ $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)			\
 	$$(call msg,BINARY,,$$@)
 	$(Q)$$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) -o $$@
 	$(Q)$(RESOLVE_BTFIDS) --btf $(TRUNNER_OUTPUT)/btf_data.bpf.o $$@
-	$(Q)ln -sf $(if $2,..,.)/tools/build/bpftool/bootstrap/bpftool $(if $2,$2/)bpftool
+	$(Q)ln -sf $(if $2,..,.)/tools/build/bpftool/bootstrap/bpftool \
+		   $(OUTPUT)/$(if $2,$2/)bpftool
 
 endef
 
-- 
2.39.1

[PATCH bpf-next 03/24] selftests/bpf: Fix fexit_stress on s390x

[Ilya Leoshkevich]

BPF_MAX_TRAMP_LINKS is smaller on s390x than on x86.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/prog_tests/fexit_stress.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_stress.c b/tools/testing/selftests/bpf/prog_tests/fexit_stress.c
index 5a7e6011f6bf..329950f572cc 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_stress.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_stress.c
@@ -2,8 +2,12 @@
 /* Copyright (c) 2019 Facebook */
 #include <test_progs.h>
 
-/* that's kernel internal BPF_MAX_TRAMP_PROGS define */
+/* that's kernel internal BPF_MAX_TRAMP_LINKS define */
+#if defined(__s390x__)
+#define CNT 27
+#else
 #define CNT 38
+#endif
 
 void serial_test_fexit_stress(void)
 {
-- 
2.39.1

[PATCH bpf-next 04/24] selftests/bpf: Fix trampoline_count on s390x

[Ilya Leoshkevich]

MAX_TRAMP_PROGS on s390x is smaller than on x86.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/prog_tests/trampoline_count.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
index 564b75bc087f..3d2e25492f40 100644
--- a/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
+++ b/tools/testing/selftests/bpf/prog_tests/trampoline_count.c
@@ -2,7 +2,11 @@
 #define _GNU_SOURCE
 #include <test_progs.h>
 
+#if defined(__s390x__)
+#define MAX_TRAMP_PROGS 27
+#else
 #define MAX_TRAMP_PROGS 38
+#endif
 
 struct inst {
 	struct bpf_object *obj;
-- 
2.39.1

[PATCH bpf-next 05/24] selftests/bpf: Fix kfree_skb on s390x

[Ilya Leoshkevich]

h_proto is big-endian; use htons() in order to make comparison work on
both little- and big-endian machines.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/prog_tests/kfree_skb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/kfree_skb.c b/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
index 73579370bfbd..c07991544a78 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfree_skb.c
@@ -36,7 +36,7 @@ static void on_sample(void *ctx, int cpu, void *data, __u32 size)
 		  "cb32_0 %x != %x\n",
 		  meta->cb32_0, cb.cb32[0]))
 		return;
-	if (CHECK(pkt_v6->eth.h_proto != 0xdd86, "check_eth",
+	if (CHECK(pkt_v6->eth.h_proto != htons(ETH_P_IPV6), "check_eth",
 		  "h_proto %x\n", pkt_v6->eth.h_proto))
 		return;
 	if (CHECK(pkt_v6->iph.nexthdr != 6, "check_ip",
-- 
2.39.1

[PATCH bpf-next 06/24] selftests/bpf: Set errno when urand_spawn() fails

[Ilya Leoshkevich]

The result of urand_spawn() is checked with ASSERT_OK_PTR, which treats
NULL as success if errno == 0.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/prog_tests/usdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/usdt.c b/tools/testing/selftests/bpf/prog_tests/usdt.c
index 9ad9da0f215e..56ed1eb9b527 100644
--- a/tools/testing/selftests/bpf/prog_tests/usdt.c
+++ b/tools/testing/selftests/bpf/prog_tests/usdt.c
@@ -314,6 +314,7 @@ static FILE *urand_spawn(int *pid)
 
 	if (fscanf(f, "%d", pid) != 1) {
 		pclose(f);
+		errno = EINVAL;
 		return NULL;
 	}
 
-- 
2.39.1

[PATCH bpf-next 07/24] selftests/bpf: Fix decap_sanity_ns cleanup

[Ilya Leoshkevich]

decap_sanity prints the following on the 1st run:

    decap_sanity: sh: 1: Syntax error: Bad fd number

and the following on the 2nd run:

    Cannot create namespace file "/run/netns/decap_sanity_ns": File exists

The problem is that the cleanup command has a typo and does nothing.
Fix the typo.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/prog_tests/decap_sanity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/decap_sanity.c b/tools/testing/selftests/bpf/prog_tests/decap_sanity.c
index 0b2f73b88c53..2853883b7cbb 100644
--- a/tools/testing/selftests/bpf/prog_tests/decap_sanity.c
+++ b/tools/testing/selftests/bpf/prog_tests/decap_sanity.c
@@ -80,6 +80,6 @@ void test_decap_sanity(void)
 		bpf_tc_hook_destroy(&qdisc_hook);
 		close_netns(nstoken);
 	}
-	system("ip netns del " NS_TEST " >& /dev/null");
+	system("ip netns del " NS_TEST " &> /dev/null");
 	decap_sanity__destroy(skel);
 }
-- 
2.39.1

[PATCH bpf-next 08/24] selftests/bpf: Fix verify_pkcs7_sig on s390x

[Ilya Leoshkevich]

Use bpf_probe_read_kernel() instead of bpf_probe_read(), which is not
defined on all architectures.

While at it, improve the error handling: do not hide the verifier log,
and check the return values of bpf_probe_read_kernel() and
bpf_copy_from_user().

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 .../selftests/bpf/prog_tests/verify_pkcs7_sig.c      |  9 +++++++++
 .../selftests/bpf/progs/test_verify_pkcs7_sig.c      | 12 ++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
index 579d6ee83ce0..75c256f79f85 100644
--- a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
+++ b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
@@ -56,11 +56,17 @@ struct data {
 	__u32 sig_len;
 };
 
+static char libbpf_log[8192];
 static bool kfunc_not_supported;
 
 static int libbpf_print_cb(enum libbpf_print_level level, const char *fmt,
 			   va_list args)
 {
+	size_t log_len = strlen(libbpf_log);
+
+	vsnprintf(libbpf_log + log_len, sizeof(libbpf_log) - log_len,
+		  fmt, args);
+
 	if (strcmp(fmt, "libbpf: extern (func ksym) '%s': not found in kernel or module BTFs\n"))
 		return 0;
 
@@ -277,6 +283,7 @@ void test_verify_pkcs7_sig(void)
 	if (!ASSERT_OK_PTR(skel, "test_verify_pkcs7_sig__open"))
 		goto close_prog;
 
+	libbpf_log[0] = 0;
 	old_print_cb = libbpf_set_print(libbpf_print_cb);
 	ret = test_verify_pkcs7_sig__load(skel);
 	libbpf_set_print(old_print_cb);
@@ -289,6 +296,8 @@ void test_verify_pkcs7_sig(void)
 		goto close_prog;
 	}
 
+	printf("%s", libbpf_log);
+
 	if (!ASSERT_OK(ret, "test_verify_pkcs7_sig__load"))
 		goto close_prog;
 
diff --git a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
index ce419304ff1f..7748cc23de8a 100644
--- a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
+++ b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
@@ -59,10 +59,14 @@ int BPF_PROG(bpf, int cmd, union bpf_attr *attr, unsigned int size)
 	if (!data_val)
 		return 0;
 
-	bpf_probe_read(&value, sizeof(value), &attr->value);
-
-	bpf_copy_from_user(data_val, sizeof(struct data),
-			   (void *)(unsigned long)value);
+	ret = bpf_probe_read_kernel(&value, sizeof(value), &attr->value);
+	if (ret)
+		return ret;
+
+	ret = bpf_copy_from_user(data_val, sizeof(struct data),
+				 (void *)(unsigned long)value);
+	if (ret)
+		return ret;
 
 	if (data_val->data_len > sizeof(data_val->data))
 		return -EINVAL;
-- 
2.39.1

Re: [PATCH bpf-next 08/24] selftests/bpf: Fix verify_pkcs7_sig on s390x

[Andrii Nakryiko]

On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Use bpf_probe_read_kernel() instead of bpf_probe_read(), which is not
> defined on all architectures.
>
> While at it, improve the error handling: do not hide the verifier log,
> and check the return values of bpf_probe_read_kernel() and
> bpf_copy_from_user().
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  .../selftests/bpf/prog_tests/verify_pkcs7_sig.c      |  9 +++++++++
>  .../selftests/bpf/progs/test_verify_pkcs7_sig.c      | 12 ++++++++----
>  2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> index 579d6ee83ce0..75c256f79f85 100644
> --- a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> +++ b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> @@ -56,11 +56,17 @@ struct data {
>         __u32 sig_len;
>  };
>
> +static char libbpf_log[8192];
>  static bool kfunc_not_supported;
>
>  static int libbpf_print_cb(enum libbpf_print_level level, const char *fmt,
>                            va_list args)
>  {
> +       size_t log_len = strlen(libbpf_log);
> +
> +       vsnprintf(libbpf_log + log_len, sizeof(libbpf_log) - log_len,
> +                 fmt, args);

it seems like test is written to assume that load might fail and we'll
get error messages, so not sure it's that useful to print out these
errors. But at the very least we should filter out DEBUG and INFO
level messages, and pass through WARN only.

Also, there is no point in having a separate log buffer, just printf
directly. test_progs will take care to collect overall log and ignore
it if test succeeds, or emit it if test fails


> +
>         if (strcmp(fmt, "libbpf: extern (func ksym) '%s': not found in kernel or module BTFs\n"))
>                 return 0;
>
> @@ -277,6 +283,7 @@ void test_verify_pkcs7_sig(void)
>         if (!ASSERT_OK_PTR(skel, "test_verify_pkcs7_sig__open"))
>                 goto close_prog;
>
> +       libbpf_log[0] = 0;
>         old_print_cb = libbpf_set_print(libbpf_print_cb);
>         ret = test_verify_pkcs7_sig__load(skel);
>         libbpf_set_print(old_print_cb);
> @@ -289,6 +296,8 @@ void test_verify_pkcs7_sig(void)
>                 goto close_prog;
>         }
>
> +       printf("%s", libbpf_log);
> +
>         if (!ASSERT_OK(ret, "test_verify_pkcs7_sig__load"))
>                 goto close_prog;
>
> diff --git a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> index ce419304ff1f..7748cc23de8a 100644
> --- a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> +++ b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> @@ -59,10 +59,14 @@ int BPF_PROG(bpf, int cmd, union bpf_attr *attr, unsigned int size)
>         if (!data_val)
>                 return 0;
>
> -       bpf_probe_read(&value, sizeof(value), &attr->value);
> -
> -       bpf_copy_from_user(data_val, sizeof(struct data),
> -                          (void *)(unsigned long)value);
> +       ret = bpf_probe_read_kernel(&value, sizeof(value), &attr->value);
> +       if (ret)
> +               return ret;
> +
> +       ret = bpf_copy_from_user(data_val, sizeof(struct data),
> +                                (void *)(unsigned long)value);
> +       if (ret)
> +               return ret;

this part looks good, we shouldn't use bpf_probe_read.

You'll have to update progs/profiler.inc.h as well, btw, which still
uses bpf_probe_read() and bpf_probe_read_str.
>
>         if (data_val->data_len > sizeof(data_val->data))
>                 return -EINVAL;
> --
> 2.39.1
>

Re: [PATCH bpf-next 08/24] selftests/bpf: Fix verify_pkcs7_sig on s390x

[Ilya Leoshkevich]

On Wed, 2023-01-25 at 17:06 -0800, Andrii Nakryiko wrote:
> On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > Use bpf_probe_read_kernel() instead of bpf_probe_read(), which is
> > not
> > defined on all architectures.
> > 
> > While at it, improve the error handling: do not hide the verifier
> > log,
> > and check the return values of bpf_probe_read_kernel() and
> > bpf_copy_from_user().
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  .../selftests/bpf/prog_tests/verify_pkcs7_sig.c      |  9
> > +++++++++
> >  .../selftests/bpf/progs/test_verify_pkcs7_sig.c      | 12
> > ++++++++----
> >  2 files changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git
> > a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> > b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> > index 579d6ee83ce0..75c256f79f85 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> > @@ -56,11 +56,17 @@ struct data {
> >         __u32 sig_len;
> >  };
> > 
> > +static char libbpf_log[8192];
> >  static bool kfunc_not_supported;
> > 
> >  static int libbpf_print_cb(enum libbpf_print_level level, const
> > char *fmt,
> >                            va_list args)
> >  {
> > +       size_t log_len = strlen(libbpf_log);
> > +
> > +       vsnprintf(libbpf_log + log_len, sizeof(libbpf_log) -
> > log_len,
> > +                 fmt, args);
> 
> it seems like test is written to assume that load might fail and
> we'll
> get error messages, so not sure it's that useful to print out these
> errors. But at the very least we should filter out DEBUG and INFO
> level messages, and pass through WARN only.
> 
> Also, there is no point in having a separate log buffer, just printf
> directly. test_progs will take care to collect overall log and ignore
> it if test succeeds, or emit it if test fails

Thanks, I completely overlooked the fact that the test framework
already hides the output in case of success. With that in mind I can do
just this:

--- a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
+++ b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
@@ -61,6 +61,9 @@ static bool kfunc_not_supported;
 static int libbpf_print_cb(enum libbpf_print_level level, const char
*fmt,
                           va_list args)
 {
+       if (level == LIBBPF_WARN)
+               vprintf(fmt, args);
+
        if (strcmp(fmt, "libbpf: extern (func ksym) '%s': not found in
kernel or module BTFs\n"))
                return 0;
 
If the load fails due to missing kfuncs, we'll skip the test - I think
in this case the output won't be printed either, so we should be fine.

> > +
> >         if (strcmp(fmt, "libbpf: extern (func ksym) '%s': not found
> > in kernel or module BTFs\n"))
> >                 return 0;
> > 
> > @@ -277,6 +283,7 @@ void test_verify_pkcs7_sig(void)
> >         if (!ASSERT_OK_PTR(skel, "test_verify_pkcs7_sig__open"))
> >                 goto close_prog;
> > 
> > +       libbpf_log[0] = 0;
> >         old_print_cb = libbpf_set_print(libbpf_print_cb);
> >         ret = test_verify_pkcs7_sig__load(skel);
> >         libbpf_set_print(old_print_cb);
> > @@ -289,6 +296,8 @@ void test_verify_pkcs7_sig(void)
> >                 goto close_prog;
> >         }
> > 
> > +       printf("%s", libbpf_log);
> > +
> >         if (!ASSERT_OK(ret, "test_verify_pkcs7_sig__load"))
> >                 goto close_prog;
> > 
> > diff --git
> > a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> > b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> > index ce419304ff1f..7748cc23de8a 100644
> > --- a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> > +++ b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> > @@ -59,10 +59,14 @@ int BPF_PROG(bpf, int cmd, union bpf_attr
> > *attr, unsigned int size)
> >         if (!data_val)
> >                 return 0;
> > 
> > -       bpf_probe_read(&value, sizeof(value), &attr->value);
> > -
> > -       bpf_copy_from_user(data_val, sizeof(struct data),
> > -                          (void *)(unsigned long)value);
> > +       ret = bpf_probe_read_kernel(&value, sizeof(value), &attr-
> > >value);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = bpf_copy_from_user(data_val, sizeof(struct data),
> > +                                (void *)(unsigned long)value);
> > +       if (ret)
> > +               return ret;
> 
> this part looks good, we shouldn't use bpf_probe_read.
> 
> You'll have to update progs/profiler.inc.h as well, btw, which still
> uses bpf_probe_read() and bpf_probe_read_str.

I remember trying this, but there were still failures due to, as I
thought back then, usage of BPF_CORE_READ() and the lack of
BPF_CORE_READ_KERNEL(). But this seems to be a generic issue. Let me
try again and post my findings as a reply to 0/24.

> >         if (data_val->data_len > sizeof(data_val->data))
> >                 return -EINVAL;
> > --
> > 2.39.1
> > 

Re: [PATCH bpf-next 08/24] selftests/bpf: Fix verify_pkcs7_sig on s390x

[Andrii Nakryiko]

On Fri, Jan 27, 2023 at 4:36 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Wed, 2023-01-25 at 17:06 -0800, Andrii Nakryiko wrote:
> > On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com>
> > wrote:
> > >
> > > Use bpf_probe_read_kernel() instead of bpf_probe_read(), which is
> > > not
> > > defined on all architectures.
> > >
> > > While at it, improve the error handling: do not hide the verifier
> > > log,
> > > and check the return values of bpf_probe_read_kernel() and
> > > bpf_copy_from_user().
> > >
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >  .../selftests/bpf/prog_tests/verify_pkcs7_sig.c      |  9
> > > +++++++++
> > >  .../selftests/bpf/progs/test_verify_pkcs7_sig.c      | 12
> > > ++++++++----
> > >  2 files changed, 17 insertions(+), 4 deletions(-)
> > >
> > > diff --git
> > > a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> > > b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> > > index 579d6ee83ce0..75c256f79f85 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> > > @@ -56,11 +56,17 @@ struct data {
> > >         __u32 sig_len;
> > >  };
> > >
> > > +static char libbpf_log[8192];
> > >  static bool kfunc_not_supported;
> > >
> > >  static int libbpf_print_cb(enum libbpf_print_level level, const
> > > char *fmt,
> > >                            va_list args)
> > >  {
> > > +       size_t log_len = strlen(libbpf_log);
> > > +
> > > +       vsnprintf(libbpf_log + log_len, sizeof(libbpf_log) -
> > > log_len,
> > > +                 fmt, args);
> >
> > it seems like test is written to assume that load might fail and
> > we'll
> > get error messages, so not sure it's that useful to print out these
> > errors. But at the very least we should filter out DEBUG and INFO
> > level messages, and pass through WARN only.
> >
> > Also, there is no point in having a separate log buffer, just printf
> > directly. test_progs will take care to collect overall log and ignore
> > it if test succeeds, or emit it if test fails
>
> Thanks, I completely overlooked the fact that the test framework
> already hides the output in case of success. With that in mind I can do
> just this:
>
> --- a/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> +++ b/tools/testing/selftests/bpf/prog_tests/verify_pkcs7_sig.c
> @@ -61,6 +61,9 @@ static bool kfunc_not_supported;
>  static int libbpf_print_cb(enum libbpf_print_level level, const char
> *fmt,
>                            va_list args)
>  {
> +       if (level == LIBBPF_WARN)
> +               vprintf(fmt, args);
> +
>         if (strcmp(fmt, "libbpf: extern (func ksym) '%s': not found in
> kernel or module BTFs\n"))
>                 return 0;
>
> If the load fails due to missing kfuncs, we'll skip the test - I think
> in this case the output won't be printed either, so we should be fine.

sgtm

>
> > > +
> > >         if (strcmp(fmt, "libbpf: extern (func ksym) '%s': not found
> > > in kernel or module BTFs\n"))
> > >                 return 0;
> > >
> > > @@ -277,6 +283,7 @@ void test_verify_pkcs7_sig(void)
> > >         if (!ASSERT_OK_PTR(skel, "test_verify_pkcs7_sig__open"))
> > >                 goto close_prog;
> > >
> > > +       libbpf_log[0] = 0;
> > >         old_print_cb = libbpf_set_print(libbpf_print_cb);
> > >         ret = test_verify_pkcs7_sig__load(skel);
> > >         libbpf_set_print(old_print_cb);
> > > @@ -289,6 +296,8 @@ void test_verify_pkcs7_sig(void)
> > >                 goto close_prog;
> > >         }
> > >
> > > +       printf("%s", libbpf_log);
> > > +
> > >         if (!ASSERT_OK(ret, "test_verify_pkcs7_sig__load"))
> > >                 goto close_prog;
> > >
> > > diff --git
> > > a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> > > b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> > > index ce419304ff1f..7748cc23de8a 100644
> > > --- a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> > > +++ b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> > > @@ -59,10 +59,14 @@ int BPF_PROG(bpf, int cmd, union bpf_attr
> > > *attr, unsigned int size)
> > >         if (!data_val)
> > >                 return 0;
> > >
> > > -       bpf_probe_read(&value, sizeof(value), &attr->value);
> > > -
> > > -       bpf_copy_from_user(data_val, sizeof(struct data),
> > > -                          (void *)(unsigned long)value);
> > > +       ret = bpf_probe_read_kernel(&value, sizeof(value), &attr-
> > > >value);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       ret = bpf_copy_from_user(data_val, sizeof(struct data),
> > > +                                (void *)(unsigned long)value);
> > > +       if (ret)
> > > +               return ret;
> >
> > this part looks good, we shouldn't use bpf_probe_read.
> >
> > You'll have to update progs/profiler.inc.h as well, btw, which still
> > uses bpf_probe_read() and bpf_probe_read_str.
>
> I remember trying this, but there were still failures due to, as I
> thought back then, usage of BPF_CORE_READ() and the lack of
> BPF_CORE_READ_KERNEL(). But this seems to be a generic issue. Let me
> try again and post my findings as a reply to 0/24.

Yep, replied there as well: BPF_PROBE_READ still using
bpf_probe_read() is an omission and we should fix that.

>
> > >         if (data_val->data_len > sizeof(data_val->data))
> > >                 return -EINVAL;
> > > --
> > > 2.39.1
> > >
>

[PATCH bpf-next 09/24] selftests/bpf: Fix xdp_do_redirect on s390x

[Ilya Leoshkevich]

s390x cache line size is 256 bytes, so skb_shared_info must be aligned
on a much larger boundary than for x86. This makes the maximum packet
size smaller.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
index a50971c6cf4a..ac70e871d62f 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
@@ -65,7 +65,11 @@ static int attach_tc_prog(struct bpf_tc_hook *hook, int fd)
 /* The maximum permissible size is: PAGE_SIZE - sizeof(struct xdp_page_head) -
  * sizeof(struct skb_shared_info) - XDP_PACKET_HEADROOM = 3368 bytes
  */
+#if defined(__s390x__)
+#define MAX_PKT_SIZE 3176
+#else
 #define MAX_PKT_SIZE 3368
+#endif
 static void test_max_pkt_size(int fd)
 {
 	char data[MAX_PKT_SIZE + 1] = {};
-- 
2.39.1

[PATCH bpf-next 10/24] selftests/bpf: Fix cgrp_local_storage on s390x

[Ilya Leoshkevich]

Sync the definition of socket_cookie between the eBPF program and the
test. Currently the test works by accident, since on little-endian it
is sometimes acceptable to access u64 as u32.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c b/tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c
index 33a2776737e7..2cc759956e3b 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgrp_local_storage.c
@@ -16,7 +16,7 @@
 
 struct socket_cookie {
 	__u64 cookie_key;
-	__u32 cookie_value;
+	__u64 cookie_value;
 };
 
 static void test_tp_btf(int cgroup_fd)
-- 
2.39.1

[PATCH bpf-next 11/24] selftests/bpf: Check stack_mprotect() return value

[Ilya Leoshkevich]

If stack_mprotect() succeeds, errno is not changed. This can produce
misleading error messages, that show stale errno.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/prog_tests/bpf_cookie.c | 6 ++++--
 tools/testing/selftests/bpf/prog_tests/test_lsm.c   | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
index 2be2d61954bc..26b2d1bffdfd 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
@@ -472,6 +472,7 @@ static void lsm_subtest(struct test_bpf_cookie *skel)
 	int prog_fd;
 	int lsm_fd = -1;
 	LIBBPF_OPTS(bpf_link_create_opts, link_opts);
+	int err;
 
 	skel->bss->lsm_res = 0;
 
@@ -482,8 +483,9 @@ static void lsm_subtest(struct test_bpf_cookie *skel)
 	if (!ASSERT_GE(lsm_fd, 0, "lsm.link_create"))
 		goto cleanup;
 
-	stack_mprotect();
-	if (!ASSERT_EQ(errno, EPERM, "stack_mprotect"))
+	err = stack_mprotect();
+	if (!ASSERT_EQ(err, -1, "stack_mprotect") ||
+	    !ASSERT_EQ(errno, EPERM, "stack_mprotect"))
 		goto cleanup;
 
 	usleep(1);
diff --git a/tools/testing/selftests/bpf/prog_tests/test_lsm.c b/tools/testing/selftests/bpf/prog_tests/test_lsm.c
index 244c01125126..16175d579bc7 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_lsm.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_lsm.c
@@ -75,7 +75,8 @@ static int test_lsm(struct lsm *skel)
 	skel->bss->monitored_pid = getpid();
 
 	err = stack_mprotect();
-	if (!ASSERT_EQ(errno, EPERM, "stack_mprotect"))
+	if (!ASSERT_EQ(err, -1, "stack_mprotect") ||
+	    !ASSERT_EQ(errno, EPERM, "stack_mprotect"))
 		return err;
 
 	ASSERT_EQ(skel->bss->mprotect_count, 1, "mprotect_count");
-- 
2.39.1

[PATCH bpf-next 12/24] selftests/bpf: Increase SIZEOF_BPF_LOCAL_STORAGE_ELEM on s390x

[Ilya Leoshkevich]

sizeof(struct bpf_local_storage_elem) is 512 on s390x:

    struct bpf_local_storage_elem {
            struct hlist_node          map_node;             /*     0    16 */
            struct hlist_node          snode;                /*    16    16 */
            struct bpf_local_storage * local_storage;        /*    32     8 */
            struct callback_head       rcu __attribute__((__aligned__(8))); /*    40    16 */

            /* XXX 200 bytes hole, try to pack */

            /* --- cacheline 1 boundary (256 bytes) --- */
            struct bpf_local_storage_data sdata __attribute__((__aligned__(256))); /*   256     8 */

            /* size: 512, cachelines: 2, members: 5 */
            /* sum members: 64, holes: 1, sum holes: 200 */
            /* padding: 248 */
            /* forced alignments: 2, forced holes: 1, sum forced holes: 200 */
    } __attribute__((__aligned__(256)));

As the existing comment suggests, use a larger number in order to be
future-proof.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/netcnt_common.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/netcnt_common.h b/tools/testing/selftests/bpf/netcnt_common.h
index 0ab1c88041cd..2d4a58e4e39c 100644
--- a/tools/testing/selftests/bpf/netcnt_common.h
+++ b/tools/testing/selftests/bpf/netcnt_common.h
@@ -8,11 +8,11 @@
 
 /* sizeof(struct bpf_local_storage_elem):
  *
- * It really is about 128 bytes on x86_64, but allocate more to account for
- * possible layout changes, different architectures, etc.
+ * It is about 128 bytes on x86_64 and 512 bytes on s390x, but allocate more to
+ * account for possible layout changes, different architectures, etc.
  * The kernel will wrap up to PAGE_SIZE internally anyway.
  */
-#define SIZEOF_BPF_LOCAL_STORAGE_ELEM		256
+#define SIZEOF_BPF_LOCAL_STORAGE_ELEM		768
 
 /* Try to estimate kernel's BPF_LOCAL_STORAGE_MAX_VALUE_SIZE: */
 #define BPF_LOCAL_STORAGE_MAX_VALUE_SIZE	(0xFFFF - \
-- 
2.39.1

[PATCH bpf-next 13/24] selftests/bpf: Add a sign-extension test for kfuncs

[Ilya Leoshkevich]

s390x ABI requires the caller to zero- or sign-extend the arguments.
eBPF already deals with zero-extension (by definition of its ABI), but
not with sign-extension.

Add a test to cover that potentially problematic area.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 net/bpf/test_run.c                             |  9 +++++++++
 .../selftests/bpf/prog_tests/kfunc_call.c      |  1 +
 .../selftests/bpf/progs/kfunc_call_test.c      | 18 ++++++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 8da0d73b368e..7dbefa4fd2eb 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -550,6 +550,14 @@ struct sock * noinline 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;
 };
@@ -746,6 +754,7 @@ 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)
diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
index 5af1ee8f0e6e..bb4cd82a788a 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
@@ -72,6 +72,7 @@ static struct kfunc_test_params kfunc_tests[] = {
 	/* success cases */
 	TC_TEST(kfunc_call_test1, 12),
 	TC_TEST(kfunc_call_test2, 3),
+	TC_TEST(kfunc_call_test4, -1234),
 	TC_TEST(kfunc_call_test_ref_btf_id, 0),
 	TC_TEST(kfunc_call_test_get_mem, 42),
 	SYSCALL_TEST(kfunc_syscall_test, 0),
diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
index f636e50be259..d91c58d06d38 100644
--- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
@@ -3,6 +3,7 @@
 #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;
@@ -17,6 +18,23 @@ 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;
 
+SEC("tc")
+int kfunc_call_test4(struct __sk_buff *skb)
+{
+	struct bpf_sock *sk = skb->sk;
+	long tmp;
+
+	if (!sk)
+		return -1;
+
+	sk = bpf_sk_fullsock(sk);
+	if (!sk)
+		return -1;
+
+	tmp = bpf_kfunc_call_test4(-3, -30, -200, -1000);
+	return (tmp >> 32) + tmp;
+}
+
 SEC("tc")
 int kfunc_call_test2(struct __sk_buff *skb)
 {
-- 
2.39.1

[PATCH bpf-next 14/24] selftests/bpf: Fix test_lsm on s390x

[Ilya Leoshkevich]

Use syscall macros to access the setdomainname() arguments; currently
the code uses gprs[2] instead of orig_gpr2 for the first argument.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/progs/lsm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/lsm.c b/tools/testing/selftests/bpf/progs/lsm.c
index d8d8af623bc2..dc93887ed34c 100644
--- a/tools/testing/selftests/bpf/progs/lsm.c
+++ b/tools/testing/selftests/bpf/progs/lsm.c
@@ -6,9 +6,10 @@
 
 #include "bpf_misc.h"
 #include "vmlinux.h"
+#include <bpf/bpf_core_read.h>
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
-#include  <errno.h>
+#include <errno.h>
 
 struct {
 	__uint(type, BPF_MAP_TYPE_ARRAY);
@@ -164,8 +165,8 @@ int copy_test = 0;
 SEC("fentry.s/" SYS_PREFIX "sys_setdomainname")
 int BPF_PROG(test_sys_setdomainname, struct pt_regs *regs)
 {
-	void *ptr = (void *)PT_REGS_PARM1(regs);
-	int len = PT_REGS_PARM2(regs);
+	void *ptr = (void *)PT_REGS_PARM1_SYSCALL(regs);
+	int len = PT_REGS_PARM2_SYSCALL(regs);
 	int buf = 0;
 	long ret;
 
-- 
2.39.1

[PATCH bpf-next 15/24] selftests/bpf: Fix test_xdp_adjust_tail_grow2 on s390x

[Ilya Leoshkevich]

s390x cache line size is 256 bytes, so skb_shared_info must be aligned
on a much larger boundary than for x86.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c  | 7 ++++++-
 .../selftests/bpf/progs/test_xdp_adjust_tail_grow.c       | 8 +++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
index 39973ea1ce43..f09505f8b038 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c
@@ -76,10 +76,15 @@ static void test_xdp_adjust_tail_grow2(void)
 {
 	const char *file = "./test_xdp_adjust_tail_grow.bpf.o";
 	char buf[4096]; /* avoid segfault: large buf to hold grow results */
-	int tailroom = 320; /* SKB_DATA_ALIGN(sizeof(struct skb_shared_info))*/;
 	struct bpf_object *obj;
 	int err, cnt, i;
 	int max_grow, prog_fd;
+	/* SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) */
+#if defined(__s390x__)
+	int tailroom = 512;
+#else
+	int tailroom = 320;
+#endif
 
 	LIBBPF_OPTS(bpf_test_run_opts, tattr,
 		.repeat		= 1,
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c
index 53b64c999450..297c260fc364 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c
@@ -9,6 +9,12 @@ int _xdp_adjust_tail_grow(struct xdp_md *xdp)
 	void *data = (void *)(long)xdp->data;
 	int data_len = bpf_xdp_get_buff_len(xdp);
 	int offset = 0;
+	/* SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) */
+#if defined(__TARGET_ARCH_s390)
+	int tailroom = 512;
+#else
+	int tailroom = 320;
+#endif
 
 	/* Data length determine test case */
 
@@ -20,7 +26,7 @@ int _xdp_adjust_tail_grow(struct xdp_md *xdp)
 		offset = 128;
 	} else if (data_len == 128) {
 		/* Max tail grow 3520 */
-		offset = 4096 - 256 - 320 - data_len;
+		offset = 4096 - 256 - tailroom - data_len;
 	} else if (data_len == 9000) {
 		offset = 10;
 	} else if (data_len == 9001) {
-- 
2.39.1

[PATCH bpf-next 16/24] selftests/bpf: Fix vmlinux test on s390x

[Ilya Leoshkevich]

Use a syscall macro to access the nanosleep()'s first argument;
currently the code uses gprs[2] instead of orig_gpr2.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/progs/test_vmlinux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_vmlinux.c b/tools/testing/selftests/bpf/progs/test_vmlinux.c
index e9dfa0313d1b..4b8e37f7fd06 100644
--- a/tools/testing/selftests/bpf/progs/test_vmlinux.c
+++ b/tools/testing/selftests/bpf/progs/test_vmlinux.c
@@ -42,7 +42,7 @@ int BPF_PROG(handle__raw_tp, struct pt_regs *regs, long id)
 	if (id != __NR_nanosleep)
 		return 0;
 
-	ts = (void *)PT_REGS_PARM1_CORE(regs);
+	ts = (void *)PT_REGS_PARM1_CORE_SYSCALL(regs);
 	if (bpf_probe_read_user(&tv_nsec, sizeof(ts->tv_nsec), &ts->tv_nsec) ||
 	    tv_nsec != MY_TV_NSEC)
 		return 0;
@@ -60,7 +60,7 @@ int BPF_PROG(handle__tp_btf, struct pt_regs *regs, long id)
 	if (id != __NR_nanosleep)
 		return 0;
 
-	ts = (void *)PT_REGS_PARM1_CORE(regs);
+	ts = (void *)PT_REGS_PARM1_CORE_SYSCALL(regs);
 	if (bpf_probe_read_user(&tv_nsec, sizeof(ts->tv_nsec), &ts->tv_nsec) ||
 	    tv_nsec != MY_TV_NSEC)
 		return 0;
-- 
2.39.1

[PATCH bpf-next 17/24] libbpf: Read usdt arg spec with bpf_probe_read_kernel()

[Ilya Leoshkevich]

Loading programs that use bpf_usdt_arg() on s390x fails with:

    ; switch (arg_spec->arg_type) {
    139: (61) r1 = *(u32 *)(r2 +8)
    R2 unbounded memory access, make sure to bounds check any such access

The bound checks seem to be already in place in the C code, and maybe
it's even possible to add extra bogus checks to placate LLVM or the
verifier. Take a simpler approach and just use a helper.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/usdt.bpf.h | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
index fdfd235e52c4..ddfa2521ab67 100644
--- a/tools/lib/bpf/usdt.bpf.h
+++ b/tools/lib/bpf/usdt.bpf.h
@@ -116,7 +116,7 @@ __weak __hidden
 int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
 {
 	struct __bpf_usdt_spec *spec;
-	struct __bpf_usdt_arg_spec *arg_spec;
+	struct __bpf_usdt_arg_spec arg_spec;
 	unsigned long val;
 	int err, spec_id;
 
@@ -133,21 +133,24 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
 	if (arg_num >= BPF_USDT_MAX_ARG_CNT || arg_num >= spec->arg_cnt)
 		return -ENOENT;
 
-	arg_spec = &spec->args[arg_num];
-	switch (arg_spec->arg_type) {
+	err = bpf_probe_read_kernel(&arg_spec, sizeof(arg_spec), &spec->args[arg_num]);
+	if (err)
+		return err;
+
+	switch (arg_spec.arg_type) {
 	case BPF_USDT_ARG_CONST:
 		/* Arg is just a constant ("-4@$-9" in USDT arg spec).
-		 * value is recorded in arg_spec->val_off directly.
+		 * value is recorded in arg_spec.val_off directly.
 		 */
-		val = arg_spec->val_off;
+		val = arg_spec.val_off;
 		break;
 	case BPF_USDT_ARG_REG:
 		/* Arg is in a register (e.g, "8@%rax" in USDT arg spec),
 		 * so we read the contents of that register directly from
 		 * struct pt_regs. To keep things simple user-space parts
-		 * record offsetof(struct pt_regs, <regname>) in arg_spec->reg_off.
+		 * record offsetof(struct pt_regs, <regname>) in arg_spec.reg_off.
 		 */
-		err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec->reg_off);
+		err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec.reg_off);
 		if (err)
 			return err;
 		break;
@@ -155,18 +158,18 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
 		/* Arg is in memory addressed by register, plus some offset
 		 * (e.g., "-4@-1204(%rbp)" in USDT arg spec). Register is
 		 * identified like with BPF_USDT_ARG_REG case, and the offset
-		 * is in arg_spec->val_off. We first fetch register contents
+		 * is in arg_spec.val_off. We first fetch register contents
 		 * from pt_regs, then do another user-space probe read to
 		 * fetch argument value itself.
 		 */
-		err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec->reg_off);
+		err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec.reg_off);
 		if (err)
 			return err;
-		err = bpf_probe_read_user(&val, sizeof(val), (void *)val + arg_spec->val_off);
+		err = bpf_probe_read_user(&val, sizeof(val), (void *)val + arg_spec.val_off);
 		if (err)
 			return err;
 #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
-		val >>= arg_spec->arg_bitshift;
+		val >>= arg_spec.arg_bitshift;
 #endif
 		break;
 	default:
@@ -177,11 +180,11 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
 	 * necessary upper arg_bitshift bits, with sign extension if argument
 	 * is signed
 	 */
-	val <<= arg_spec->arg_bitshift;
-	if (arg_spec->arg_signed)
-		val = ((long)val) >> arg_spec->arg_bitshift;
+	val <<= arg_spec.arg_bitshift;
+	if (arg_spec.arg_signed)
+		val = ((long)val) >> arg_spec.arg_bitshift;
 	else
-		val = val >> arg_spec->arg_bitshift;
+		val = val >> arg_spec.arg_bitshift;
 	*res = val;
 	return 0;
 }
-- 
2.39.1

Re: [PATCH bpf-next 17/24] libbpf: Read usdt arg spec with bpf_probe_read_kernel()

[Andrii Nakryiko]

On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Loading programs that use bpf_usdt_arg() on s390x fails with:
>
>     ; switch (arg_spec->arg_type) {
>     139: (61) r1 = *(u32 *)(r2 +8)
>     R2 unbounded memory access, make sure to bounds check any such access

can you show a bit longer log? we shouldn't just  use
bpf_probe_read_kernel for this. I suspect strategically placed
barrier_var() calls will solve this. This is usually an issue with
compiler reordering operations and doing actual check after it already
speculatively adjusted pointer (which is technically safe and ok if we
never deref that pointer, but verifier doesn't recognize such pattern)

>
> The bound checks seem to be already in place in the C code, and maybe
> it's even possible to add extra bogus checks to placate LLVM or the
> verifier. Take a simpler approach and just use a helper.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/lib/bpf/usdt.bpf.h | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
> index fdfd235e52c4..ddfa2521ab67 100644
> --- a/tools/lib/bpf/usdt.bpf.h
> +++ b/tools/lib/bpf/usdt.bpf.h
> @@ -116,7 +116,7 @@ __weak __hidden
>  int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
>  {
>         struct __bpf_usdt_spec *spec;
> -       struct __bpf_usdt_arg_spec *arg_spec;
> +       struct __bpf_usdt_arg_spec arg_spec;
>         unsigned long val;
>         int err, spec_id;
>
> @@ -133,21 +133,24 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
>         if (arg_num >= BPF_USDT_MAX_ARG_CNT || arg_num >= spec->arg_cnt)
>                 return -ENOENT;
>
> -       arg_spec = &spec->args[arg_num];
> -       switch (arg_spec->arg_type) {
> +       err = bpf_probe_read_kernel(&arg_spec, sizeof(arg_spec), &spec->args[arg_num]);
> +       if (err)
> +               return err;
> +
> +       switch (arg_spec.arg_type) {
>         case BPF_USDT_ARG_CONST:
>                 /* Arg is just a constant ("-4@$-9" in USDT arg spec).
> -                * value is recorded in arg_spec->val_off directly.
> +                * value is recorded in arg_spec.val_off directly.
>                  */
> -               val = arg_spec->val_off;
> +               val = arg_spec.val_off;
>                 break;
>         case BPF_USDT_ARG_REG:
>                 /* Arg is in a register (e.g, "8@%rax" in USDT arg spec),
>                  * so we read the contents of that register directly from
>                  * struct pt_regs. To keep things simple user-space parts
> -                * record offsetof(struct pt_regs, <regname>) in arg_spec->reg_off.
> +                * record offsetof(struct pt_regs, <regname>) in arg_spec.reg_off.
>                  */
> -               err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec->reg_off);
> +               err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec.reg_off);
>                 if (err)
>                         return err;
>                 break;
> @@ -155,18 +158,18 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
>                 /* Arg is in memory addressed by register, plus some offset
>                  * (e.g., "-4@-1204(%rbp)" in USDT arg spec). Register is
>                  * identified like with BPF_USDT_ARG_REG case, and the offset
> -                * is in arg_spec->val_off. We first fetch register contents
> +                * is in arg_spec.val_off. We first fetch register contents
>                  * from pt_regs, then do another user-space probe read to
>                  * fetch argument value itself.
>                  */
> -               err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec->reg_off);
> +               err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec.reg_off);
>                 if (err)
>                         return err;
> -               err = bpf_probe_read_user(&val, sizeof(val), (void *)val + arg_spec->val_off);
> +               err = bpf_probe_read_user(&val, sizeof(val), (void *)val + arg_spec.val_off);
>                 if (err)
>                         return err;
>  #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> -               val >>= arg_spec->arg_bitshift;
> +               val >>= arg_spec.arg_bitshift;
>  #endif
>                 break;
>         default:
> @@ -177,11 +180,11 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
>          * necessary upper arg_bitshift bits, with sign extension if argument
>          * is signed
>          */
> -       val <<= arg_spec->arg_bitshift;
> -       if (arg_spec->arg_signed)
> -               val = ((long)val) >> arg_spec->arg_bitshift;
> +       val <<= arg_spec.arg_bitshift;
> +       if (arg_spec.arg_signed)
> +               val = ((long)val) >> arg_spec.arg_bitshift;
>         else
> -               val = val >> arg_spec->arg_bitshift;
> +               val = val >> arg_spec.arg_bitshift;
>         *res = val;
>         return 0;
>  }
> --
> 2.39.1
>

Re: [PATCH bpf-next 17/24] libbpf: Read usdt arg spec with bpf_probe_read_kernel()

[Ilya Leoshkevich]

On Wed, 2023-01-25 at 16:26 -0800, Andrii Nakryiko wrote:
> On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > Loading programs that use bpf_usdt_arg() on s390x fails with:
> > 
> >     ; switch (arg_spec->arg_type) {
> >     139: (61) r1 = *(u32 *)(r2 +8)
> >     R2 unbounded memory access, make sure to bounds check any such
> > access
> 
> can you show a bit longer log? we shouldn't just  use
> bpf_probe_read_kernel for this. I suspect strategically placed
> barrier_var() calls will solve this. This is usually an issue with
> compiler reordering operations and doing actual check after it
> already
> speculatively adjusted pointer (which is technically safe and ok if
> we
> never deref that pointer, but verifier doesn't recognize such
> pattern)

The full log is here:

https://gist.github.com/iii-i/b6149ee99b37078ec920ab1d3bb45134

The relevant part seems to be:

; if (arg_num >= BPF_USDT_MAX_ARG_CNT || arg_num >= spec->arg_cnt)
128: (79) r1 = *(u64 *)(r10 -24)      ; frame1:
R1_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R10=fp0
129: (25) if r1 > 0xb goto pc+83      ; frame1:
R1_w=scalar(umax=11,var_off=(0x0; 0xf))
; if (arg_num >= BPF_USDT_MAX_ARG_CNT || arg_num >= spec->arg_cnt)
130: (69) r1 = *(u16 *)(r8 +200)      ; frame1:
R1_w=scalar(umax=65535,var_off=(0x0; 0xffff))
R8_w=map_value(off=0,ks=4,vs=208,imm=0)
131: (67) r1 <<= 48                   ; frame1:
R1_w=scalar(smax=9223090561878065152,umax=18446462598732840960,var_off=
(0x0; 0xffff000000000000),s32_min=0,s32_max=0,u32_max=0)
132: (c7) r1 s>>= 48                  ; frame1: R1_w=scalar(smin=-
32768,smax=32767)
; if (arg_num >= BPF_USDT_MAX_ARG_CNT || arg_num >= spec->arg_cnt)
133: (79) r2 = *(u64 *)(r10 -24)      ; frame1:
R2=scalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R10=fp0
134: (bd) if r1 <= r2 goto pc+78      ; frame1: R1=scalar(smin=-
32768,smax=32767) R2=scalar(umax=4294967295,var_off=(0x0; 0xffffffff))
; arg_spec = &spec->args[arg_num];
135: (79) r1 = *(u64 *)(r10 -24)      ; frame1:
R1_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R10=fp0
136: (67) r1 <<= 4                    ; frame1:
R1_w=scalar(umax=68719476720,var_off=(0x0;
0xffffffff0),s32_max=2147483632,u32_max=-16)
137: (bf) r2 = r8                     ; frame1:
R2_w=map_value(off=0,ks=4,vs=208,imm=0)
R8=map_value(off=0,ks=4,vs=208,imm=0)
138: (0f) r2 += r1                    ; frame1:
R1_w=scalar(umax=68719476720,var_off=(0x0;
0xffffffff0),s32_max=2147483632,u32_max=-16)
R2_w=map_value(off=0,ks=4,vs=208,umax=68719476720,var_off=(0x0;
0xffffffff0),s32_max=2147483632,u32_max=-16)
; switch (arg_spec->arg_type) {
139: (61) r1 = *(u32 *)(r2 +8)

#128-#129 make sure that *(u64 *)(r10 -24) <= 11, but when #133
loads it again, this constraint is not there. I guess we need to
force flushing r1 to stack? The following helps:

--- a/tools/lib/bpf/usdt.bpf.h
+++ b/tools/lib/bpf/usdt.bpf.h
@@ -130,7 +130,10 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64
arg_num, long *res)
        if (!spec)
                return -ESRCH;
 
-       if (arg_num >= BPF_USDT_MAX_ARG_CNT || arg_num >= spec-
>arg_cnt)
+       if (arg_num >= BPF_USDT_MAX_ARG_CNT)
+               return -ENOENT;
+       barrier_var(arg_num);
+       if (arg_num >= spec->arg_cnt)
                return -ENOENT;
 
        arg_spec = &spec->args[arg_num];

I can use this in v2 if it looks good.



Btw, I looked at the barrier_var() definition:

#define barrier_var(var) asm volatile("" : "=r"(var) : "0"(var))

and I'm curious why it's not defined like this:

#define barrier_var(var) asm volatile("" : "+r"(var))

which is a bit simpler?
> 

Re: [PATCH bpf-next 17/24] libbpf: Read usdt arg spec with bpf_probe_read_kernel()

[Andrii Nakryiko]

On Thu, Jan 26, 2023 at 3:41 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Wed, 2023-01-25 at 16:26 -0800, Andrii Nakryiko wrote:
> > On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com>
> > wrote:
> > >
> > > Loading programs that use bpf_usdt_arg() on s390x fails with:
> > >
> > >     ; switch (arg_spec->arg_type) {
> > >     139: (61) r1 = *(u32 *)(r2 +8)
> > >     R2 unbounded memory access, make sure to bounds check any such
> > > access
> >
> > can you show a bit longer log? we shouldn't just  use
> > bpf_probe_read_kernel for this. I suspect strategically placed
> > barrier_var() calls will solve this. This is usually an issue with
> > compiler reordering operations and doing actual check after it
> > already
> > speculatively adjusted pointer (which is technically safe and ok if
> > we
> > never deref that pointer, but verifier doesn't recognize such
> > pattern)
>
> The full log is here:
>
> https://gist.github.com/iii-i/b6149ee99b37078ec920ab1d3bb45134
>
> The relevant part seems to be:
>
> ; if (arg_num >= BPF_USDT_MAX_ARG_CNT || arg_num >= spec->arg_cnt)
> 128: (79) r1 = *(u64 *)(r10 -24)      ; frame1:
> R1_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R10=fp0
> 129: (25) if r1 > 0xb goto pc+83      ; frame1:
> R1_w=scalar(umax=11,var_off=(0x0; 0xf))
> ; if (arg_num >= BPF_USDT_MAX_ARG_CNT || arg_num >= spec->arg_cnt)
> 130: (69) r1 = *(u16 *)(r8 +200)      ; frame1:
> R1_w=scalar(umax=65535,var_off=(0x0; 0xffff))
> R8_w=map_value(off=0,ks=4,vs=208,imm=0)
> 131: (67) r1 <<= 48                   ; frame1:
> R1_w=scalar(smax=9223090561878065152,umax=18446462598732840960,var_off=
> (0x0; 0xffff000000000000),s32_min=0,s32_max=0,u32_max=0)
> 132: (c7) r1 s>>= 48                  ; frame1: R1_w=scalar(smin=-
> 32768,smax=32767)
> ; if (arg_num >= BPF_USDT_MAX_ARG_CNT || arg_num >= spec->arg_cnt)
> 133: (79) r2 = *(u64 *)(r10 -24)      ; frame1:
> R2=scalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R10=fp0
> 134: (bd) if r1 <= r2 goto pc+78      ; frame1: R1=scalar(smin=-
> 32768,smax=32767) R2=scalar(umax=4294967295,var_off=(0x0; 0xffffffff))
> ; arg_spec = &spec->args[arg_num];
> 135: (79) r1 = *(u64 *)(r10 -24)      ; frame1:
> R1_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R10=fp0
> 136: (67) r1 <<= 4                    ; frame1:
> R1_w=scalar(umax=68719476720,var_off=(0x0;
> 0xffffffff0),s32_max=2147483632,u32_max=-16)
> 137: (bf) r2 = r8                     ; frame1:
> R2_w=map_value(off=0,ks=4,vs=208,imm=0)
> R8=map_value(off=0,ks=4,vs=208,imm=0)
> 138: (0f) r2 += r1                    ; frame1:
> R1_w=scalar(umax=68719476720,var_off=(0x0;
> 0xffffffff0),s32_max=2147483632,u32_max=-16)
> R2_w=map_value(off=0,ks=4,vs=208,umax=68719476720,var_off=(0x0;
> 0xffffffff0),s32_max=2147483632,u32_max=-16)
> ; switch (arg_spec->arg_type) {
> 139: (61) r1 = *(u32 *)(r2 +8)
>
> #128-#129 make sure that *(u64 *)(r10 -24) <= 11, but when #133
> loads it again, this constraint is not there. I guess we need to
> force flushing r1 to stack? The following helps:
>
> --- a/tools/lib/bpf/usdt.bpf.h
> +++ b/tools/lib/bpf/usdt.bpf.h
> @@ -130,7 +130,10 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64
> arg_num, long *res)
>         if (!spec)
>                 return -ESRCH;
>
> -       if (arg_num >= BPF_USDT_MAX_ARG_CNT || arg_num >= spec-
> >arg_cnt)
> +       if (arg_num >= BPF_USDT_MAX_ARG_CNT)
> +               return -ENOENT;
> +       barrier_var(arg_num);
> +       if (arg_num >= spec->arg_cnt)
>                 return -ENOENT;
>
>         arg_spec = &spec->args[arg_num];
>
> I can use this in v2 if it looks good.

arg_num -> spec->arg_cnt is "real" check, arg_num >=
BPF_USDT_MAX_ARG_CNT is more to satisfy verifier (we know that
spec->arg_cnt won't be >= BPF_USDT_MAX_ARG_CNT). Let's swap two checks
in order and keep BPF_USDT_MAX_ARG_CNT close to spec->args[arg_num]
use? And if barrier_var() is necessary, then so be it.

>
>
>
> Btw, I looked at the barrier_var() definition:
>
> #define barrier_var(var) asm volatile("" : "=r"(var) : "0"(var))
>
> and I'm curious why it's not defined like this:
>
> #define barrier_var(var) asm volatile("" : "+r"(var))
>
> which is a bit simpler?
> >

no reason, just unfamiliarity with embedded asm back then, we can
update it we they are equivalent

Re: [PATCH bpf-next 17/24] libbpf: Read usdt arg spec with bpf_probe_read_kernel()

[Ilya Leoshkevich]

On Thu, 2023-01-26 at 11:03 -0800, Andrii Nakryiko wrote:
> On Thu, Jan 26, 2023 at 3:41 AM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > On Wed, 2023-01-25 at 16:26 -0800, Andrii Nakryiko wrote:
> > > On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich
> > > <iii@linux.ibm.com>
> > > wrote:
> > > > 
> > > > Loading programs that use bpf_usdt_arg() on s390x fails with:
> > > > 
> > > >     ; switch (arg_spec->arg_type) {
> > > >     139: (61) r1 = *(u32 *)(r2 +8)
> > > >     R2 unbounded memory access, make sure to bounds check any
> > > > such
> > > > access
> > > 
> > > can you show a bit longer log? we shouldn't just  use
> > > bpf_probe_read_kernel for this. I suspect strategically placed
> > > barrier_var() calls will solve this. This is usually an issue
> > > with
> > > compiler reordering operations and doing actual check after it
> > > already
> > > speculatively adjusted pointer (which is technically safe and ok
> > > if
> > > we
> > > never deref that pointer, but verifier doesn't recognize such
> > > pattern)
> > 
> > The full log is here:
> > 
> > https://gist.github.com/iii-i/b6149ee99b37078ec920ab1d3bb45134

[...]

> > --- a/tools/lib/bpf/usdt.bpf.h
> > +++ b/tools/lib/bpf/usdt.bpf.h
> > @@ -130,7 +130,10 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64
> > arg_num, long *res)
> >         if (!spec)
> >                 return -ESRCH;
> > 
> > -       if (arg_num >= BPF_USDT_MAX_ARG_CNT || arg_num >= spec-
> > > arg_cnt)
> > +       if (arg_num >= BPF_USDT_MAX_ARG_CNT)
> > +               return -ENOENT;
> > +       barrier_var(arg_num);
> > +       if (arg_num >= spec->arg_cnt)
> >                 return -ENOENT;
> > 
> >         arg_spec = &spec->args[arg_num];
> > 
> > I can use this in v2 if it looks good.
> 
> arg_num -> spec->arg_cnt is "real" check, arg_num >=
> BPF_USDT_MAX_ARG_CNT is more to satisfy verifier (we know that
> spec->arg_cnt won't be >= BPF_USDT_MAX_ARG_CNT). Let's swap two
> checks
> in order and keep BPF_USDT_MAX_ARG_CNT close to spec->args[arg_num]
> use? And if barrier_var() is necessary, then so be it.

Unfortunately just swapping did not help, so let's use the barrier.

> > Btw, I looked at the barrier_var() definition:
> > 
> > #define barrier_var(var) asm volatile("" : "=r"(var) : "0"(var))
> > 
> > and I'm curious why it's not defined like this:
> > 
> > #define barrier_var(var) asm volatile("" : "+r"(var))
> > 
> > which is a bit simpler?
> > > 
> 
> no reason, just unfamiliarity with embedded asm back then, we can
> update it we they are equivalent

Thanks! I will add a simplification in v2 then.

[PATCH bpf-next 18/24] s390/bpf: Fix a typo in a comment

[Ilya Leoshkevich]

"decription" should be "description".

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index af35052d06ed..eb1a78c0e6a8 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -510,7 +510,7 @@ static void bpf_skip(struct bpf_jit *jit, int size)
  * Emit function prologue
  *
  * Save registers and create stack frame if necessary.
- * See stack frame layout desription in "bpf_jit.h"!
+ * See stack frame layout description in "bpf_jit.h"!
  */
 static void bpf_jit_prologue(struct bpf_jit *jit, u32 stack_depth)
 {
-- 
2.39.1

[PATCH bpf-next 19/24] s390/bpf: Add expoline to tail calls

[Ilya Leoshkevich]

All the indirect jumps in the eBPF JIT already use expolines, except
for the tail call one.

Fixes: de5cb6eb514e ("s390: use expoline thunks in the BPF JIT")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index eb1a78c0e6a8..8400a06c926e 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -1393,8 +1393,16 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 		/* lg %r1,bpf_func(%r1) */
 		EMIT6_DISP_LH(0xe3000000, 0x0004, REG_1, REG_1, REG_0,
 			      offsetof(struct bpf_prog, bpf_func));
-		/* bc 0xf,tail_call_start(%r1) */
-		_EMIT4(0x47f01000 + jit->tail_call_start);
+		if (nospec_uses_trampoline()) {
+			jit->seen |= SEEN_FUNC;
+			/* aghi %r1,tail_call_start */
+			EMIT4_IMM(0xa70b0000, REG_1, jit->tail_call_start);
+			/* brcl 0xf,__s390_indirect_jump_r1 */
+			EMIT6_PCREL_RILC(0xc0040000, 0xf, jit->r1_thunk_ip);
+		} else {
+			/* bc 0xf,tail_call_start(%r1) */
+			_EMIT4(0x47f01000 + jit->tail_call_start);
+		}
 		/* out: */
 		if (jit->prg_buf) {
 			*(u16 *)(jit->prg_buf + patch_1_clrj + 2) =
-- 
2.39.1

[PATCH bpf-next 20/24] s390/bpf: Implement bpf_arch_text_poke()

[Ilya Leoshkevich]

bpf_arch_text_poke() is used to hotpatch eBPF programs and trampolines.
s390x has a very strict hotpatching restriction: the only thing that is
allowed to be hotpatched is conditional branch mask.

Take the same approach as commit de5012b41e5c ("s390/ftrace: implement
hotpatching"): create a conditional jump to a "plt", which loads the
target address from memory and jumps to it; then first patch this
address, and then the mask.

Trampolines (introduced in the next patch) respect the ftrace calling
convention: the return address is in %r0, and %r1 is clobbered. With
that in mind, bpf_arch_text_poke() does not differentiate between jumps
and calls.

However, there is a simple optimization for jumps (for the epilogue_ip
case): if a jump already points to the destination, then there is no
"plt" and we can just flip the mask.

For simplicity, the "plt" template is defined in assembly, and its size
is used to define C arrays. There doesn't seem to be a way to convey
this size to C as a constant, so it's hardcoded and double-checked
during runtime.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 97 ++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 8400a06c926e..c72eb3fc1f98 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -30,6 +30,7 @@
 #include <asm/facility.h>
 #include <asm/nospec-branch.h>
 #include <asm/set_memory.h>
+#include <asm/text-patching.h>
 #include "bpf_jit.h"
 
 struct bpf_jit {
@@ -50,6 +51,8 @@ struct bpf_jit {
 	int r14_thunk_ip;	/* Address of expoline thunk for 'br %r14' */
 	int tail_call_start;	/* Tail call start offset */
 	int excnt;		/* Number of exception table entries */
+	int prologue_plt_ret;	/* Return address for prologue hotpatch PLT */
+	int prologue_plt;	/* Start of prologue hotpatch PLT */
 };
 
 #define SEEN_MEM	BIT(0)		/* use mem[] for temporary storage */
@@ -506,6 +509,36 @@ static void bpf_skip(struct bpf_jit *jit, int size)
 	}
 }
 
+/*
+ * PLT for hotpatchable calls. The calling convention is the same as for the
+ * ftrace hotpatch trampolines: %r0 is return address, %r1 is clobbered.
+ */
+extern const char bpf_plt[];
+extern const char bpf_plt_ret[];
+extern const char bpf_plt_target[];
+extern const char bpf_plt_end[];
+#define BPF_PLT_SIZE 32
+asm(
+	".pushsection .rodata\n"
+	"	.align 8\n"
+	"bpf_plt:\n"
+	"	lgrl %r0,bpf_plt_ret\n"
+	"	lgrl %r1,bpf_plt_target\n"
+	"	br %r1\n"
+	"	.align 8\n"
+	"bpf_plt_ret: .quad 0\n"
+	"bpf_plt_target: .quad 0\n"
+	"bpf_plt_end:\n"
+	"	.popsection\n"
+);
+
+static void bpf_jit_plt(void *plt, void *ret, void *target)
+{
+	memcpy(plt, bpf_plt, BPF_PLT_SIZE);
+	*(void **)((char *)plt + (bpf_plt_ret - bpf_plt)) = ret;
+	*(void **)((char *)plt + (bpf_plt_target - bpf_plt)) = target;
+}
+
 /*
  * Emit function prologue
  *
@@ -514,6 +547,11 @@ static void bpf_skip(struct bpf_jit *jit, int size)
  */
 static void bpf_jit_prologue(struct bpf_jit *jit, u32 stack_depth)
 {
+	/* No-op for hotpatching */
+	/* brcl 0,prologue_plt */
+	EMIT6_PCREL_RILC(0xc0040000, 0, jit->prologue_plt);
+	jit->prologue_plt_ret = jit->prg;
+
 	if (jit->seen & SEEN_TAIL_CALL) {
 		/* xc STK_OFF_TCCNT(4,%r15),STK_OFF_TCCNT(%r15) */
 		_EMIT6(0xd703f000 | STK_OFF_TCCNT, 0xf000 | STK_OFF_TCCNT);
@@ -589,6 +627,13 @@ static void bpf_jit_epilogue(struct bpf_jit *jit, u32 stack_depth)
 		/* br %r1 */
 		_EMIT2(0x07f1);
 	}
+
+	jit->prg = ALIGN(jit->prg, 8);
+	jit->prologue_plt = jit->prg;
+	if (jit->prg_buf)
+		bpf_jit_plt(jit->prg_buf + jit->prg,
+			    jit->prg_buf + jit->prologue_plt_ret, NULL);
+	jit->prg += BPF_PLT_SIZE;
 }
 
 static int get_probe_mem_regno(const u8 *insn)
@@ -1776,6 +1821,9 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
 	struct bpf_jit jit;
 	int pass;
 
+	if (WARN_ON_ONCE(bpf_plt_end - bpf_plt != BPF_PLT_SIZE))
+		return orig_fp;
+
 	if (!fp->jit_requested)
 		return orig_fp;
 
@@ -1867,3 +1915,52 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
 					   tmp : orig_fp);
 	return fp;
 }
+
+int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
+		       void *old_addr, void *new_addr)
+{
+	struct {
+		u16 opc;
+		s32 disp;
+	} __packed insn;
+	char expected_plt[BPF_PLT_SIZE];
+	char current_plt[BPF_PLT_SIZE];
+	char *plt;
+	int err;
+
+	/* Verify the branch to be patched. */
+	err = copy_from_kernel_nofault(&insn, ip, sizeof(insn));
+	if (err < 0)
+		return err;
+	if (insn.opc != (0xc004 | (old_addr ? 0xf0 : 0)))
+		return -EINVAL;
+
+	if (t == BPF_MOD_JUMP &&
+	    insn.disp == ((char *)new_addr - (char *)ip) >> 1) {
+		/*
+		 * The branch already points to the destination,
+		 * there is no PLT.
+		 */
+	} else {
+		/* Verify the PLT. */
+		plt = (char *)ip + (insn.disp << 1);
+		err = copy_from_kernel_nofault(current_plt, plt, BPF_PLT_SIZE);
+		if (err < 0)
+			return err;
+		bpf_jit_plt(expected_plt, (char *)ip + 6, old_addr);
+		if (memcmp(current_plt, expected_plt, BPF_PLT_SIZE))
+			return -EINVAL;
+		/* Adjust the call address. */
+		s390_kernel_write(plt + (bpf_plt_target - bpf_plt),
+				  &new_addr, sizeof(void *));
+	}
+
+	/* Adjust the mask of the branch. */
+	insn.opc = 0xc004 | (new_addr ? 0xf0 : 0);
+	s390_kernel_write((char *)ip + 1, (char *)&insn.opc + 1, 1);
+
+	/* Make the new code visible to the other CPUs. */
+	text_poke_sync_lock();
+
+	return 0;
+}
-- 
2.39.1

[PATCH bpf-next 21/24] bpf: btf: Add BTF_FMODEL_SIGNED_ARG flag

[Ilya Leoshkevich]

s390x eBPF JIT needs to know whether a function return value is signed
and which function arguments are signed, in order to generate code
compliant with the s390x ABI.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 include/linux/bpf.h |  4 ++++
 include/linux/btf.h | 15 ++++++++++-----
 kernel/bpf/btf.c    | 16 +++++++++++++++-
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 14a0264fac57..cf89504c8dda 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -899,8 +899,12 @@ enum bpf_cgroup_storage_type {
 /* The argument is a structure. */
 #define BTF_FMODEL_STRUCT_ARG		BIT(0)
 
+/* The argument is signed. */
+#define BTF_FMODEL_SIGNED_ARG		BIT(1)
+
 struct btf_func_model {
 	u8 ret_size;
+	u8 ret_flags;
 	u8 nr_args;
 	u8 arg_size[MAX_BPF_FUNC_ARGS];
 	u8 arg_flags[MAX_BPF_FUNC_ARGS];
diff --git a/include/linux/btf.h b/include/linux/btf.h
index 5f628f323442..e9b90d9c3569 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -236,6 +236,16 @@ static inline bool btf_type_is_small_int(const struct btf_type *t)
 	return btf_type_is_int(t) && t->size <= sizeof(u64);
 }
 
+static inline u8 btf_int_encoding(const struct btf_type *t)
+{
+	return BTF_INT_ENCODING(*(u32 *)(t + 1));
+}
+
+static inline bool btf_type_is_signed_int(const struct btf_type *t)
+{
+	return btf_type_is_int(t) && (btf_int_encoding(t) & BTF_INT_SIGNED);
+}
+
 static inline bool btf_type_is_enum(const struct btf_type *t)
 {
 	return BTF_INFO_KIND(t->info) == BTF_KIND_ENUM;
@@ -306,11 +316,6 @@ static inline u8 btf_int_offset(const struct btf_type *t)
 	return BTF_INT_OFFSET(*(u32 *)(t + 1));
 }
 
-static inline u8 btf_int_encoding(const struct btf_type *t)
-{
-	return BTF_INT_ENCODING(*(u32 *)(t + 1));
-}
-
 static inline bool btf_type_is_scalar(const struct btf_type *t)
 {
 	return btf_type_is_int(t) || btf_type_is_enum(t);
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 47b8cb96f2c2..1622a3b15d6f 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6453,6 +6453,18 @@ static int __get_type_size(struct btf *btf, u32 btf_id,
 	return -EINVAL;
 }
 
+static u8 __get_type_fmodel_flags(const struct btf_type *t)
+{
+	u8 flags = 0;
+
+	if (__btf_type_is_struct(t))
+		flags |= BTF_FMODEL_STRUCT_ARG;
+	if (btf_type_is_signed_int(t))
+		flags |= BTF_FMODEL_SIGNED_ARG;
+
+	return flags;
+}
+
 int btf_distill_func_proto(struct bpf_verifier_log *log,
 			   struct btf *btf,
 			   const struct btf_type *func,
@@ -6473,6 +6485,7 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 			m->arg_flags[i] = 0;
 		}
 		m->ret_size = 8;
+		m->ret_flags = 0;
 		m->nr_args = MAX_BPF_FUNC_REG_ARGS;
 		return 0;
 	}
@@ -6492,6 +6505,7 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 		return -EINVAL;
 	}
 	m->ret_size = ret;
+	m->ret_flags = __get_type_fmodel_flags(t);
 
 	for (i = 0; i < nargs; i++) {
 		if (i == nargs - 1 && args[i].type == 0) {
@@ -6516,7 +6530,7 @@ int btf_distill_func_proto(struct bpf_verifier_log *log,
 			return -EINVAL;
 		}
 		m->arg_size[i] = ret;
-		m->arg_flags[i] = __btf_type_is_struct(t) ? BTF_FMODEL_STRUCT_ARG : 0;
+		m->arg_flags[i] = __get_type_fmodel_flags(t);
 	}
 	m->nr_args = nargs;
 	return 0;
-- 
2.39.1

[PATCH bpf-next 22/24] s390/bpf: Implement arch_prepare_bpf_trampoline()

[Ilya Leoshkevich]

arch_prepare_bpf_trampoline() is used for direct attachment of eBPF
programs to various places, bypassing kprobes. It's responsible for
calling a number of eBPF programs before, instead and/or after
whatever they are attached to.

Add a s390x implementation, paying attention to the following:

- Reuse the existing JIT infrastructure, where possible.
- Like the existing JIT, prefer making multiple passes instead of
  backpatching. Currently 2 passes is enough. If literal pool is
  introduced, this needs to be raised to 3. However, at the moment
  adding literal pool only makes the code larger. If branch
  shortening is introduced, the number of passes needs to be
  increased even further.
- Support both regular and ftrace calling conventions, depending on
  the trampoline flags.
- Use expolines for indirect calls.
- Handle the mismatch between the eBPF and the s390x ABIs.
- Sign-extend fmod_ret return values.

invoke_bpf_prog() produces about 120 bytes; it might be possible to
slightly optimize this, but reaching 50 bytes, like on x86_64, looks
unrealistic: just loading cookie, __bpf_prog_enter, bpf_func, insnsi
and __bpf_prog_exit as literals already takes at least 5 * 12 = 60
bytes, and we can't use relative addressing for most of them.
Therefore, lower BPF_MAX_TRAMP_LINKS on s390x.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 535 +++++++++++++++++++++++++++++++++--
 include/linux/bpf.h          |   4 +
 2 files changed, 517 insertions(+), 22 deletions(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index c72eb3fc1f98..ea8203bd4112 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -71,6 +71,10 @@ struct bpf_jit {
 #define REG_0		REG_W0			/* Register 0 */
 #define REG_1		REG_W1			/* Register 1 */
 #define REG_2		BPF_REG_1		/* Register 2 */
+#define REG_3		BPF_REG_2		/* Register 3 */
+#define REG_4		BPF_REG_3		/* Register 4 */
+#define REG_7		BPF_REG_6		/* Register 7 */
+#define REG_8		BPF_REG_7		/* Register 8 */
 #define REG_14		BPF_REG_0		/* Register 14 */
 
 /*
@@ -595,6 +599,43 @@ static void bpf_jit_prologue(struct bpf_jit *jit, u32 stack_depth)
 	}
 }
 
+/*
+ * Emit an expoline for a jump that follows
+ */
+static void emit_expoline(struct bpf_jit *jit)
+{
+	/* exrl %r0,.+10 */
+	EMIT6_PCREL_RIL(0xc6000000, jit->prg + 10);
+	/* j . */
+	EMIT4_PCREL(0xa7f40000, 0);
+}
+
+/*
+ * Emit __s390_indirect_jump_r1 thunk if necessary
+ */
+static void emit_r1_thunk(struct bpf_jit *jit)
+{
+	if (nospec_uses_trampoline()) {
+		jit->r1_thunk_ip = jit->prg;
+		emit_expoline(jit);
+		/* br %r1 */
+		_EMIT2(0x07f1);
+	}
+}
+
+/*
+ * Call r1 either directly or via __s390_indirect_jump_r1 thunk
+ */
+static void call_r1(struct bpf_jit *jit)
+{
+	if (nospec_uses_trampoline())
+		/* brasl %r14,__s390_indirect_jump_r1 */
+		EMIT6_PCREL_RILB(0xc0050000, REG_14, jit->r1_thunk_ip);
+	else
+		/* basr %r14,%r1 */
+		EMIT2(0x0d00, REG_14, REG_1);
+}
+
 /*
  * Function epilogue
  */
@@ -608,25 +649,13 @@ static void bpf_jit_epilogue(struct bpf_jit *jit, u32 stack_depth)
 	if (nospec_uses_trampoline()) {
 		jit->r14_thunk_ip = jit->prg;
 		/* Generate __s390_indirect_jump_r14 thunk */
-		/* exrl %r0,.+10 */
-		EMIT6_PCREL_RIL(0xc6000000, jit->prg + 10);
-		/* j . */
-		EMIT4_PCREL(0xa7f40000, 0);
+		emit_expoline(jit);
 	}
 	/* br %r14 */
 	_EMIT2(0x07fe);
 
-	if ((nospec_uses_trampoline()) &&
-	    (is_first_pass(jit) || (jit->seen & SEEN_FUNC))) {
-		jit->r1_thunk_ip = jit->prg;
-		/* Generate __s390_indirect_jump_r1 thunk */
-		/* exrl %r0,.+10 */
-		EMIT6_PCREL_RIL(0xc6000000, jit->prg + 10);
-		/* j . */
-		EMIT4_PCREL(0xa7f40000, 0);
-		/* br %r1 */
-		_EMIT2(0x07f1);
-	}
+	if (is_first_pass(jit) || (jit->seen & SEEN_FUNC))
+		emit_r1_thunk(jit);
 
 	jit->prg = ALIGN(jit->prg, 8);
 	jit->prologue_plt = jit->prg;
@@ -707,6 +736,34 @@ static int bpf_jit_probe_mem(struct bpf_jit *jit, struct bpf_prog *fp,
 	return 0;
 }
 
+/*
+ * Sign-extend the register if necessary
+ */
+static int sign_extend(struct bpf_jit *jit, int r, u8 size, u8 flags)
+{
+	if (!(flags & BTF_FMODEL_SIGNED_ARG))
+		return 0;
+
+	switch (size) {
+	case 1:
+		/* lgbr %r,%r */
+		EMIT4(0xb9060000, r, r);
+		return 0;
+	case 2:
+		/* lghr %r,%r */
+		EMIT4(0xb9070000, r, r);
+		return 0;
+	case 4:
+		/* lgfr %r,%r */
+		EMIT4(0xb9140000, r, r);
+		return 0;
+	case 8:
+		return 0;
+	default:
+		return -1;
+	}
+}
+
 /*
  * Compile one eBPF instruction into s390x code
  *
@@ -1355,13 +1412,8 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 		jit->seen |= SEEN_FUNC;
 		/* lgrl %w1,func */
 		EMIT6_PCREL_RILB(0xc4080000, REG_W1, _EMIT_CONST_U64(func));
-		if (nospec_uses_trampoline()) {
-			/* brasl %r14,__s390_indirect_jump_r1 */
-			EMIT6_PCREL_RILB(0xc0050000, REG_14, jit->r1_thunk_ip);
-		} else {
-			/* basr %r14,%w1 */
-			EMIT2(0x0d00, REG_14, REG_W1);
-		}
+		/* %r1() */
+		call_r1(jit);
 		/* lgr %b0,%r2: load return value into %b0 */
 		EMIT4(0xb9040000, BPF_REG_0, REG_2);
 		break;
@@ -1964,3 +2016,442 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
 
 	return 0;
 }
+
+struct bpf_tramp_jit {
+	struct bpf_jit common;
+	int orig_stack_args_off;/* Offset of arguments placed on stack by the
+				 * func_addr's original caller
+				 */
+	int stack_size;		/* Trampoline stack size */
+	int stack_args_off;	/* Offset of stack arguments for calling
+				 * func_addr, has to be at the top
+				 */
+	int reg_args_off;	/* Offset of register arguments for calling
+				 * func_addr
+				 */
+	int ip_off;		/* For bpf_get_func_ip(), has to be at
+				 * (ctx - 16)
+				 */
+	int arg_cnt_off;	/* For bpf_get_func_arg_cnt(), has to be at
+				 * (ctx - 8)
+				 */
+	int bpf_args_off;	/* Offset of BPF_PROG context, which consists
+				 * of BPF arguments followed by return value
+				 */
+	int retval_off;		/* Offset of return value (see above) */
+	int r7_r8_off;		/* Offset of saved %r7 and %r8, which are used
+				 * for __bpf_prog_enter() return value and
+				 * func_addr respectively
+				 */
+	int r14_off;		/* Offset of saved %r14 */
+	int run_ctx_off;	/* Offset of struct bpf_tramp_run_ctx */
+	int do_fexit;		/* do_fexit: label */
+};
+
+static void load_imm64(struct bpf_jit *jit, int dst_reg, u64 val)
+{
+	/* llihf %dst_reg,val_hi */
+	EMIT6_IMM(0xc00e0000, dst_reg, (val >> 32));
+	/* oilf %rdst_reg,val_lo */
+	EMIT6_IMM(0xc00d0000, dst_reg, val);
+}
+
+static void invoke_bpf_prog(struct bpf_tramp_jit *tjit,
+			    const struct btf_func_model *m,
+			    struct bpf_tramp_link *tlink, bool save_ret)
+{
+	struct bpf_jit *jit = &tjit->common;
+	int cookie_off = tjit->run_ctx_off +
+			 offsetof(struct bpf_tramp_run_ctx, bpf_cookie);
+	struct bpf_prog *p = tlink->link.prog;
+	int patch;
+
+	/*
+	 * run_ctx.cookie = tlink->cookie;
+	 */
+
+	/* %r0 = tlink->cookie */
+	load_imm64(jit, REG_W0, tlink->cookie);
+	/* stg %r0,cookie_off(%r15) */
+	EMIT6_DISP_LH(0xe3000000, 0x0024, REG_W0, REG_0, REG_15, cookie_off);
+
+	/*
+	 * if ((start = __bpf_prog_enter(p, &run_ctx)) == 0)
+	 *         goto skip;
+	 */
+
+	/* %r1 = __bpf_prog_enter */
+	load_imm64(jit, REG_1, (u64)bpf_trampoline_enter(p));
+	/* %r2 = p */
+	load_imm64(jit, REG_2, (u64)p);
+	/* la %r3,run_ctx_off(%r15) */
+	EMIT4_DISP(0x41000000, REG_3, REG_15, tjit->run_ctx_off);
+	/* %r1() */
+	call_r1(jit);
+	/* ltgr %r7,%r2 */
+	EMIT4(0xb9020000, REG_7, REG_2);
+	/* brcl 8,skip */
+	patch = jit->prg;
+	EMIT6_PCREL_RILC(0xc0040000, 8, 0);
+
+	/*
+	 * retval = bpf_func(args, p->insnsi);
+	 */
+
+	/* %r1 = p->bpf_func */
+	load_imm64(jit, REG_1, (u64)p->bpf_func);
+	/* la %r2,bpf_args_off(%r15) */
+	EMIT4_DISP(0x41000000, REG_2, REG_15, tjit->bpf_args_off);
+	/* %r3 = p->insnsi */
+	if (!p->jited)
+		load_imm64(jit, REG_3, (u64)p->insnsi);
+	/* %r1() */
+	call_r1(jit);
+	/* stg %r2,retval_off(%r15) */
+	if (save_ret) {
+		sign_extend(jit, REG_2, m->ret_size, m->ret_flags);
+		EMIT6_DISP_LH(0xe3000000, 0x0024, REG_2, REG_0, REG_15,
+			      tjit->retval_off);
+	}
+
+	/* skip: */
+	if (jit->prg_buf)
+		*(u32 *)&jit->prg_buf[patch + 2] = (jit->prg - patch) >> 1;
+
+	/*
+	 * __bpf_prog_exit(p, start, &run_ctx);
+	 */
+
+	/* %r1 = __bpf_prog_exit */
+	load_imm64(jit, REG_1, (u64)bpf_trampoline_exit(p));
+	/* %r2 = p */
+	load_imm64(jit, REG_2, (u64)p);
+	/* lgr %r3,%r7 */
+	EMIT4(0xb9040000, REG_3, REG_7);
+	/* la %r4,run_ctx_off(%r15) */
+	EMIT4_DISP(0x41000000, REG_4, REG_15, tjit->run_ctx_off);
+	/* %r1() */
+	call_r1(jit);
+}
+
+static int alloc_stack(struct bpf_tramp_jit *tjit, size_t size)
+{
+	int stack_offset = tjit->stack_size;
+
+	tjit->stack_size += size;
+	return stack_offset;
+}
+
+/* ABI uses %r2 - %r6 for parameter passing. */
+#define MAX_NR_REG_ARGS 5
+
+/* The "L" field of the "mvc" instruction is 8 bits. */
+#define MAX_MVC_SIZE 256
+#define MAX_NR_STACK_ARGS (MAX_MVC_SIZE / sizeof(u64))
+
+/* -mfentry generates a 6-byte nop on s390x. */
+#define S390X_PATCH_SIZE 6
+
+int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
+				  struct bpf_tramp_jit *tjit,
+				  const struct btf_func_model *m,
+				  u32 flags, struct bpf_tramp_links *tlinks,
+				  void *func_addr)
+{
+	struct bpf_tramp_links *fmod_ret = &tlinks[BPF_TRAMP_MODIFY_RETURN];
+	struct bpf_tramp_links *fentry = &tlinks[BPF_TRAMP_FENTRY];
+	struct bpf_tramp_links *fexit = &tlinks[BPF_TRAMP_FEXIT];
+	int nr_bpf_args, nr_reg_args, nr_stack_args;
+	struct bpf_jit *jit = &tjit->common;
+	int arg, bpf_arg_off;
+	int i, j;
+
+	/* Support as many stack arguments as "mvc" instruction can handle. */
+	nr_reg_args = min_t(int, m->nr_args, MAX_NR_REG_ARGS);
+	nr_stack_args = m->nr_args - nr_reg_args;
+	if (nr_stack_args > MAX_NR_STACK_ARGS)
+		return -ENOTSUPP;
+
+	/* Return to %r14, since func_addr and %r0 are not available. */
+	if (!func_addr && !(flags & BPF_TRAMP_F_ORIG_STACK))
+		flags |= BPF_TRAMP_F_SKIP_FRAME;
+
+	/*
+	 * Compute how many arguments we need to pass to BPF programs.
+	 * BPF ABI mirrors that of x86_64: arguments that are 16 bytes or
+	 * smaller are packed into 1 or 2 registers; larger arguments are
+	 * passed via pointers.
+	 * In s390x ABI, arguments that are 8 bytes or smaller are packed into
+	 * a register; larger arguments are passed via pointers.
+	 * We need to deal with this difference.
+	 */
+	nr_bpf_args = 0;
+	for (i = 0; i < m->nr_args; i++) {
+		if (m->arg_size[i] <= 8)
+			nr_bpf_args += 1;
+		else if (m->arg_size[i] <= 16)
+			nr_bpf_args += 2;
+		else
+			return -ENOTSUPP;
+	}
+
+	/*
+	 * Calculate the stack layout.
+	 */
+
+	/* Reserve STACK_FRAME_OVERHEAD bytes for the callees. */
+	tjit->stack_size = STACK_FRAME_OVERHEAD;
+	tjit->stack_args_off = alloc_stack(tjit, nr_stack_args * sizeof(u64));
+	tjit->reg_args_off = alloc_stack(tjit, nr_reg_args * sizeof(u64));
+	tjit->ip_off = alloc_stack(tjit, sizeof(u64));
+	tjit->arg_cnt_off = alloc_stack(tjit, sizeof(u64));
+	tjit->bpf_args_off = alloc_stack(tjit, nr_bpf_args * sizeof(u64));
+	tjit->retval_off = alloc_stack(tjit, sizeof(u64));
+	tjit->r7_r8_off = alloc_stack(tjit, 2 * sizeof(u64));
+	tjit->r14_off = alloc_stack(tjit, sizeof(u64));
+	tjit->run_ctx_off = alloc_stack(tjit,
+					sizeof(struct bpf_tramp_run_ctx));
+	/* The caller has already reserved STACK_FRAME_OVERHEAD bytes. */
+	tjit->stack_size -= STACK_FRAME_OVERHEAD;
+	tjit->orig_stack_args_off = tjit->stack_size + STACK_FRAME_OVERHEAD;
+
+	/* aghi %r15,-stack_size */
+	EMIT4_IMM(0xa70b0000, REG_15, -tjit->stack_size);
+	/* stmg %r2,%rN,fwd_reg_args_off(%r15) */
+	if (nr_reg_args)
+		EMIT6_DISP_LH(0xeb000000, 0x0024, REG_2,
+			      REG_2 + (nr_reg_args - 1), REG_15,
+			      tjit->reg_args_off);
+	for (i = 0, j = 0; i < m->nr_args; i++) {
+		if (i < MAX_NR_REG_ARGS)
+			arg = REG_2 + i;
+		else
+			arg = tjit->orig_stack_args_off +
+			      (i - MAX_NR_REG_ARGS) * sizeof(u64);
+		bpf_arg_off = tjit->bpf_args_off + j * sizeof(u64);
+		if (m->arg_size[i] <= 8) {
+			if (i < MAX_NR_REG_ARGS)
+				/* stg %arg,bpf_arg_off(%r15) */
+				EMIT6_DISP_LH(0xe3000000, 0x0024, arg,
+					      REG_0, REG_15, bpf_arg_off);
+			else
+				/* mvc bpf_arg_off(8,%r15),arg(%r15) */
+				_EMIT6(0xd207f000 | bpf_arg_off,
+				       0xf000 | arg);
+			j += 1;
+		} else {
+			if (i < MAX_NR_REG_ARGS) {
+				/* mvc bpf_arg_off(16,%r15),0(%arg) */
+				_EMIT6(0xd20ff000 | bpf_arg_off,
+				       reg2hex[arg] << 12);
+			} else {
+				/* lg %r1,arg(%r15) */
+				EMIT6_DISP_LH(0xe3000000, 0x0004, REG_1, REG_0,
+					      REG_15, arg);
+				/* mvc bpf_arg_off(16,%r15),0(%r1) */
+				_EMIT6(0xd20ff000 | bpf_arg_off, 0x1000);
+			}
+			j += 2;
+		}
+	}
+	/* stmg %r7,%r8,r7_r8_off(%r15) */
+	EMIT6_DISP_LH(0xeb000000, 0x0024, REG_7, REG_8, REG_15,
+		      tjit->r7_r8_off);
+	/* stg %r14,r14_off(%r15) */
+	EMIT6_DISP_LH(0xe3000000, 0x0024, REG_14, REG_0, REG_15, tjit->r14_off);
+
+	if (flags & BPF_TRAMP_F_ORIG_STACK) {
+		/*
+		 * The ftrace trampoline puts the return address (which is the
+		 * address of the original function + S390X_PATCH_SIZE) into
+		 * %r0; see ftrace_shared_hotpatch_trampoline_br and
+		 * ftrace_init_nop() for details.
+		 */
+
+		/* lgr %r8,%r0 */
+		EMIT4(0xb9040000, REG_8, REG_0);
+	} else {
+		/* %r8 = func_addr + S390X_PATCH_SIZE */
+		load_imm64(jit, REG_8, (u64)func_addr + S390X_PATCH_SIZE);
+	}
+
+	/*
+	 * ip = func_addr;
+	 * arg_cnt = m->nr_args;
+	 */
+
+	if (flags & BPF_TRAMP_F_IP_ARG) {
+		/* %r0 = func_addr */
+		load_imm64(jit, REG_0, (u64)func_addr);
+		/* stg %r0,ip_off(%r15) */
+		EMIT6_DISP_LH(0xe3000000, 0x0024, REG_0, REG_0, REG_15,
+			      tjit->ip_off);
+	}
+	/* lghi %r0,nr_bpf_args */
+	EMIT4_IMM(0xa7090000, REG_0, nr_bpf_args);
+	/* stg %r0,arg_cnt_off(%r15) */
+	EMIT6_DISP_LH(0xe3000000, 0x0024, REG_0, REG_0, REG_15,
+		      tjit->arg_cnt_off);
+
+	if (flags & BPF_TRAMP_F_CALL_ORIG) {
+		/*
+		 * __bpf_tramp_enter(im);
+		 */
+
+		/* %r1 = __bpf_tramp_enter */
+		load_imm64(jit, REG_1, (u64)__bpf_tramp_enter);
+		/* %r2 = im */
+		load_imm64(jit, REG_2, (u64)im);
+		/* %r1() */
+		call_r1(jit);
+	}
+
+	for (i = 0; i < fentry->nr_links; i++)
+		invoke_bpf_prog(tjit, m, fentry->links[i],
+				flags & BPF_TRAMP_F_RET_FENTRY_RET);
+
+	if (fmod_ret->nr_links) {
+		/*
+		 * retval = 0;
+		 */
+
+		/* xc retval_off(8,%r15),retval_off(%r15) */
+		_EMIT6(0xd707f000 | tjit->retval_off,
+		       0xf000 | tjit->retval_off);
+
+		for (i = 0; i < fmod_ret->nr_links; i++) {
+			invoke_bpf_prog(tjit, m, fmod_ret->links[i], true);
+
+			/*
+			 * if (retval)
+			 *         goto do_fexit;
+			 */
+
+			/* ltg %r0,retval_off(%r15) */
+			EMIT6_DISP_LH(0xe3000000, 0x0002, REG_0, REG_0, REG_15,
+				      tjit->retval_off);
+			/* brcl 7,do_fexit */
+			EMIT6_PCREL_RILC(0xc0040000, 7, tjit->do_fexit);
+		}
+	}
+
+	if (flags & BPF_TRAMP_F_CALL_ORIG) {
+		/*
+		 * retval = func_addr(args);
+		 */
+
+		/* lmg %r2,%rN,reg_args_off(%r15) */
+		if (nr_reg_args)
+			EMIT6_DISP_LH(0xeb000000, 0x0004, REG_2,
+				      REG_2 + (nr_reg_args - 1), REG_15,
+				      tjit->reg_args_off);
+		/* mvc stack_args_off(N,%r15),orig_stack_args_off(%r15) */
+		if (nr_stack_args)
+			_EMIT6(0xd200f000 |
+				       (nr_stack_args * sizeof(u64) - 1) << 16 |
+				       tjit->stack_args_off,
+			       0xf000 | tjit->orig_stack_args_off);
+		/* lgr %r1,%r8 */
+		EMIT4(0xb9040000, REG_1, REG_8);
+		/* %r1() */
+		call_r1(jit);
+		/* stg %r2,retval_off(%r15) */
+		EMIT6_DISP_LH(0xe3000000, 0x0024, REG_2, REG_0, REG_15,
+			      tjit->retval_off);
+
+		im->ip_after_call = jit->prg_buf + jit->prg;
+
+		/*
+		 * The following nop will be patched by bpf_tramp_image_put().
+		 */
+
+		/* brcl 0,im->ip_epilogue */
+		EMIT6_PCREL_RILC(0xc0040000, 0, (u64)im->ip_epilogue);
+	}
+
+	/* do_fexit: */
+	tjit->do_fexit = jit->prg;
+	for (i = 0; i < fexit->nr_links; i++)
+		invoke_bpf_prog(tjit, m, fexit->links[i], false);
+
+	if (flags & BPF_TRAMP_F_CALL_ORIG) {
+		im->ip_epilogue = jit->prg_buf + jit->prg;
+
+		/*
+		 * __bpf_tramp_exit(im);
+		 */
+
+		/* %r1 = __bpf_tramp_exit */
+		load_imm64(jit, REG_1, (u64)__bpf_tramp_exit);
+		/* %r2 = im */
+		load_imm64(jit, REG_2, (u64)im);
+		/* %r1() */
+		call_r1(jit);
+	}
+
+	/* lmg %r2,%rN,reg_args_off(%r15) */
+	if ((flags & BPF_TRAMP_F_RESTORE_REGS) && nr_reg_args)
+		EMIT6_DISP_LH(0xeb000000, 0x0004, REG_2,
+			      REG_2 + (nr_reg_args - 1), REG_15,
+			      tjit->reg_args_off);
+	/* lgr %r1,%r8 */
+	if (!(flags & BPF_TRAMP_F_SKIP_FRAME))
+		EMIT4(0xb9040000, REG_1, REG_8);
+	/* lmg %r7,%r8,r7_r8_off(%r15) */
+	EMIT6_DISP_LH(0xeb000000, 0x0004, REG_7, REG_8, REG_15,
+		      tjit->r7_r8_off);
+	/* lg %r14,r14_off(%r15) */
+	EMIT6_DISP_LH(0xe3000000, 0x0004, REG_14, REG_0, REG_15, tjit->r14_off);
+	/* lg %r2,retval_off(%r15) */
+	if (flags & (BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_RET_FENTRY_RET))
+		EMIT6_DISP_LH(0xe3000000, 0x0004, REG_2, REG_0, REG_15,
+			      tjit->retval_off);
+	/* aghi %r15,stack_size */
+	EMIT4_IMM(0xa70b0000, REG_15, tjit->stack_size);
+	/* Emit an expoline for the following indirect jump. */
+	if (nospec_uses_trampoline())
+		emit_expoline(jit);
+	if (flags & BPF_TRAMP_F_SKIP_FRAME)
+		/* br %r14 */
+		_EMIT2(0x07fe);
+	else
+		/* br %r1 */
+		_EMIT2(0x07f1);
+
+	emit_r1_thunk(jit);
+
+	return 0;
+}
+
+int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
+				void *image_end, const struct btf_func_model *m,
+				u32 flags, struct bpf_tramp_links *tlinks,
+				void *func_addr)
+{
+	struct bpf_tramp_jit tjit;
+	int ret;
+	int i;
+
+	for (i = 0; i < 2; i++) {
+		if (i == 0) {
+			/* Compute offsets, check whether the code fits. */
+			memset(&tjit, 0, sizeof(tjit));
+		} else {
+			/* Generate the code. */
+			tjit.common.prg = 0;
+			tjit.common.prg_buf = image;
+		}
+		ret = __arch_prepare_bpf_trampoline(im, &tjit, m, flags,
+						    tlinks, func_addr);
+		if (ret < 0)
+			return ret;
+		if (tjit.common.prg > (char *)image_end - (char *)image)
+			/*
+			 * Use the same error code as for exceeding
+			 * BPF_MAX_TRAMP_LINKS.
+			 */
+			return -E2BIG;
+	}
+
+	return ret;
+}
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index cf89504c8dda..52ff43bbf996 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -943,7 +943,11 @@ struct btf_func_model {
 /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
  * bytes on x86.
  */
+#if defined(__s390x__)
+#define BPF_MAX_TRAMP_LINKS 27
+#else
 #define BPF_MAX_TRAMP_LINKS 38
+#endif
 
 struct bpf_tramp_links {
 	struct bpf_tramp_link *links[BPF_MAX_TRAMP_LINKS];
-- 
2.39.1

Re: [PATCH bpf-next 22/24] s390/bpf: Implement arch_prepare_bpf_trampoline()

[Andrii Nakryiko]

On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> arch_prepare_bpf_trampoline() is used for direct attachment of eBPF
> programs to various places, bypassing kprobes. It's responsible for
> calling a number of eBPF programs before, instead and/or after
> whatever they are attached to.
>
> Add a s390x implementation, paying attention to the following:
>
> - Reuse the existing JIT infrastructure, where possible.
> - Like the existing JIT, prefer making multiple passes instead of
>   backpatching. Currently 2 passes is enough. If literal pool is
>   introduced, this needs to be raised to 3. However, at the moment
>   adding literal pool only makes the code larger. If branch
>   shortening is introduced, the number of passes needs to be
>   increased even further.
> - Support both regular and ftrace calling conventions, depending on
>   the trampoline flags.
> - Use expolines for indirect calls.
> - Handle the mismatch between the eBPF and the s390x ABIs.
> - Sign-extend fmod_ret return values.
>
> invoke_bpf_prog() produces about 120 bytes; it might be possible to
> slightly optimize this, but reaching 50 bytes, like on x86_64, looks
> unrealistic: just loading cookie, __bpf_prog_enter, bpf_func, insnsi
> and __bpf_prog_exit as literals already takes at least 5 * 12 = 60
> bytes, and we can't use relative addressing for most of them.
> Therefore, lower BPF_MAX_TRAMP_LINKS on s390x.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  arch/s390/net/bpf_jit_comp.c | 535 +++++++++++++++++++++++++++++++++--
>  include/linux/bpf.h          |   4 +
>  2 files changed, 517 insertions(+), 22 deletions(-)
>

[...]

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index cf89504c8dda..52ff43bbf996 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -943,7 +943,11 @@ struct btf_func_model {
>  /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
>   * bytes on x86.
>   */
> +#if defined(__s390x__)
> +#define BPF_MAX_TRAMP_LINKS 27
> +#else
>  #define BPF_MAX_TRAMP_LINKS 38
> +#endif

if we turn this into enum definition, then on selftests side we can
just discover this from vmlinux BTF, instead of hard-coding
arch-specific constants. Thoughts?

>
>  struct bpf_tramp_links {
>         struct bpf_tramp_link *links[BPF_MAX_TRAMP_LINKS];
> --
> 2.39.1
>

Re: [PATCH bpf-next 22/24] s390/bpf: Implement arch_prepare_bpf_trampoline()

[Ilya Leoshkevich]

On Wed, 2023-01-25 at 17:15 -0800, Andrii Nakryiko wrote:
> On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > arch_prepare_bpf_trampoline() is used for direct attachment of eBPF
> > programs to various places, bypassing kprobes. It's responsible for
> > calling a number of eBPF programs before, instead and/or after
> > whatever they are attached to.
> > 
> > Add a s390x implementation, paying attention to the following:
> > 
> > - Reuse the existing JIT infrastructure, where possible.
> > - Like the existing JIT, prefer making multiple passes instead of
> >   backpatching. Currently 2 passes is enough. If literal pool is
> >   introduced, this needs to be raised to 3. However, at the moment
> >   adding literal pool only makes the code larger. If branch
> >   shortening is introduced, the number of passes needs to be
> >   increased even further.
> > - Support both regular and ftrace calling conventions, depending on
> >   the trampoline flags.
> > - Use expolines for indirect calls.
> > - Handle the mismatch between the eBPF and the s390x ABIs.
> > - Sign-extend fmod_ret return values.
> > 
> > invoke_bpf_prog() produces about 120 bytes; it might be possible to
> > slightly optimize this, but reaching 50 bytes, like on x86_64,
> > looks
> > unrealistic: just loading cookie, __bpf_prog_enter, bpf_func,
> > insnsi
> > and __bpf_prog_exit as literals already takes at least 5 * 12 = 60
> > bytes, and we can't use relative addressing for most of them.
> > Therefore, lower BPF_MAX_TRAMP_LINKS on s390x.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  arch/s390/net/bpf_jit_comp.c | 535
> > +++++++++++++++++++++++++++++++++--
> >  include/linux/bpf.h          |   4 +
> >  2 files changed, 517 insertions(+), 22 deletions(-)
> > 
> 
> [...]
> 
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index cf89504c8dda..52ff43bbf996 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -943,7 +943,11 @@ struct btf_func_model {
> >  /* Each call __bpf_prog_enter + call bpf_func + call
> > __bpf_prog_exit is ~50
> >   * bytes on x86.
> >   */
> > +#if defined(__s390x__)
> > +#define BPF_MAX_TRAMP_LINKS 27
> > +#else
> >  #define BPF_MAX_TRAMP_LINKS 38
> > +#endif
> 
> if we turn this into enum definition, then on selftests side we can
> just discover this from vmlinux BTF, instead of hard-coding
> arch-specific constants. Thoughts?

This seems to work. I can replace 3/24 and 4/24 with that in v2.
Some random notes:

- It doesn't seem to be possible to #include "vlinux.h" into tests,
  so one has to go through the btf__load_vmlinux_btf() dance and
  allocate the fd arrays dynamically.

- One has to give this enum an otherwise unnecessary name, so that
  it's easy to find. This doesn't seem like a big deal though:

enum bpf_max_tramp_links {
#if defined(__s390x__)
	BPF_MAX_TRAMP_LINKS = 27,
#else
	BPF_MAX_TRAMP_LINKS = 38,
#endif
};

- An alternative might be to expose this via /proc, since the users
  might be interested in it too.

> > 
> >  struct bpf_tramp_links {
> >         struct bpf_tramp_link *links[BPF_MAX_TRAMP_LINKS];
> > --
> > 2.39.1
> > 

Re: [PATCH bpf-next 22/24] s390/bpf: Implement arch_prepare_bpf_trampoline()

[Andrii Nakryiko]

On Thu, Jan 26, 2023 at 6:30 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Wed, 2023-01-25 at 17:15 -0800, Andrii Nakryiko wrote:
> > On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com>
> > wrote:
> > >
> > > arch_prepare_bpf_trampoline() is used for direct attachment of eBPF
> > > programs to various places, bypassing kprobes. It's responsible for
> > > calling a number of eBPF programs before, instead and/or after
> > > whatever they are attached to.
> > >
> > > Add a s390x implementation, paying attention to the following:
> > >
> > > - Reuse the existing JIT infrastructure, where possible.
> > > - Like the existing JIT, prefer making multiple passes instead of
> > >   backpatching. Currently 2 passes is enough. If literal pool is
> > >   introduced, this needs to be raised to 3. However, at the moment
> > >   adding literal pool only makes the code larger. If branch
> > >   shortening is introduced, the number of passes needs to be
> > >   increased even further.
> > > - Support both regular and ftrace calling conventions, depending on
> > >   the trampoline flags.
> > > - Use expolines for indirect calls.
> > > - Handle the mismatch between the eBPF and the s390x ABIs.
> > > - Sign-extend fmod_ret return values.
> > >
> > > invoke_bpf_prog() produces about 120 bytes; it might be possible to
> > > slightly optimize this, but reaching 50 bytes, like on x86_64,
> > > looks
> > > unrealistic: just loading cookie, __bpf_prog_enter, bpf_func,
> > > insnsi
> > > and __bpf_prog_exit as literals already takes at least 5 * 12 = 60
> > > bytes, and we can't use relative addressing for most of them.
> > > Therefore, lower BPF_MAX_TRAMP_LINKS on s390x.
> > >
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >  arch/s390/net/bpf_jit_comp.c | 535
> > > +++++++++++++++++++++++++++++++++--
> > >  include/linux/bpf.h          |   4 +
> > >  2 files changed, 517 insertions(+), 22 deletions(-)
> > >
> >
> > [...]
> >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index cf89504c8dda..52ff43bbf996 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -943,7 +943,11 @@ struct btf_func_model {
> > >  /* Each call __bpf_prog_enter + call bpf_func + call
> > > __bpf_prog_exit is ~50
> > >   * bytes on x86.
> > >   */
> > > +#if defined(__s390x__)
> > > +#define BPF_MAX_TRAMP_LINKS 27
> > > +#else
> > >  #define BPF_MAX_TRAMP_LINKS 38
> > > +#endif
> >
> > if we turn this into enum definition, then on selftests side we can
> > just discover this from vmlinux BTF, instead of hard-coding
> > arch-specific constants. Thoughts?
>
> This seems to work. I can replace 3/24 and 4/24 with that in v2.
> Some random notes:
>
> - It doesn't seem to be possible to #include "vlinux.h" into tests,
>   so one has to go through the btf__load_vmlinux_btf() dance and
>   allocate the fd arrays dynamically.

yes, you can't include vmlinux.h into user-space code, of course. And
yes it's true about needing to use btf__load_vmlinux_btf().

But I didn't get what you are saying about fd arrays, tbh. Can you
please elaborate?

>
> - One has to give this enum an otherwise unnecessary name, so that
>   it's easy to find. This doesn't seem like a big deal though:
>
> enum bpf_max_tramp_links {

not really, you can keep it anonymous enum. We do that in
include/uapi/linux/bpf.h for a lot of constants

> #if defined(__s390x__)
>         BPF_MAX_TRAMP_LINKS = 27,
> #else
>         BPF_MAX_TRAMP_LINKS = 38,
> #endif
> };
>
> - An alternative might be to expose this via /proc, since the users
>   might be interested in it too.

I'd say let's not, there is no need, having it in BTF is more than
enough for testing purposes

>
> > >
> > >  struct bpf_tramp_links {
> > >         struct bpf_tramp_link *links[BPF_MAX_TRAMP_LINKS];
> > > --
> > > 2.39.1
> > >
>

Re: [PATCH bpf-next 22/24] s390/bpf: Implement arch_prepare_bpf_trampoline()

[Ilya Leoshkevich]

On Thu, 2023-01-26 at 11:06 -0800, Andrii Nakryiko wrote:
> On Thu, Jan 26, 2023 at 6:30 AM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > On Wed, 2023-01-25 at 17:15 -0800, Andrii Nakryiko wrote:
> > > On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich
> > > <iii@linux.ibm.com>
> > > wrote:
> > > > 
> > > > arch_prepare_bpf_trampoline() is used for direct attachment of
> > > > eBPF
> > > > programs to various places, bypassing kprobes. It's responsible
> > > > for
> > > > calling a number of eBPF programs before, instead and/or after
> > > > whatever they are attached to.
> > > > 
> > > > Add a s390x implementation, paying attention to the following:
> > > > 
> > > > - Reuse the existing JIT infrastructure, where possible.
> > > > - Like the existing JIT, prefer making multiple passes instead
> > > > of
> > > >   backpatching. Currently 2 passes is enough. If literal pool
> > > > is
> > > >   introduced, this needs to be raised to 3. However, at the
> > > > moment
> > > >   adding literal pool only makes the code larger. If branch
> > > >   shortening is introduced, the number of passes needs to be
> > > >   increased even further.
> > > > - Support both regular and ftrace calling conventions,
> > > > depending on
> > > >   the trampoline flags.
> > > > - Use expolines for indirect calls.
> > > > - Handle the mismatch between the eBPF and the s390x ABIs.
> > > > - Sign-extend fmod_ret return values.
> > > > 
> > > > invoke_bpf_prog() produces about 120 bytes; it might be
> > > > possible to
> > > > slightly optimize this, but reaching 50 bytes, like on x86_64,
> > > > looks
> > > > unrealistic: just loading cookie, __bpf_prog_enter, bpf_func,
> > > > insnsi
> > > > and __bpf_prog_exit as literals already takes at least 5 * 12 =
> > > > 60
> > > > bytes, and we can't use relative addressing for most of them.
> > > > Therefore, lower BPF_MAX_TRAMP_LINKS on s390x.
> > > > 
> > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > ---
> > > >  arch/s390/net/bpf_jit_comp.c | 535
> > > > +++++++++++++++++++++++++++++++++--
> > > >  include/linux/bpf.h          |   4 +
> > > >  2 files changed, 517 insertions(+), 22 deletions(-)
> > > > 
> > > 
> > > [...]
> > > 
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index cf89504c8dda..52ff43bbf996 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -943,7 +943,11 @@ struct btf_func_model {
> > > >  /* Each call __bpf_prog_enter + call bpf_func + call
> > > > __bpf_prog_exit is ~50
> > > >   * bytes on x86.
> > > >   */
> > > > +#if defined(__s390x__)
> > > > +#define BPF_MAX_TRAMP_LINKS 27
> > > > +#else
> > > >  #define BPF_MAX_TRAMP_LINKS 38
> > > > +#endif
> > > 
> > > if we turn this into enum definition, then on selftests side we
> > > can
> > > just discover this from vmlinux BTF, instead of hard-coding
> > > arch-specific constants. Thoughts?
> > 
> > This seems to work. I can replace 3/24 and 4/24 with that in v2.
> > Some random notes:
> > 
> > - It doesn't seem to be possible to #include "vlinux.h" into tests,
> >   so one has to go through the btf__load_vmlinux_btf() dance and
> >   allocate the fd arrays dynamically.
> 
> yes, you can't include vmlinux.h into user-space code, of course. And
> yes it's true about needing to use btf__load_vmlinux_btf().
> 
> But I didn't get what you are saying about fd arrays, tbh. Can you
> please elaborate?

That's a really minor thing; fexit_fd and and link_fd in fexit_stress
now need to be allocated dynamically.

> > - One has to give this enum an otherwise unnecessary name, so that
> >   it's easy to find. This doesn't seem like a big deal though:
> > 
> > enum bpf_max_tramp_links {
> 
> not really, you can keep it anonymous enum. We do that in
> include/uapi/linux/bpf.h for a lot of constants

How would you find it then? My current code is:

int get_bpf_max_tramp_links_from(struct btf *btf)
{
        const struct btf_enum *e;
        const struct btf_type *t;
        const char *name;
        int id;

        id = btf__find_by_name_kind(btf, "bpf_max_tramp_links",
BTF_KIND_ENUM);
        if (!ASSERT_GT(id, 0, "bpf_max_tramp_links id"))
                return -1;
        t = btf__type_by_id(btf, id);
        if (!ASSERT_OK_PTR(t, "bpf_max_tramp_links type"))
                return -1;
        if (!ASSERT_EQ(btf_vlen(t), 1, "bpf_max_tramp_links vlen"))
                return -1;
        e = btf_enum(t);
        if (!ASSERT_OK_PTR(e, "bpf_max_tramp_links[0]"))
                return -1;
        name = btf__name_by_offset(btf, e->name_off);
        if (!ASSERT_OK_PTR(name, "bpf_max_tramp_links[0].name_off") &&
            !ASSERT_STREQ(name, "BPF_MAX_TRAMP_LINKS",
"BPF_MAX_TRAMP_LINKS"))
                return -1;

        return e->val;
}

Is there a way to bypass looking up the enum, and go straight for the
named member?

> > #if defined(__s390x__)
> >         BPF_MAX_TRAMP_LINKS = 27,
> > #else
> >         BPF_MAX_TRAMP_LINKS = 38,
> > #endif
> > };
> > 
> > - An alternative might be to expose this via /proc, since the users
> >   might be interested in it too.
> 
> I'd say let's not, there is no need, having it in BTF is more than
> enough for testing purposes

Fair enough.
> 

Re: [PATCH bpf-next 22/24] s390/bpf: Implement arch_prepare_bpf_trampoline()

[Andrii Nakryiko]

On Fri, Jan 27, 2023 at 3:15 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Thu, 2023-01-26 at 11:06 -0800, Andrii Nakryiko wrote:
> > On Thu, Jan 26, 2023 at 6:30 AM Ilya Leoshkevich <iii@linux.ibm.com>
> > wrote:
> > >
> > > On Wed, 2023-01-25 at 17:15 -0800, Andrii Nakryiko wrote:
> > > > On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich
> > > > <iii@linux.ibm.com>
> > > > wrote:
> > > > >
> > > > > arch_prepare_bpf_trampoline() is used for direct attachment of
> > > > > eBPF
> > > > > programs to various places, bypassing kprobes. It's responsible
> > > > > for
> > > > > calling a number of eBPF programs before, instead and/or after
> > > > > whatever they are attached to.
> > > > >
> > > > > Add a s390x implementation, paying attention to the following:
> > > > >
> > > > > - Reuse the existing JIT infrastructure, where possible.
> > > > > - Like the existing JIT, prefer making multiple passes instead
> > > > > of
> > > > >   backpatching. Currently 2 passes is enough. If literal pool
> > > > > is
> > > > >   introduced, this needs to be raised to 3. However, at the
> > > > > moment
> > > > >   adding literal pool only makes the code larger. If branch
> > > > >   shortening is introduced, the number of passes needs to be
> > > > >   increased even further.
> > > > > - Support both regular and ftrace calling conventions,
> > > > > depending on
> > > > >   the trampoline flags.
> > > > > - Use expolines for indirect calls.
> > > > > - Handle the mismatch between the eBPF and the s390x ABIs.
> > > > > - Sign-extend fmod_ret return values.
> > > > >
> > > > > invoke_bpf_prog() produces about 120 bytes; it might be
> > > > > possible to
> > > > > slightly optimize this, but reaching 50 bytes, like on x86_64,
> > > > > looks
> > > > > unrealistic: just loading cookie, __bpf_prog_enter, bpf_func,
> > > > > insnsi
> > > > > and __bpf_prog_exit as literals already takes at least 5 * 12 =
> > > > > 60
> > > > > bytes, and we can't use relative addressing for most of them.
> > > > > Therefore, lower BPF_MAX_TRAMP_LINKS on s390x.
> > > > >
> > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > > ---
> > > > >  arch/s390/net/bpf_jit_comp.c | 535
> > > > > +++++++++++++++++++++++++++++++++--
> > > > >  include/linux/bpf.h          |   4 +
> > > > >  2 files changed, 517 insertions(+), 22 deletions(-)
> > > > >
> > > >
> > > > [...]
> > > >
> > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > > index cf89504c8dda..52ff43bbf996 100644
> > > > > --- a/include/linux/bpf.h
> > > > > +++ b/include/linux/bpf.h
> > > > > @@ -943,7 +943,11 @@ struct btf_func_model {
> > > > >  /* Each call __bpf_prog_enter + call bpf_func + call
> > > > > __bpf_prog_exit is ~50
> > > > >   * bytes on x86.
> > > > >   */
> > > > > +#if defined(__s390x__)
> > > > > +#define BPF_MAX_TRAMP_LINKS 27
> > > > > +#else
> > > > >  #define BPF_MAX_TRAMP_LINKS 38
> > > > > +#endif
> > > >
> > > > if we turn this into enum definition, then on selftests side we
> > > > can
> > > > just discover this from vmlinux BTF, instead of hard-coding
> > > > arch-specific constants. Thoughts?
> > >
> > > This seems to work. I can replace 3/24 and 4/24 with that in v2.
> > > Some random notes:
> > >
> > > - It doesn't seem to be possible to #include "vlinux.h" into tests,
> > >   so one has to go through the btf__load_vmlinux_btf() dance and
> > >   allocate the fd arrays dynamically.
> >
> > yes, you can't include vmlinux.h into user-space code, of course. And
> > yes it's true about needing to use btf__load_vmlinux_btf().
> >
> > But I didn't get what you are saying about fd arrays, tbh. Can you
> > please elaborate?
>
> That's a really minor thing; fexit_fd and and link_fd in fexit_stress
> now need to be allocated dynamically.
>
> > > - One has to give this enum an otherwise unnecessary name, so that
> > >   it's easy to find. This doesn't seem like a big deal though:
> > >
> > > enum bpf_max_tramp_links {
> >
> > not really, you can keep it anonymous enum. We do that in
> > include/uapi/linux/bpf.h for a lot of constants
>
> How would you find it then? My current code is:
>
> int get_bpf_max_tramp_links_from(struct btf *btf)
> {
>         const struct btf_enum *e;
>         const struct btf_type *t;
>         const char *name;
>         int id;
>
>         id = btf__find_by_name_kind(btf, "bpf_max_tramp_links",
> BTF_KIND_ENUM);
>         if (!ASSERT_GT(id, 0, "bpf_max_tramp_links id"))
>                 return -1;
>         t = btf__type_by_id(btf, id);
>         if (!ASSERT_OK_PTR(t, "bpf_max_tramp_links type"))
>                 return -1;
>         if (!ASSERT_EQ(btf_vlen(t), 1, "bpf_max_tramp_links vlen"))
>                 return -1;
>         e = btf_enum(t);
>         if (!ASSERT_OK_PTR(e, "bpf_max_tramp_links[0]"))
>                 return -1;
>         name = btf__name_by_offset(btf, e->name_off);
>         if (!ASSERT_OK_PTR(name, "bpf_max_tramp_links[0].name_off") &&
>             !ASSERT_STREQ(name, "BPF_MAX_TRAMP_LINKS",
> "BPF_MAX_TRAMP_LINKS"))
>                 return -1;
>
>         return e->val;
> }
>
> Is there a way to bypass looking up the enum, and go straight for the
> named member?


don't use btf__find_by_name_kind, just iterate all types and look at
all anonymous enums and its values, roughly

for (i = 1; i < btf__type_cnt(btf); i++) {
    const btf_type *t = btf__type_by_id(i);
    if (!btf_is_enum(t) || t->name_off)
        continue;
    for (j = 0; j < btf_vlen(t); j++) {
        if (strcmp(btf__str_by_offset(btf, btf_enum(t)[j].name_off),
"BPF_MAX_TRAMP_LINKS") != 0)
            continue;
        /* found it */
    }
}

but cleaner :)


>
> > > #if defined(__s390x__)
> > >         BPF_MAX_TRAMP_LINKS = 27,
> > > #else
> > >         BPF_MAX_TRAMP_LINKS = 38,
> > > #endif
> > > };
> > >
> > > - An alternative might be to expose this via /proc, since the users
> > >   might be interested in it too.
> >
> > I'd say let's not, there is no need, having it in BTF is more than
> > enough for testing purposes
>
> Fair enough.
> >

Re: [PATCH bpf-next 22/24] s390/bpf: Implement arch_prepare_bpf_trampoline()

[kernel test robot]

Hi Ilya,

I love your patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Ilya-Leoshkevich/selftests-bpf-Fix-liburandom_read-so-linker-error/20230128-150321
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20230125213817.1424447-23-iii%40linux.ibm.com
patch subject: [PATCH bpf-next 22/24] s390/bpf: Implement arch_prepare_bpf_trampoline()
config: s390-defconfig (https://download.01.org/0day-ci/archive/20230215/202302151241.GpoGkTCm-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/6257ca590791a94b97f1b4f72851dc110290a52b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Ilya-Leoshkevich/selftests-bpf-Fix-liburandom_read-so-linker-error/20230128-150321
        git checkout 6257ca590791a94b97f1b4f72851dc110290a52b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash arch/s390/net/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302151241.GpoGkTCm-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> arch/s390/net/bpf_jit_comp.c:2155:5: warning: no previous prototype for '__arch_prepare_bpf_trampoline' [-Wmissing-prototypes]
    2155 | int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/__arch_prepare_bpf_trampoline +2155 arch/s390/net/bpf_jit_comp.c

  2154	
> 2155	int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
  2156					  struct bpf_tramp_jit *tjit,
  2157					  const struct btf_func_model *m,
  2158					  u32 flags, struct bpf_tramp_links *tlinks,
  2159					  void *func_addr)
  2160	{
  2161		struct bpf_tramp_links *fmod_ret = &tlinks[BPF_TRAMP_MODIFY_RETURN];
  2162		struct bpf_tramp_links *fentry = &tlinks[BPF_TRAMP_FENTRY];
  2163		struct bpf_tramp_links *fexit = &tlinks[BPF_TRAMP_FEXIT];
  2164		int nr_bpf_args, nr_reg_args, nr_stack_args;
  2165		struct bpf_jit *jit = &tjit->common;
  2166		int arg, bpf_arg_off;
  2167		int i, j;
  2168	
  2169		/* Support as many stack arguments as "mvc" instruction can handle. */
  2170		nr_reg_args = min_t(int, m->nr_args, MAX_NR_REG_ARGS);
  2171		nr_stack_args = m->nr_args - nr_reg_args;
  2172		if (nr_stack_args > MAX_NR_STACK_ARGS)
  2173			return -ENOTSUPP;
  2174	
  2175		/* Return to %r14, since func_addr and %r0 are not available. */
  2176		if (!func_addr && !(flags & BPF_TRAMP_F_ORIG_STACK))
  2177			flags |= BPF_TRAMP_F_SKIP_FRAME;
  2178	
  2179		/*
  2180		 * Compute how many arguments we need to pass to BPF programs.
  2181		 * BPF ABI mirrors that of x86_64: arguments that are 16 bytes or
  2182		 * smaller are packed into 1 or 2 registers; larger arguments are
  2183		 * passed via pointers.
  2184		 * In s390x ABI, arguments that are 8 bytes or smaller are packed into
  2185		 * a register; larger arguments are passed via pointers.
  2186		 * We need to deal with this difference.
  2187		 */
  2188		nr_bpf_args = 0;
  2189		for (i = 0; i < m->nr_args; i++) {
  2190			if (m->arg_size[i] <= 8)
  2191				nr_bpf_args += 1;
  2192			else if (m->arg_size[i] <= 16)
  2193				nr_bpf_args += 2;
  2194			else
  2195				return -ENOTSUPP;
  2196		}
  2197	
  2198		/*
  2199		 * Calculate the stack layout.
  2200		 */
  2201	
  2202		/* Reserve STACK_FRAME_OVERHEAD bytes for the callees. */
  2203		tjit->stack_size = STACK_FRAME_OVERHEAD;
  2204		tjit->stack_args_off = alloc_stack(tjit, nr_stack_args * sizeof(u64));
  2205		tjit->reg_args_off = alloc_stack(tjit, nr_reg_args * sizeof(u64));
  2206		tjit->ip_off = alloc_stack(tjit, sizeof(u64));
  2207		tjit->arg_cnt_off = alloc_stack(tjit, sizeof(u64));
  2208		tjit->bpf_args_off = alloc_stack(tjit, nr_bpf_args * sizeof(u64));
  2209		tjit->retval_off = alloc_stack(tjit, sizeof(u64));
  2210		tjit->r7_r8_off = alloc_stack(tjit, 2 * sizeof(u64));
  2211		tjit->r14_off = alloc_stack(tjit, sizeof(u64));
  2212		tjit->run_ctx_off = alloc_stack(tjit,
  2213						sizeof(struct bpf_tramp_run_ctx));
  2214		/* The caller has already reserved STACK_FRAME_OVERHEAD bytes. */
  2215		tjit->stack_size -= STACK_FRAME_OVERHEAD;
  2216		tjit->orig_stack_args_off = tjit->stack_size + STACK_FRAME_OVERHEAD;
  2217	
  2218		/* aghi %r15,-stack_size */
  2219		EMIT4_IMM(0xa70b0000, REG_15, -tjit->stack_size);
  2220		/* stmg %r2,%rN,fwd_reg_args_off(%r15) */
  2221		if (nr_reg_args)
  2222			EMIT6_DISP_LH(0xeb000000, 0x0024, REG_2,
  2223				      REG_2 + (nr_reg_args - 1), REG_15,
  2224				      tjit->reg_args_off);
  2225		for (i = 0, j = 0; i < m->nr_args; i++) {
  2226			if (i < MAX_NR_REG_ARGS)
  2227				arg = REG_2 + i;
  2228			else
  2229				arg = tjit->orig_stack_args_off +
  2230				      (i - MAX_NR_REG_ARGS) * sizeof(u64);
  2231			bpf_arg_off = tjit->bpf_args_off + j * sizeof(u64);
  2232			if (m->arg_size[i] <= 8) {
  2233				if (i < MAX_NR_REG_ARGS)
  2234					/* stg %arg,bpf_arg_off(%r15) */
  2235					EMIT6_DISP_LH(0xe3000000, 0x0024, arg,
  2236						      REG_0, REG_15, bpf_arg_off);
  2237				else
  2238					/* mvc bpf_arg_off(8,%r15),arg(%r15) */
  2239					_EMIT6(0xd207f000 | bpf_arg_off,
  2240					       0xf000 | arg);
  2241				j += 1;
  2242			} else {
  2243				if (i < MAX_NR_REG_ARGS) {
  2244					/* mvc bpf_arg_off(16,%r15),0(%arg) */
  2245					_EMIT6(0xd20ff000 | bpf_arg_off,
  2246					       reg2hex[arg] << 12);
  2247				} else {
  2248					/* lg %r1,arg(%r15) */
  2249					EMIT6_DISP_LH(0xe3000000, 0x0004, REG_1, REG_0,
  2250						      REG_15, arg);
  2251					/* mvc bpf_arg_off(16,%r15),0(%r1) */
  2252					_EMIT6(0xd20ff000 | bpf_arg_off, 0x1000);
  2253				}
  2254				j += 2;
  2255			}
  2256		}
  2257		/* stmg %r7,%r8,r7_r8_off(%r15) */
  2258		EMIT6_DISP_LH(0xeb000000, 0x0024, REG_7, REG_8, REG_15,
  2259			      tjit->r7_r8_off);
  2260		/* stg %r14,r14_off(%r15) */
  2261		EMIT6_DISP_LH(0xe3000000, 0x0024, REG_14, REG_0, REG_15, tjit->r14_off);
  2262	
  2263		if (flags & BPF_TRAMP_F_ORIG_STACK) {
  2264			/*
  2265			 * The ftrace trampoline puts the return address (which is the
  2266			 * address of the original function + S390X_PATCH_SIZE) into
  2267			 * %r0; see ftrace_shared_hotpatch_trampoline_br and
  2268			 * ftrace_init_nop() for details.
  2269			 */
  2270	
  2271			/* lgr %r8,%r0 */
  2272			EMIT4(0xb9040000, REG_8, REG_0);
  2273		} else {
  2274			/* %r8 = func_addr + S390X_PATCH_SIZE */
  2275			load_imm64(jit, REG_8, (u64)func_addr + S390X_PATCH_SIZE);
  2276		}
  2277	
  2278		/*
  2279		 * ip = func_addr;
  2280		 * arg_cnt = m->nr_args;
  2281		 */
  2282	
  2283		if (flags & BPF_TRAMP_F_IP_ARG) {
  2284			/* %r0 = func_addr */
  2285			load_imm64(jit, REG_0, (u64)func_addr);
  2286			/* stg %r0,ip_off(%r15) */
  2287			EMIT6_DISP_LH(0xe3000000, 0x0024, REG_0, REG_0, REG_15,
  2288				      tjit->ip_off);
  2289		}
  2290		/* lghi %r0,nr_bpf_args */
  2291		EMIT4_IMM(0xa7090000, REG_0, nr_bpf_args);
  2292		/* stg %r0,arg_cnt_off(%r15) */
  2293		EMIT6_DISP_LH(0xe3000000, 0x0024, REG_0, REG_0, REG_15,
  2294			      tjit->arg_cnt_off);
  2295	
  2296		if (flags & BPF_TRAMP_F_CALL_ORIG) {
  2297			/*
  2298			 * __bpf_tramp_enter(im);
  2299			 */
  2300	
  2301			/* %r1 = __bpf_tramp_enter */
  2302			load_imm64(jit, REG_1, (u64)__bpf_tramp_enter);
  2303			/* %r2 = im */
  2304			load_imm64(jit, REG_2, (u64)im);
  2305			/* %r1() */
  2306			call_r1(jit);
  2307		}
  2308	
  2309		for (i = 0; i < fentry->nr_links; i++)
  2310			invoke_bpf_prog(tjit, m, fentry->links[i],
  2311					flags & BPF_TRAMP_F_RET_FENTRY_RET);
  2312	
  2313		if (fmod_ret->nr_links) {
  2314			/*
  2315			 * retval = 0;
  2316			 */
  2317	
  2318			/* xc retval_off(8,%r15),retval_off(%r15) */
  2319			_EMIT6(0xd707f000 | tjit->retval_off,
  2320			       0xf000 | tjit->retval_off);
  2321	
  2322			for (i = 0; i < fmod_ret->nr_links; i++) {
  2323				invoke_bpf_prog(tjit, m, fmod_ret->links[i], true);
  2324	
  2325				/*
  2326				 * if (retval)
  2327				 *         goto do_fexit;
  2328				 */
  2329	
  2330				/* ltg %r0,retval_off(%r15) */
  2331				EMIT6_DISP_LH(0xe3000000, 0x0002, REG_0, REG_0, REG_15,
  2332					      tjit->retval_off);
  2333				/* brcl 7,do_fexit */
  2334				EMIT6_PCREL_RILC(0xc0040000, 7, tjit->do_fexit);
  2335			}
  2336		}
  2337	
  2338		if (flags & BPF_TRAMP_F_CALL_ORIG) {
  2339			/*
  2340			 * retval = func_addr(args);
  2341			 */
  2342	
  2343			/* lmg %r2,%rN,reg_args_off(%r15) */
  2344			if (nr_reg_args)
  2345				EMIT6_DISP_LH(0xeb000000, 0x0004, REG_2,
  2346					      REG_2 + (nr_reg_args - 1), REG_15,
  2347					      tjit->reg_args_off);
  2348			/* mvc stack_args_off(N,%r15),orig_stack_args_off(%r15) */
  2349			if (nr_stack_args)
  2350				_EMIT6(0xd200f000 |
  2351					       (nr_stack_args * sizeof(u64) - 1) << 16 |
  2352					       tjit->stack_args_off,
  2353				       0xf000 | tjit->orig_stack_args_off);
  2354			/* lgr %r1,%r8 */
  2355			EMIT4(0xb9040000, REG_1, REG_8);
  2356			/* %r1() */
  2357			call_r1(jit);
  2358			/* stg %r2,retval_off(%r15) */
  2359			EMIT6_DISP_LH(0xe3000000, 0x0024, REG_2, REG_0, REG_15,
  2360				      tjit->retval_off);
  2361	
  2362			im->ip_after_call = jit->prg_buf + jit->prg;
  2363	
  2364			/*
  2365			 * The following nop will be patched by bpf_tramp_image_put().
  2366			 */
  2367	
  2368			/* brcl 0,im->ip_epilogue */
  2369			EMIT6_PCREL_RILC(0xc0040000, 0, (u64)im->ip_epilogue);
  2370		}
  2371	
  2372		/* do_fexit: */
  2373		tjit->do_fexit = jit->prg;
  2374		for (i = 0; i < fexit->nr_links; i++)
  2375			invoke_bpf_prog(tjit, m, fexit->links[i], false);
  2376	
  2377		if (flags & BPF_TRAMP_F_CALL_ORIG) {
  2378			im->ip_epilogue = jit->prg_buf + jit->prg;
  2379	
  2380			/*
  2381			 * __bpf_tramp_exit(im);
  2382			 */
  2383	
  2384			/* %r1 = __bpf_tramp_exit */
  2385			load_imm64(jit, REG_1, (u64)__bpf_tramp_exit);
  2386			/* %r2 = im */
  2387			load_imm64(jit, REG_2, (u64)im);
  2388			/* %r1() */
  2389			call_r1(jit);
  2390		}
  2391	
  2392		/* lmg %r2,%rN,reg_args_off(%r15) */
  2393		if ((flags & BPF_TRAMP_F_RESTORE_REGS) && nr_reg_args)
  2394			EMIT6_DISP_LH(0xeb000000, 0x0004, REG_2,
  2395				      REG_2 + (nr_reg_args - 1), REG_15,
  2396				      tjit->reg_args_off);
  2397		/* lgr %r1,%r8 */
  2398		if (!(flags & BPF_TRAMP_F_SKIP_FRAME))
  2399			EMIT4(0xb9040000, REG_1, REG_8);
  2400		/* lmg %r7,%r8,r7_r8_off(%r15) */
  2401		EMIT6_DISP_LH(0xeb000000, 0x0004, REG_7, REG_8, REG_15,
  2402			      tjit->r7_r8_off);
  2403		/* lg %r14,r14_off(%r15) */
  2404		EMIT6_DISP_LH(0xe3000000, 0x0004, REG_14, REG_0, REG_15, tjit->r14_off);
  2405		/* lg %r2,retval_off(%r15) */
  2406		if (flags & (BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_RET_FENTRY_RET))
  2407			EMIT6_DISP_LH(0xe3000000, 0x0004, REG_2, REG_0, REG_15,
  2408				      tjit->retval_off);
  2409		/* aghi %r15,stack_size */
  2410		EMIT4_IMM(0xa70b0000, REG_15, tjit->stack_size);
  2411		/* Emit an expoline for the following indirect jump. */
  2412		if (nospec_uses_trampoline())
  2413			emit_expoline(jit);
  2414		if (flags & BPF_TRAMP_F_SKIP_FRAME)
  2415			/* br %r14 */
  2416			_EMIT2(0x07fe);
  2417		else
  2418			/* br %r1 */
  2419			_EMIT2(0x07f1);
  2420	
  2421		emit_r1_thunk(jit);
  2422	
  2423		return 0;
  2424	}
  2425	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH bpf-next 22/24] s390/bpf: Implement arch_prepare_bpf_trampoline()

[kernel test robot]

Hi Ilya,

I love your patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Ilya-Leoshkevich/selftests-bpf-Fix-liburandom_read-so-linker-error/20230128-150321
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20230125213817.1424447-23-iii%40linux.ibm.com
patch subject: [PATCH bpf-next 22/24] s390/bpf: Implement arch_prepare_bpf_trampoline()
config: s390-randconfig-r025-20230212 (https://download.01.org/0day-ci/archive/20230216/202302160158.P4gvGOoV-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project db89896bbbd2251fff457699635acbbedeead27f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/6257ca590791a94b97f1b4f72851dc110290a52b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Ilya-Leoshkevich/selftests-bpf-Fix-liburandom_read-so-linker-error/20230128-150321
        git checkout 6257ca590791a94b97f1b4f72851dc110290a52b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash arch/s390/net/ drivers/md/bcache/ drivers/pci/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302160158.P4gvGOoV-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from arch/s390/net/bpf_jit_comp.c:21:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from arch/s390/net/bpf_jit_comp.c:21:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from arch/s390/net/bpf_jit_comp.c:21:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> arch/s390/net/bpf_jit_comp.c:2155:5: warning: no previous prototype for function '__arch_prepare_bpf_trampoline' [-Wmissing-prototypes]
   int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
       ^
   arch/s390/net/bpf_jit_comp.c:2155:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
   ^
   static 
   13 warnings generated.


vim +/__arch_prepare_bpf_trampoline +2155 arch/s390/net/bpf_jit_comp.c

  2154	
> 2155	int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
  2156					  struct bpf_tramp_jit *tjit,
  2157					  const struct btf_func_model *m,
  2158					  u32 flags, struct bpf_tramp_links *tlinks,
  2159					  void *func_addr)
  2160	{
  2161		struct bpf_tramp_links *fmod_ret = &tlinks[BPF_TRAMP_MODIFY_RETURN];
  2162		struct bpf_tramp_links *fentry = &tlinks[BPF_TRAMP_FENTRY];
  2163		struct bpf_tramp_links *fexit = &tlinks[BPF_TRAMP_FEXIT];
  2164		int nr_bpf_args, nr_reg_args, nr_stack_args;
  2165		struct bpf_jit *jit = &tjit->common;
  2166		int arg, bpf_arg_off;
  2167		int i, j;
  2168	
  2169		/* Support as many stack arguments as "mvc" instruction can handle. */
  2170		nr_reg_args = min_t(int, m->nr_args, MAX_NR_REG_ARGS);
  2171		nr_stack_args = m->nr_args - nr_reg_args;
  2172		if (nr_stack_args > MAX_NR_STACK_ARGS)
  2173			return -ENOTSUPP;
  2174	
  2175		/* Return to %r14, since func_addr and %r0 are not available. */
  2176		if (!func_addr && !(flags & BPF_TRAMP_F_ORIG_STACK))
  2177			flags |= BPF_TRAMP_F_SKIP_FRAME;
  2178	
  2179		/*
  2180		 * Compute how many arguments we need to pass to BPF programs.
  2181		 * BPF ABI mirrors that of x86_64: arguments that are 16 bytes or
  2182		 * smaller are packed into 1 or 2 registers; larger arguments are
  2183		 * passed via pointers.
  2184		 * In s390x ABI, arguments that are 8 bytes or smaller are packed into
  2185		 * a register; larger arguments are passed via pointers.
  2186		 * We need to deal with this difference.
  2187		 */
  2188		nr_bpf_args = 0;
  2189		for (i = 0; i < m->nr_args; i++) {
  2190			if (m->arg_size[i] <= 8)
  2191				nr_bpf_args += 1;
  2192			else if (m->arg_size[i] <= 16)
  2193				nr_bpf_args += 2;
  2194			else
  2195				return -ENOTSUPP;
  2196		}
  2197	
  2198		/*
  2199		 * Calculate the stack layout.
  2200		 */
  2201	
  2202		/* Reserve STACK_FRAME_OVERHEAD bytes for the callees. */
  2203		tjit->stack_size = STACK_FRAME_OVERHEAD;
  2204		tjit->stack_args_off = alloc_stack(tjit, nr_stack_args * sizeof(u64));
  2205		tjit->reg_args_off = alloc_stack(tjit, nr_reg_args * sizeof(u64));
  2206		tjit->ip_off = alloc_stack(tjit, sizeof(u64));
  2207		tjit->arg_cnt_off = alloc_stack(tjit, sizeof(u64));
  2208		tjit->bpf_args_off = alloc_stack(tjit, nr_bpf_args * sizeof(u64));
  2209		tjit->retval_off = alloc_stack(tjit, sizeof(u64));
  2210		tjit->r7_r8_off = alloc_stack(tjit, 2 * sizeof(u64));
  2211		tjit->r14_off = alloc_stack(tjit, sizeof(u64));
  2212		tjit->run_ctx_off = alloc_stack(tjit,
  2213						sizeof(struct bpf_tramp_run_ctx));
  2214		/* The caller has already reserved STACK_FRAME_OVERHEAD bytes. */
  2215		tjit->stack_size -= STACK_FRAME_OVERHEAD;
  2216		tjit->orig_stack_args_off = tjit->stack_size + STACK_FRAME_OVERHEAD;
  2217	
  2218		/* aghi %r15,-stack_size */
  2219		EMIT4_IMM(0xa70b0000, REG_15, -tjit->stack_size);
  2220		/* stmg %r2,%rN,fwd_reg_args_off(%r15) */
  2221		if (nr_reg_args)
  2222			EMIT6_DISP_LH(0xeb000000, 0x0024, REG_2,
  2223				      REG_2 + (nr_reg_args - 1), REG_15,
  2224				      tjit->reg_args_off);
  2225		for (i = 0, j = 0; i < m->nr_args; i++) {
  2226			if (i < MAX_NR_REG_ARGS)
  2227				arg = REG_2 + i;
  2228			else
  2229				arg = tjit->orig_stack_args_off +
  2230				      (i - MAX_NR_REG_ARGS) * sizeof(u64);
  2231			bpf_arg_off = tjit->bpf_args_off + j * sizeof(u64);
  2232			if (m->arg_size[i] <= 8) {
  2233				if (i < MAX_NR_REG_ARGS)
  2234					/* stg %arg,bpf_arg_off(%r15) */
  2235					EMIT6_DISP_LH(0xe3000000, 0x0024, arg,
  2236						      REG_0, REG_15, bpf_arg_off);
  2237				else
  2238					/* mvc bpf_arg_off(8,%r15),arg(%r15) */
  2239					_EMIT6(0xd207f000 | bpf_arg_off,
  2240					       0xf000 | arg);
  2241				j += 1;
  2242			} else {
  2243				if (i < MAX_NR_REG_ARGS) {
  2244					/* mvc bpf_arg_off(16,%r15),0(%arg) */
  2245					_EMIT6(0xd20ff000 | bpf_arg_off,
  2246					       reg2hex[arg] << 12);
  2247				} else {
  2248					/* lg %r1,arg(%r15) */
  2249					EMIT6_DISP_LH(0xe3000000, 0x0004, REG_1, REG_0,
  2250						      REG_15, arg);
  2251					/* mvc bpf_arg_off(16,%r15),0(%r1) */
  2252					_EMIT6(0xd20ff000 | bpf_arg_off, 0x1000);
  2253				}
  2254				j += 2;
  2255			}
  2256		}
  2257		/* stmg %r7,%r8,r7_r8_off(%r15) */
  2258		EMIT6_DISP_LH(0xeb000000, 0x0024, REG_7, REG_8, REG_15,
  2259			      tjit->r7_r8_off);
  2260		/* stg %r14,r14_off(%r15) */
  2261		EMIT6_DISP_LH(0xe3000000, 0x0024, REG_14, REG_0, REG_15, tjit->r14_off);
  2262	
  2263		if (flags & BPF_TRAMP_F_ORIG_STACK) {
  2264			/*
  2265			 * The ftrace trampoline puts the return address (which is the
  2266			 * address of the original function + S390X_PATCH_SIZE) into
  2267			 * %r0; see ftrace_shared_hotpatch_trampoline_br and
  2268			 * ftrace_init_nop() for details.
  2269			 */
  2270	
  2271			/* lgr %r8,%r0 */
  2272			EMIT4(0xb9040000, REG_8, REG_0);
  2273		} else {
  2274			/* %r8 = func_addr + S390X_PATCH_SIZE */
  2275			load_imm64(jit, REG_8, (u64)func_addr + S390X_PATCH_SIZE);
  2276		}
  2277	
  2278		/*
  2279		 * ip = func_addr;
  2280		 * arg_cnt = m->nr_args;
  2281		 */
  2282	
  2283		if (flags & BPF_TRAMP_F_IP_ARG) {
  2284			/* %r0 = func_addr */
  2285			load_imm64(jit, REG_0, (u64)func_addr);
  2286			/* stg %r0,ip_off(%r15) */
  2287			EMIT6_DISP_LH(0xe3000000, 0x0024, REG_0, REG_0, REG_15,
  2288				      tjit->ip_off);
  2289		}
  2290		/* lghi %r0,nr_bpf_args */
  2291		EMIT4_IMM(0xa7090000, REG_0, nr_bpf_args);
  2292		/* stg %r0,arg_cnt_off(%r15) */
  2293		EMIT6_DISP_LH(0xe3000000, 0x0024, REG_0, REG_0, REG_15,
  2294			      tjit->arg_cnt_off);
  2295	
  2296		if (flags & BPF_TRAMP_F_CALL_ORIG) {
  2297			/*
  2298			 * __bpf_tramp_enter(im);
  2299			 */
  2300	
  2301			/* %r1 = __bpf_tramp_enter */
  2302			load_imm64(jit, REG_1, (u64)__bpf_tramp_enter);
  2303			/* %r2 = im */
  2304			load_imm64(jit, REG_2, (u64)im);
  2305			/* %r1() */
  2306			call_r1(jit);
  2307		}
  2308	
  2309		for (i = 0; i < fentry->nr_links; i++)
  2310			invoke_bpf_prog(tjit, m, fentry->links[i],
  2311					flags & BPF_TRAMP_F_RET_FENTRY_RET);
  2312	
  2313		if (fmod_ret->nr_links) {
  2314			/*
  2315			 * retval = 0;
  2316			 */
  2317	
  2318			/* xc retval_off(8,%r15),retval_off(%r15) */
  2319			_EMIT6(0xd707f000 | tjit->retval_off,
  2320			       0xf000 | tjit->retval_off);
  2321	
  2322			for (i = 0; i < fmod_ret->nr_links; i++) {
  2323				invoke_bpf_prog(tjit, m, fmod_ret->links[i], true);
  2324	
  2325				/*
  2326				 * if (retval)
  2327				 *         goto do_fexit;
  2328				 */
  2329	
  2330				/* ltg %r0,retval_off(%r15) */
  2331				EMIT6_DISP_LH(0xe3000000, 0x0002, REG_0, REG_0, REG_15,
  2332					      tjit->retval_off);
  2333				/* brcl 7,do_fexit */
  2334				EMIT6_PCREL_RILC(0xc0040000, 7, tjit->do_fexit);
  2335			}
  2336		}
  2337	
  2338		if (flags & BPF_TRAMP_F_CALL_ORIG) {
  2339			/*
  2340			 * retval = func_addr(args);
  2341			 */
  2342	
  2343			/* lmg %r2,%rN,reg_args_off(%r15) */
  2344			if (nr_reg_args)
  2345				EMIT6_DISP_LH(0xeb000000, 0x0004, REG_2,
  2346					      REG_2 + (nr_reg_args - 1), REG_15,
  2347					      tjit->reg_args_off);
  2348			/* mvc stack_args_off(N,%r15),orig_stack_args_off(%r15) */
  2349			if (nr_stack_args)
  2350				_EMIT6(0xd200f000 |
  2351					       (nr_stack_args * sizeof(u64) - 1) << 16 |
  2352					       tjit->stack_args_off,
  2353				       0xf000 | tjit->orig_stack_args_off);
  2354			/* lgr %r1,%r8 */
  2355			EMIT4(0xb9040000, REG_1, REG_8);
  2356			/* %r1() */
  2357			call_r1(jit);
  2358			/* stg %r2,retval_off(%r15) */
  2359			EMIT6_DISP_LH(0xe3000000, 0x0024, REG_2, REG_0, REG_15,
  2360				      tjit->retval_off);
  2361	
  2362			im->ip_after_call = jit->prg_buf + jit->prg;
  2363	
  2364			/*
  2365			 * The following nop will be patched by bpf_tramp_image_put().
  2366			 */
  2367	
  2368			/* brcl 0,im->ip_epilogue */
  2369			EMIT6_PCREL_RILC(0xc0040000, 0, (u64)im->ip_epilogue);
  2370		}
  2371	
  2372		/* do_fexit: */
  2373		tjit->do_fexit = jit->prg;
  2374		for (i = 0; i < fexit->nr_links; i++)
  2375			invoke_bpf_prog(tjit, m, fexit->links[i], false);
  2376	
  2377		if (flags & BPF_TRAMP_F_CALL_ORIG) {
  2378			im->ip_epilogue = jit->prg_buf + jit->prg;
  2379	
  2380			/*
  2381			 * __bpf_tramp_exit(im);
  2382			 */
  2383	
  2384			/* %r1 = __bpf_tramp_exit */
  2385			load_imm64(jit, REG_1, (u64)__bpf_tramp_exit);
  2386			/* %r2 = im */
  2387			load_imm64(jit, REG_2, (u64)im);
  2388			/* %r1() */
  2389			call_r1(jit);
  2390		}
  2391	
  2392		/* lmg %r2,%rN,reg_args_off(%r15) */
  2393		if ((flags & BPF_TRAMP_F_RESTORE_REGS) && nr_reg_args)
  2394			EMIT6_DISP_LH(0xeb000000, 0x0004, REG_2,
  2395				      REG_2 + (nr_reg_args - 1), REG_15,
  2396				      tjit->reg_args_off);
  2397		/* lgr %r1,%r8 */
  2398		if (!(flags & BPF_TRAMP_F_SKIP_FRAME))
  2399			EMIT4(0xb9040000, REG_1, REG_8);
  2400		/* lmg %r7,%r8,r7_r8_off(%r15) */
  2401		EMIT6_DISP_LH(0xeb000000, 0x0004, REG_7, REG_8, REG_15,
  2402			      tjit->r7_r8_off);
  2403		/* lg %r14,r14_off(%r15) */
  2404		EMIT6_DISP_LH(0xe3000000, 0x0004, REG_14, REG_0, REG_15, tjit->r14_off);
  2405		/* lg %r2,retval_off(%r15) */
  2406		if (flags & (BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_RET_FENTRY_RET))
  2407			EMIT6_DISP_LH(0xe3000000, 0x0004, REG_2, REG_0, REG_15,
  2408				      tjit->retval_off);
  2409		/* aghi %r15,stack_size */
  2410		EMIT4_IMM(0xa70b0000, REG_15, tjit->stack_size);
  2411		/* Emit an expoline for the following indirect jump. */
  2412		if (nospec_uses_trampoline())
  2413			emit_expoline(jit);
  2414		if (flags & BPF_TRAMP_F_SKIP_FRAME)
  2415			/* br %r14 */
  2416			_EMIT2(0x07fe);
  2417		else
  2418			/* br %r1 */
  2419			_EMIT2(0x07f1);
  2420	
  2421		emit_r1_thunk(jit);
  2422	
  2423		return 0;
  2424	}
  2425	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

[PATCH bpf-next 23/24] s390/bpf: Implement bpf_jit_supports_subprog_tailcalls()

[Ilya Leoshkevich]

Allow mixing subprogs and tail calls by passing the current tail
call count to subprogs.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 37 ++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index ea8203bd4112..f4cdecb32629 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -58,7 +58,6 @@ struct bpf_jit {
 #define SEEN_MEM	BIT(0)		/* use mem[] for temporary storage */
 #define SEEN_LITERAL	BIT(1)		/* code uses literals */
 #define SEEN_FUNC	BIT(2)		/* calls C functions */
-#define SEEN_TAIL_CALL	BIT(3)		/* code uses tail calls */
 #define SEEN_STACK	(SEEN_FUNC | SEEN_MEM)
 
 /*
@@ -549,20 +548,23 @@ static void bpf_jit_plt(void *plt, void *ret, void *target)
  * Save registers and create stack frame if necessary.
  * See stack frame layout description in "bpf_jit.h"!
  */
-static void bpf_jit_prologue(struct bpf_jit *jit, u32 stack_depth)
+static void bpf_jit_prologue(struct bpf_jit *jit, struct bpf_prog *fp,
+			     u32 stack_depth)
 {
 	/* No-op for hotpatching */
 	/* brcl 0,prologue_plt */
 	EMIT6_PCREL_RILC(0xc0040000, 0, jit->prologue_plt);
 	jit->prologue_plt_ret = jit->prg;
 
-	if (jit->seen & SEEN_TAIL_CALL) {
+	if (fp->aux->func_idx == 0) {
+		/* Initialize the tail call counter in the main program. */
 		/* xc STK_OFF_TCCNT(4,%r15),STK_OFF_TCCNT(%r15) */
 		_EMIT6(0xd703f000 | STK_OFF_TCCNT, 0xf000 | STK_OFF_TCCNT);
 	} else {
 		/*
-		 * There are no tail calls. Insert nops in order to have
-		 * tail_call_start at a predictable offset.
+		 * Skip the tail call counter initialization in subprograms.
+		 * Insert nops in order to have tail_call_start at a
+		 * predictable offset.
 		 */
 		bpf_skip(jit, 6);
 	}
@@ -1410,6 +1412,19 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 
 		REG_SET_SEEN(BPF_REG_5);
 		jit->seen |= SEEN_FUNC;
+		/*
+		 * Copy the tail call counter to where the callee expects it.
+		 *
+		 * Note 1: The callee can increment the tail call counter, but
+		 * we do not load it back, since the x86 JIT does not do this
+		 * either.
+		 *
+		 * Note 2: We assume that the verifier does not let us call the
+		 * main program, which clears the tail call counter on entry.
+		 */
+		/* mvc STK_OFF_TCCNT(4,%r15),N(%r15) */
+		_EMIT6(0xd203f000 | STK_OFF_TCCNT,
+		       0xf000 | (STK_OFF_TCCNT + STK_OFF + stack_depth));
 		/* lgrl %w1,func */
 		EMIT6_PCREL_RILB(0xc4080000, REG_W1, _EMIT_CONST_U64(func));
 		/* %r1() */
@@ -1426,10 +1441,7 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 		 *  B1: pointer to ctx
 		 *  B2: pointer to bpf_array
 		 *  B3: index in bpf_array
-		 */
-		jit->seen |= SEEN_TAIL_CALL;
-
-		/*
+		 *
 		 * if (index >= array->map.max_entries)
 		 *         goto out;
 		 */
@@ -1793,7 +1805,7 @@ static int bpf_jit_prog(struct bpf_jit *jit, struct bpf_prog *fp,
 	jit->prg = 0;
 	jit->excnt = 0;
 
-	bpf_jit_prologue(jit, stack_depth);
+	bpf_jit_prologue(jit, fp, stack_depth);
 	if (bpf_set_addr(jit, 0) < 0)
 		return -1;
 	for (i = 0; i < fp->len; i += insn_count) {
@@ -2455,3 +2467,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
 
 	return ret;
 }
+
+bool bpf_jit_supports_subprog_tailcalls(void)
+{
+	return true;
+}
-- 
2.39.1

[PATCH bpf-next 24/24] s390/bpf: Implement bpf_jit_supports_kfunc_call()

[Ilya Leoshkevich]

Implement calling kernel functions from eBPF. In general, the eBPF ABI
is fairly close to that of s390x, with one important difference: on
s390x callers should sign-extend signed arguments. Handle that by using
information returned by bpf_jit_find_kfunc_model().

At the moment bpf_jit_find_kfunc_model() does not seem to play nicely
with XDP metadata functions: add_kfunc_call() adds an "abstract" bpf_*()
version to kfunc_btf_tab, but then fixup_kfunc_call() puts the concrete
version into insn->imm, which bpf_jit_find_kfunc_model() cannot find.
But this seems to be a common code problem.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index f4cdecb32629..74b38e817bd8 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -1401,9 +1401,10 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 	 */
 	case BPF_JMP | BPF_CALL:
 	{
-		u64 func;
+		const struct btf_func_model *m;
 		bool func_addr_fixed;
-		int ret;
+		int j, ret;
+		u64 func;
 
 		ret = bpf_jit_get_func_addr(fp, insn, extra_pass,
 					    &func, &func_addr_fixed);
@@ -1425,6 +1426,21 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 		/* mvc STK_OFF_TCCNT(4,%r15),N(%r15) */
 		_EMIT6(0xd203f000 | STK_OFF_TCCNT,
 		       0xf000 | (STK_OFF_TCCNT + STK_OFF + stack_depth));
+
+		/* Sign-extend the kfunc arguments. */
+		if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
+			m = bpf_jit_find_kfunc_model(fp, insn);
+			if (!m)
+				return -1;
+
+			for (j = 0; j < m->nr_args; j++) {
+				if (sign_extend(jit, BPF_REG_1 + j,
+						m->arg_size[j],
+						m->arg_flags[j]))
+					return -1;
+			}
+		}
+
 		/* lgrl %w1,func */
 		EMIT6_PCREL_RILB(0xc4080000, REG_W1, _EMIT_CONST_U64(func));
 		/* %r1() */
@@ -1980,6 +1996,11 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
 	return fp;
 }
 
+bool bpf_jit_supports_kfunc_call(void)
+{
+	return true;
+}
+
 int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
 		       void *old_addr, void *new_addr)
 {
-- 
2.39.1

Re: [PATCH bpf-next 24/24] s390/bpf: Implement bpf_jit_supports_kfunc_call()

[Alexei Starovoitov]

On Wed, Jan 25, 2023 at 10:38:17PM +0100, Ilya Leoshkevich wrote:
> +
> +		/* Sign-extend the kfunc arguments. */
> +		if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> +			m = bpf_jit_find_kfunc_model(fp, insn);
> +			if (!m)
> +				return -1;
> +
> +			for (j = 0; j < m->nr_args; j++) {
> +				if (sign_extend(jit, BPF_REG_1 + j,
> +						m->arg_size[j],
> +						m->arg_flags[j]))
> +					return -1;
> +			}
> +		}

Is this because s390 doesn't have subregisters?
Could you give an example where it's necessary?
I'm guessing a bpf prog compiled with alu32 and operates on signed int
that is passed into a kfunc that expects 'int' in 64-bit reg?

>  
> +bool bpf_jit_supports_kfunc_call(void)
> +{
> +	return true;
> +}

Timely :) Thanks for working it.

Re: [PATCH bpf-next 24/24] s390/bpf: Implement bpf_jit_supports_kfunc_call()

[Ilya Leoshkevich]

On Wed, 2023-01-25 at 17:28 -0800, Alexei Starovoitov wrote:
> On Wed, Jan 25, 2023 at 10:38:17PM +0100, Ilya Leoshkevich wrote:
> > +
> > +               /* Sign-extend the kfunc arguments. */
> > +               if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> > +                       m = bpf_jit_find_kfunc_model(fp, insn);
> > +                       if (!m)
> > +                               return -1;
> > +
> > +                       for (j = 0; j < m->nr_args; j++) {
> > +                               if (sign_extend(jit, BPF_REG_1 + j,
> > +                                               m->arg_size[j],
> > +                                               m->arg_flags[j]))
> > +                                       return -1;
> > +                       }
> > +               }
> 
> Is this because s390 doesn't have subregisters?
> Could you give an example where it's necessary?
> I'm guessing a bpf prog compiled with alu32 and operates on signed
> int
> that is passed into a kfunc that expects 'int' in 64-bit reg?

Precisely. The test added in 13/24 fails without this:

verify_success:PASS:skel 0 nsec                                       
verify_success:PASS:bpf_object__find_program_by_name 0 nsec           
verify_success:PASS:kfunc_call_test4 0 nsec                           
verify_success:FAIL:retval unexpected retval: actual 4294966065 !=
expected -1234                                                        
#94/10   kfunc_call/kfunc_call_test4:FAIL                    

Looking at the assembly:

; long noinline bpf_kfunc_call_test4(signed char a, short b, int c,
long d)
0000000000936a78 <bpf_kfunc_call_test4>:
  936a78:       c0 04 00 00 00 00       jgnop   936a78
<bpf_kfunc_call_test4>
; 	return (long)a + (long)b + (long)c + d;
  936a7e:       b9 08 00 45             agr     %r4,%r5
  936a82:       b9 08 00 43             agr     %r4,%r3
  936a86:       b9 08 00 24             agr     %r2,%r4
  936a8a:       c0 f4 00 1e 3b 27       jg      cfe0d8
<__s390_indirect_jump_r14>

As per the s390x ABI, bpf_kfunc_call_test4() has the right to assume
that a, b and c are sign-extended by the caller, which results in using
64-bit additions (agr) without any additional conversions.

On the JITed code side (without this hunk) we have:

; tmp = bpf_kfunc_call_test4(-3, -30, -200, -1000);
;        5:       b4 10 00 00 ff ff ff fd w1 = -3
   0x3ff7fdcdad4:       llilf   %r2,0xfffffffd
;        6:       b4 20 00 00 ff ff ff e2 w2 = -30
   0x3ff7fdcdada:       llilf   %r3,0xffffffe2
;        7:       b4 30 00 00 ff ff ff 38 w3 = -200
   0x3ff7fdcdae0:       llilf   %r4,0xffffff38
;       8:       b7 40 00 00 ff ff fc 18 r4 = -1000
   0x3ff7fdcdae6:       lgfi    %r5,-1000
   0x3ff7fdcdaec:       mvc     64(4,%r15),160(%r15)
   0x3ff7fdcdaf2:       lgrl    %r1,bpf_kfunc_call_test4@GOT
   0x3ff7fdcdaf8:       brasl   %r14,__s390_indirect_jump_r1

This first 3 llilfs are 32-bit loads, that need to be sign-extended
to 64 bits.

> > +bool bpf_jit_supports_kfunc_call(void)
> > +{
> > +       return true;
> > +}
> 
> Timely :) Thanks for working it.

Re: [PATCH bpf-next 24/24] s390/bpf: Implement bpf_jit_supports_kfunc_call()

[Alexei Starovoitov]

On Fri, Jan 27, 2023 at 3:36 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Wed, 2023-01-25 at 17:28 -0800, Alexei Starovoitov wrote:
> > On Wed, Jan 25, 2023 at 10:38:17PM +0100, Ilya Leoshkevich wrote:
> > > +
> > > +               /* Sign-extend the kfunc arguments. */
> > > +               if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> > > +                       m = bpf_jit_find_kfunc_model(fp, insn);
> > > +                       if (!m)
> > > +                               return -1;
> > > +
> > > +                       for (j = 0; j < m->nr_args; j++) {
> > > +                               if (sign_extend(jit, BPF_REG_1 + j,
> > > +                                               m->arg_size[j],
> > > +                                               m->arg_flags[j]))
> > > +                                       return -1;
> > > +                       }
> > > +               }
> >
> > Is this because s390 doesn't have subregisters?
> > Could you give an example where it's necessary?
> > I'm guessing a bpf prog compiled with alu32 and operates on signed
> > int
> > that is passed into a kfunc that expects 'int' in 64-bit reg?
>
> Precisely. The test added in 13/24 fails without this:
>
> verify_success:PASS:skel 0 nsec
> verify_success:PASS:bpf_object__find_program_by_name 0 nsec
> verify_success:PASS:kfunc_call_test4 0 nsec
> verify_success:FAIL:retval unexpected retval: actual 4294966065 !=
> expected -1234
> #94/10   kfunc_call/kfunc_call_test4:FAIL
>
> Looking at the assembly:
>
> ; long noinline bpf_kfunc_call_test4(signed char a, short b, int c,
> long d)
> 0000000000936a78 <bpf_kfunc_call_test4>:
>   936a78:       c0 04 00 00 00 00       jgnop   936a78
> <bpf_kfunc_call_test4>
> ;       return (long)a + (long)b + (long)c + d;
>   936a7e:       b9 08 00 45             agr     %r4,%r5
>   936a82:       b9 08 00 43             agr     %r4,%r3
>   936a86:       b9 08 00 24             agr     %r2,%r4
>   936a8a:       c0 f4 00 1e 3b 27       jg      cfe0d8
> <__s390_indirect_jump_r14>
>
> As per the s390x ABI, bpf_kfunc_call_test4() has the right to assume
> that a, b and c are sign-extended by the caller, which results in using
> 64-bit additions (agr) without any additional conversions.
>
> On the JITed code side (without this hunk) we have:
>
> ; tmp = bpf_kfunc_call_test4(-3, -30, -200, -1000);
> ;        5:       b4 10 00 00 ff ff ff fd w1 = -3
>    0x3ff7fdcdad4:       llilf   %r2,0xfffffffd
> ;        6:       b4 20 00 00 ff ff ff e2 w2 = -30
>    0x3ff7fdcdada:       llilf   %r3,0xffffffe2
> ;        7:       b4 30 00 00 ff ff ff 38 w3 = -200
>    0x3ff7fdcdae0:       llilf   %r4,0xffffff38
> ;       8:       b7 40 00 00 ff ff fc 18 r4 = -1000
>    0x3ff7fdcdae6:       lgfi    %r5,-1000
>    0x3ff7fdcdaec:       mvc     64(4,%r15),160(%r15)
>    0x3ff7fdcdaf2:       lgrl    %r1,bpf_kfunc_call_test4@GOT
>    0x3ff7fdcdaf8:       brasl   %r14,__s390_indirect_jump_r1
>
> This first 3 llilfs are 32-bit loads, that need to be sign-extended
> to 64 bits.

All makes sense. Please add this explanation to the commit log.
When 2nd arch appears that needs similar sign extension in
the caller we can add this logic to the verifier.

In parallel we're working on new sign extension instructions.
Doing sign extension with shifts in the verifier won't be efficient,
so we need to wait for them.

Re: [PATCH bpf-next 00/24] Support bpf trampoline for s390x

[Andrii Nakryiko]

On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Hi,
>
> This series implements poke, trampoline, kfunc, mixing subprogs and
> tailcalls, and fixes a number of tests on s390x.
>
> The following failures still remain:
>
> #52      core_read_macros:FAIL
> Uses BPF_PROBE_READ(), shouldn't there be BPF_PROBE_READ_KERNEL()?

BPF_PROBE_READ(), similarly to BPF_CORE_READ() both use
bpf_probe_read_kernel() internally, as it's most common use case. We
have separate BPF_PROBE_READ_USER() and BPF_CORE_READ_USER() for when
bpf_probe_read_user() has to be used.

>
> #82      get_stack_raw_tp:FAIL
> get_stack_print_output:FAIL:user_stack corrupted user stack
> Known issue:
> We cannot reliably unwind userspace on s390x without DWARF.

like in principle, or frame pointers (or some equivalent) needs to be
configured for this to work?

Asking also in the context of [0], where s390x was excluded. If there
is actually a way to enable frame pointer-based stack unwinding on
s390x, would be nice to hear from an expert.

  [0] https://pagure.io/fesco/issue/2923

>
> #101     ksyms_module:FAIL
> address of kernel function bpf_testmod_test_mod_kfunc is out of range
> Known issue:
> Kernel and modules are too far away from each other on s390x.
>
> #167     sk_assign:FAIL
> Uses legacy map definitions in 'maps' section.

Hm.. assuming new enough iproute2, new-style .maps definition should
be supported, right? Let's convert map definition?

>
> #190     stacktrace_build_id:FAIL
> Known issue:
> We cannot reliably unwind userspace on s390x without DWARF.
>
> #211     test_bpffs:FAIL
> iterators.bpf.c is broken on s390x, it uses BPF_CORE_READ(), shouldn't
> there be BPF_CORE_READ_KERNEL()?

BPF_CORE_READ() is that, so must be something else

>
> #218     test_profiler:FAIL
> A lot of BPF_PROBE_READ() usages.

ditto, something else

>
> #281     xdp_metadata:FAIL
> See patch 24.
>
> #284     xdp_synproxy:FAIL
> Verifier error:
> ; value = bpf_tcp_raw_gen_syncookie_ipv4(hdr->ipv4, hdr->tcp,
> 281: (79) r1 = *(u64 *)(r10 -80)      ; R1_w=pkt(off=14,r=74,imm=0) R10=fp0
> 282: (bf) r2 = r8                     ; R2_w=pkt(id=5,off=14,r=74,umax=60,var_off=(0x0; 0x3c)) R8=pkt(id=5,off=14,r=74,umax=60,var_off=(0x0; 0x3c))
> 283: (79) r3 = *(u64 *)(r10 -104)     ; R3_w=scalar(umax=60,var_off=(0x0; 0x3c)) R10=fp0
> 284: (85) call bpf_tcp_raw_gen_syncookie_ipv4#204
> invalid access to packet, off=14 size=0, R2(id=5,off=14,r=74)
> R2 offset is outside of the packet

third arg to bpf_tcp_raw_gen_syncookie_ipv4() is defined as
ARG_CONST_SIZE, so is required to be strictly positive, which doesn't
seem to be "proven" to verifier. Please provided bigger log, it might
another instance of needing to sprinkle barrier_var() around.

And maybe thinking about using ARG_CONST_SIZE_OR_ZERO instead of ARG_CONST_SIZE.

>
> None of these seem to be due to the new changes.
>
> Best regards,
> Ilya
>
> Ilya Leoshkevich (24):
>   selftests/bpf: Fix liburandom_read.so linker error
>   selftests/bpf: Fix symlink creation error
>   selftests/bpf: Fix fexit_stress on s390x
>   selftests/bpf: Fix trampoline_count on s390x
>   selftests/bpf: Fix kfree_skb on s390x
>   selftests/bpf: Set errno when urand_spawn() fails
>   selftests/bpf: Fix decap_sanity_ns cleanup
>   selftests/bpf: Fix verify_pkcs7_sig on s390x
>   selftests/bpf: Fix xdp_do_redirect on s390x
>   selftests/bpf: Fix cgrp_local_storage on s390x
>   selftests/bpf: Check stack_mprotect() return value
>   selftests/bpf: Increase SIZEOF_BPF_LOCAL_STORAGE_ELEM on s390x
>   selftests/bpf: Add a sign-extension test for kfuncs
>   selftests/bpf: Fix test_lsm on s390x
>   selftests/bpf: Fix test_xdp_adjust_tail_grow2 on s390x
>   selftests/bpf: Fix vmlinux test on s390x
>   libbpf: Read usdt arg spec with bpf_probe_read_kernel()
>   s390/bpf: Fix a typo in a comment
>   s390/bpf: Add expoline to tail calls
>   s390/bpf: Implement bpf_arch_text_poke()
>   bpf: btf: Add BTF_FMODEL_SIGNED_ARG flag
>   s390/bpf: Implement arch_prepare_bpf_trampoline()
>   s390/bpf: Implement bpf_jit_supports_subprog_tailcalls()
>   s390/bpf: Implement bpf_jit_supports_kfunc_call()
>
>  arch/s390/net/bpf_jit_comp.c                  | 708 +++++++++++++++++-
>  include/linux/bpf.h                           |   8 +
>  include/linux/btf.h                           |  15 +-
>  kernel/bpf/btf.c                              |  16 +-
>  net/bpf/test_run.c                            |   9 +
>  tools/lib/bpf/usdt.bpf.h                      |  33 +-
>  tools/testing/selftests/bpf/Makefile          |   7 +-
>  tools/testing/selftests/bpf/netcnt_common.h   |   6 +-
>  .../selftests/bpf/prog_tests/bpf_cookie.c     |   6 +-
>  .../bpf/prog_tests/cgrp_local_storage.c       |   2 +-
>  .../selftests/bpf/prog_tests/decap_sanity.c   |   2 +-
>  .../selftests/bpf/prog_tests/fexit_stress.c   |   6 +-
>  .../selftests/bpf/prog_tests/kfree_skb.c      |   2 +-
>  .../selftests/bpf/prog_tests/kfunc_call.c     |   1 +
>  .../selftests/bpf/prog_tests/test_lsm.c       |   3 +-
>  .../bpf/prog_tests/trampoline_count.c         |   4 +
>  tools/testing/selftests/bpf/prog_tests/usdt.c |   1 +
>  .../bpf/prog_tests/verify_pkcs7_sig.c         |   9 +
>  .../bpf/prog_tests/xdp_adjust_tail.c          |   7 +-
>  .../bpf/prog_tests/xdp_do_redirect.c          |   4 +
>  .../selftests/bpf/progs/kfunc_call_test.c     |  18 +
>  tools/testing/selftests/bpf/progs/lsm.c       |   7 +-
>  .../bpf/progs/test_verify_pkcs7_sig.c         |  12 +-
>  .../selftests/bpf/progs/test_vmlinux.c        |   4 +-
>  .../bpf/progs/test_xdp_adjust_tail_grow.c     |   8 +-
>  25 files changed, 816 insertions(+), 82 deletions(-)
>
> --
> 2.39.1
>

Re: [PATCH bpf-next 00/24] Support bpf trampoline for s390x

[Ilya Leoshkevich]

On Wed, 2023-01-25 at 16:45 -0800, Andrii Nakryiko wrote:
> On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > Hi,
> > 
> > This series implements poke, trampoline, kfunc, mixing subprogs and
> > tailcalls, and fixes a number of tests on s390x.
> > 
> > The following failures still remain:
> > 
> > #52      core_read_macros:FAIL
> > Uses BPF_PROBE_READ(), shouldn't there be BPF_PROBE_READ_KERNEL()?
> 
> BPF_PROBE_READ(), similarly to BPF_CORE_READ() both use
> bpf_probe_read_kernel() internally, as it's most common use case. We
> have separate BPF_PROBE_READ_USER() and BPF_CORE_READ_USER() for when
> bpf_probe_read_user() has to be used.

At least purely from the code perspective, BPF_PROBE_READ() seems to
delegate to bpf_probe_read(). The following therefore helps with this
test:

--- a/tools/lib/bpf/bpf_core_read.h
+++ b/tools/lib/bpf/bpf_core_read.h
@@ -364,7 +364,7 @@ enum bpf_enum_value_kind {
 
 /* Non-CO-RE variant of BPF_CORE_READ_INTO() */
 #define BPF_PROBE_READ_INTO(dst, src, a, ...) ({                     
\
-       ___core_read(bpf_probe_read, bpf_probe_read,                  
\
+       ___core_read(bpf_probe_read_kernel, bpf_probe_read_kernel,    
\
                     dst, (src), a, ##__VA_ARGS__)                    
\
 })
 
@@ -400,7 +400,7 @@ enum bpf_enum_value_kind {
 
 /* Non-CO-RE variant of BPF_CORE_READ_STR_INTO() */
 #define BPF_PROBE_READ_STR_INTO(dst, src, a, ...) ({                 
\
-       ___core_read(bpf_probe_read_str, bpf_probe_read,              
\
+       ___core_read(bpf_probe_read_kernel_str, bpf_probe_read_kernel,
\
                     dst, (src), a, ##__VA_ARGS__)                    
\
 })

but I'm not sure if there are backward compatibility concerns, or if
libbpf is supposed to rewrite this when
!ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE.

> > 
> > #82      get_stack_raw_tp:FAIL
> > get_stack_print_output:FAIL:user_stack corrupted user stack
> > Known issue:
> > We cannot reliably unwind userspace on s390x without DWARF.
> 
> like in principle, or frame pointers (or some equivalent) needs to be
> configured for this to work?
> 
> Asking also in the context of [0], where s390x was excluded. If there
> is actually a way to enable frame pointer-based stack unwinding on
> s390x, would be nice to hear from an expert.
> 
>   [0] https://pagure.io/fesco/issue/2923

For DWARFless unwinding we have -mbackchain (not to be confused with
-fno-omit-frame-pointer, which we also have, but which just hurts
performance without providing tangible benefits).
-mbackchain has a few problems though:

- It's not atomic. Here is a typical prologue with -mbackchain:

        1: stmg    %r11,%r15,88(%r15)  # save non-volatile registers
        2: lgr     %r14,%r15           # %r14 = sp
        3: lay     %r15,-160(%r15)     # sp -= 160
        4: stg     %r14,0(%r15)        # *(void **)sp = %r14

  The invariant here is that *(void **)%r15 is always a pointer to the
  next frame. This means that if we unwind from (4), we are totally
  broken. This does not happen if we unwind from any other instruction,
  but still.

- Unwinding from (1)-(3) is not particularly good either. PSW points to
  the callee, but R15 points to the caller's frame.

- Unwinding leaf functions is like the previous problem, but worse:
  they often do not establish a stack frame at all, so PSW and R15 are
  out of sync for the entire duration of the call.

Therefore .eh_frame-based unwinding is preferred, since it covers all
these corner cases and is therefore reliable. From what I understand,
adding .eh_frame unwinding to the kernel is not desirable. In an
internal discussion we had another idea though: would it be possible to
have an eBPF program that does .eh_frame parsing and unwinding? I
understand that it can be technically challenging at the moment, but
the end result would not be exploitable by crafting malicious
.eh_frame sections, won't loop endlessly, will have good performance,
etc.

> > #101     ksyms_module:FAIL
> > address of kernel function bpf_testmod_test_mod_kfunc is out of
> > range
> > Known issue:
> > Kernel and modules are too far away from each other on s390x.
> > 
> > #167     sk_assign:FAIL
> > Uses legacy map definitions in 'maps' section.
> 
> Hm.. assuming new enough iproute2, new-style .maps definition should
> be supported, right? Let's convert map definition?

Yep, that worked. Will include in v2.

> > #190     stacktrace_build_id:FAIL
> > Known issue:
> > We cannot reliably unwind userspace on s390x without DWARF.
> > 
> > #211     test_bpffs:FAIL
> > iterators.bpf.c is broken on s390x, it uses BPF_CORE_READ(),
> > shouldn't
> > there be BPF_CORE_READ_KERNEL()?
> 
> BPF_CORE_READ() is that, so must be something else
> 
> > 
> > #218     test_profiler:FAIL
> > A lot of BPF_PROBE_READ() usages.
> 
> ditto, something else
> 
> > 
> > #281     xdp_metadata:FAIL
> > See patch 24.
> > 
> > #284     xdp_synproxy:FAIL
> > Verifier error:
> > ; value = bpf_tcp_raw_gen_syncookie_ipv4(hdr->ipv4, hdr->tcp,
> > 281: (79) r1 = *(u64 *)(r10 -80)      ; R1_w=pkt(off=14,r=74,imm=0)
> > R10=fp0
> > 282: (bf) r2 = r8                     ;
> > R2_w=pkt(id=5,off=14,r=74,umax=60,var_off=(0x0; 0x3c))
> > R8=pkt(id=5,off=14,r=74,umax=60,var_off=(0x0; 0x3c))
> > 283: (79) r3 = *(u64 *)(r10 -104)     ;
> > R3_w=scalar(umax=60,var_off=(0x0; 0x3c)) R10=fp0
> > 284: (85) call bpf_tcp_raw_gen_syncookie_ipv4#204
> > invalid access to packet, off=14 size=0, R2(id=5,off=14,r=74)
> > R2 offset is outside of the packet
> 
> third arg to bpf_tcp_raw_gen_syncookie_ipv4() is defined as
> ARG_CONST_SIZE, so is required to be strictly positive, which doesn't
> seem to be "proven" to verifier. Please provided bigger log, it might
> another instance of needing to sprinkle barrier_var() around.
> 
> And maybe thinking about using ARG_CONST_SIZE_OR_ZERO instead of
> ARG_CONST_SIZE.

Here is the full log:

https://gist.github.com/iii-i/8e20100c33ab6f0dffb5e6e51d1330e8

Apparently we do indeed lose a constraint established by
if (hdr->tcp_len < sizeof(*hdr->tcp)). But the naive

--- a/tools/testing/selftests/bpf/progs/xdp_synproxy_kern.c
+++ b/tools/testing/selftests/bpf/progs/xdp_synproxy_kern.c
@@ -401,6 +401,7 @@ static __always_inline int tcp_dissect(void *data,
void *data_end,
        hdr->tcp_len = hdr->tcp->doff * 4;
        if (hdr->tcp_len < sizeof(*hdr->tcp))
                return XDP_DROP;
+       barrier_var(hdr->tcp_len);
 
        return XDP_TX;
 }
@@ -791,6 +792,7 @@ static __always_inline int syncookie_part2(void
*ctx, void *data, void *data_end
        hdr->tcp_len = hdr->tcp->doff * 4;
        if (hdr->tcp_len < sizeof(*hdr->tcp))
                return XDP_ABORTED;
+       barrier_var(hdr->tcp_len);
 
        return hdr->tcp->syn ? syncookie_handle_syn(hdr, ctx, data,
data_end, xdp) :
                               syncookie_handle_ack(hdr);

does not help.

> 
> > 
> > None of these seem to be due to the new changes.
> > 
> > Best regards,
> > Ilya
> > 
> > Ilya Leoshkevich (24):
> >   selftests/bpf: Fix liburandom_read.so linker error
> >   selftests/bpf: Fix symlink creation error
> >   selftests/bpf: Fix fexit_stress on s390x
> >   selftests/bpf: Fix trampoline_count on s390x
> >   selftests/bpf: Fix kfree_skb on s390x
> >   selftests/bpf: Set errno when urand_spawn() fails
> >   selftests/bpf: Fix decap_sanity_ns cleanup
> >   selftests/bpf: Fix verify_pkcs7_sig on s390x
> >   selftests/bpf: Fix xdp_do_redirect on s390x
> >   selftests/bpf: Fix cgrp_local_storage on s390x
> >   selftests/bpf: Check stack_mprotect() return value
> >   selftests/bpf: Increase SIZEOF_BPF_LOCAL_STORAGE_ELEM on s390x
> >   selftests/bpf: Add a sign-extension test for kfuncs
> >   selftests/bpf: Fix test_lsm on s390x
> >   selftests/bpf: Fix test_xdp_adjust_tail_grow2 on s390x
> >   selftests/bpf: Fix vmlinux test on s390x
> >   libbpf: Read usdt arg spec with bpf_probe_read_kernel()
> >   s390/bpf: Fix a typo in a comment
> >   s390/bpf: Add expoline to tail calls
> >   s390/bpf: Implement bpf_arch_text_poke()
> >   bpf: btf: Add BTF_FMODEL_SIGNED_ARG flag
> >   s390/bpf: Implement arch_prepare_bpf_trampoline()
> >   s390/bpf: Implement bpf_jit_supports_subprog_tailcalls()
> >   s390/bpf: Implement bpf_jit_supports_kfunc_call()
> > 
> >  arch/s390/net/bpf_jit_comp.c                  | 708
> > +++++++++++++++++-
> >  include/linux/bpf.h                           |   8 +
> >  include/linux/btf.h                           |  15 +-
> >  kernel/bpf/btf.c                              |  16 +-
> >  net/bpf/test_run.c                            |   9 +
> >  tools/lib/bpf/usdt.bpf.h                      |  33 +-
> >  tools/testing/selftests/bpf/Makefile          |   7 +-
> >  tools/testing/selftests/bpf/netcnt_common.h   |   6 +-
> >  .../selftests/bpf/prog_tests/bpf_cookie.c     |   6 +-
> >  .../bpf/prog_tests/cgrp_local_storage.c       |   2 +-
> >  .../selftests/bpf/prog_tests/decap_sanity.c   |   2 +-
> >  .../selftests/bpf/prog_tests/fexit_stress.c   |   6 +-
> >  .../selftests/bpf/prog_tests/kfree_skb.c      |   2 +-
> >  .../selftests/bpf/prog_tests/kfunc_call.c     |   1 +
> >  .../selftests/bpf/prog_tests/test_lsm.c       |   3 +-
> >  .../bpf/prog_tests/trampoline_count.c         |   4 +
> >  tools/testing/selftests/bpf/prog_tests/usdt.c |   1 +
> >  .../bpf/prog_tests/verify_pkcs7_sig.c         |   9 +
> >  .../bpf/prog_tests/xdp_adjust_tail.c          |   7 +-
> >  .../bpf/prog_tests/xdp_do_redirect.c          |   4 +
> >  .../selftests/bpf/progs/kfunc_call_test.c     |  18 +
> >  tools/testing/selftests/bpf/progs/lsm.c       |   7 +-
> >  .../bpf/progs/test_verify_pkcs7_sig.c         |  12 +-
> >  .../selftests/bpf/progs/test_vmlinux.c        |   4 +-
> >  .../bpf/progs/test_xdp_adjust_tail_grow.c     |   8 +-
> >  25 files changed, 816 insertions(+), 82 deletions(-)
> > 
> > --
> > 2.39.1
> > 

Re: [PATCH bpf-next 00/24] Support bpf trampoline for s390x

[Andrii Nakryiko]

On Fri, Jan 27, 2023 at 8:51 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Wed, 2023-01-25 at 16:45 -0800, Andrii Nakryiko wrote:
> > On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich <iii@linux.ibm.com>
> > wrote:
> > >
> > > Hi,
> > >
> > > This series implements poke, trampoline, kfunc, mixing subprogs and
> > > tailcalls, and fixes a number of tests on s390x.
> > >
> > > The following failures still remain:
> > >
> > > #52      core_read_macros:FAIL
> > > Uses BPF_PROBE_READ(), shouldn't there be BPF_PROBE_READ_KERNEL()?
> >
> > BPF_PROBE_READ(), similarly to BPF_CORE_READ() both use
> > bpf_probe_read_kernel() internally, as it's most common use case. We
> > have separate BPF_PROBE_READ_USER() and BPF_CORE_READ_USER() for when
> > bpf_probe_read_user() has to be used.
>
> At least purely from the code perspective, BPF_PROBE_READ() seems to
> delegate to bpf_probe_read(). The following therefore helps with this
> test:
>
> --- a/tools/lib/bpf/bpf_core_read.h
> +++ b/tools/lib/bpf/bpf_core_read.h
> @@ -364,7 +364,7 @@ enum bpf_enum_value_kind {
>
>  /* Non-CO-RE variant of BPF_CORE_READ_INTO() */
>  #define BPF_PROBE_READ_INTO(dst, src, a, ...) ({
> \
> -       ___core_read(bpf_probe_read, bpf_probe_read,
> \
> +       ___core_read(bpf_probe_read_kernel, bpf_probe_read_kernel,
> \
>                      dst, (src), a, ##__VA_ARGS__)
> \
>  })
>
> @@ -400,7 +400,7 @@ enum bpf_enum_value_kind {
>
>  /* Non-CO-RE variant of BPF_CORE_READ_STR_INTO() */
>  #define BPF_PROBE_READ_STR_INTO(dst, src, a, ...) ({
> \
> -       ___core_read(bpf_probe_read_str, bpf_probe_read,
> \
> +       ___core_read(bpf_probe_read_kernel_str, bpf_probe_read_kernel,
> \
>                      dst, (src), a, ##__VA_ARGS__)
> \
>  })
>
> but I'm not sure if there are backward compatibility concerns, or if
> libbpf is supposed to rewrite this when
> !ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE.

Oh, this is just a bug, it was an omission when we converted
BPF_CORE_READ to bpf_probe_read_kernel. If you look at bpf_core_read
definition, it is using bpf_probe_read_kernel, which is also the
intent for BPF_PROBE_READ. Let's fix this.

And there is no backwards compat concerns because libbpf will
automatically convert calls to bpf_probe_read_{kernel,user}[_str] to
bpf_probe_read[_str] if host kernel doesn't yet support kernel or user
specific variants.

>
> > >
> > > #82      get_stack_raw_tp:FAIL
> > > get_stack_print_output:FAIL:user_stack corrupted user stack
> > > Known issue:
> > > We cannot reliably unwind userspace on s390x without DWARF.
> >
> > like in principle, or frame pointers (or some equivalent) needs to be
> > configured for this to work?
> >
> > Asking also in the context of [0], where s390x was excluded. If there
> > is actually a way to enable frame pointer-based stack unwinding on
> > s390x, would be nice to hear from an expert.
> >
> >   [0] https://pagure.io/fesco/issue/2923
>
> For DWARFless unwinding we have -mbackchain (not to be confused with
> -fno-omit-frame-pointer, which we also have, but which just hurts
> performance without providing tangible benefits).
> -mbackchain has a few problems though:
>
> - It's not atomic. Here is a typical prologue with -mbackchain:
>
>         1: stmg    %r11,%r15,88(%r15)  # save non-volatile registers
>         2: lgr     %r14,%r15           # %r14 = sp
>         3: lay     %r15,-160(%r15)     # sp -= 160
>         4: stg     %r14,0(%r15)        # *(void **)sp = %r14
>
>   The invariant here is that *(void **)%r15 is always a pointer to the
>   next frame. This means that if we unwind from (4), we are totally
>   broken. This does not happen if we unwind from any other instruction,
>   but still.
>
> - Unwinding from (1)-(3) is not particularly good either. PSW points to
>   the callee, but R15 points to the caller's frame.
>
> - Unwinding leaf functions is like the previous problem, but worse:
>   they often do not establish a stack frame at all, so PSW and R15 are
>   out of sync for the entire duration of the call.
>
> Therefore .eh_frame-based unwinding is preferred, since it covers all
> these corner cases and is therefore reliable. From what I understand,
> adding .eh_frame unwinding to the kernel is not desirable. In an
> internal discussion we had another idea though: would it be possible to
> have an eBPF program that does .eh_frame parsing and unwinding? I
> understand that it can be technically challenging at the moment, but
> the end result would not be exploitable by crafting malicious
> .eh_frame sections, won't loop endlessly, will have good performance,
> etc.

Thanks for details. This was all discussed at length in Fedora
-fno-omit-frame-pointer discussion I linked above, so no real need to
go over this again. .eh_frame-based unwinding on BPF side is possible,
but only for processes that you knew about (and preprocessed) before
you started profiling session. Pre-processing is memory and
cpu-intensive operation on busy systems, and they will miss any
processes started during profiling. So as a general approach for
system-wide profiling it leaves a lot to be desired.

Should we enable -mbackchain in our CI for s390x to be able to capture
stack traces (even if on some instructions they might be incomplete or
outright broken)?

>
> > > #101     ksyms_module:FAIL
> > > address of kernel function bpf_testmod_test_mod_kfunc is out of
> > > range
> > > Known issue:
> > > Kernel and modules are too far away from each other on s390x.
> > >
> > > #167     sk_assign:FAIL
> > > Uses legacy map definitions in 'maps' section.
> >
> > Hm.. assuming new enough iproute2, new-style .maps definition should
> > be supported, right? Let's convert map definition?
>
> Yep, that worked. Will include in v2.

Nice.

>
> > > #190     stacktrace_build_id:FAIL
> > > Known issue:
> > > We cannot reliably unwind userspace on s390x without DWARF.
> > >
> > > #211     test_bpffs:FAIL
> > > iterators.bpf.c is broken on s390x, it uses BPF_CORE_READ(),
> > > shouldn't
> > > there be BPF_CORE_READ_KERNEL()?
> >
> > BPF_CORE_READ() is that, so must be something else
> >
> > >
> > > #218     test_profiler:FAIL
> > > A lot of BPF_PROBE_READ() usages.
> >
> > ditto, something else
> >
> > >
> > > #281     xdp_metadata:FAIL
> > > See patch 24.
> > >
> > > #284     xdp_synproxy:FAIL
> > > Verifier error:
> > > ; value = bpf_tcp_raw_gen_syncookie_ipv4(hdr->ipv4, hdr->tcp,
> > > 281: (79) r1 = *(u64 *)(r10 -80)      ; R1_w=pkt(off=14,r=74,imm=0)
> > > R10=fp0
> > > 282: (bf) r2 = r8                     ;
> > > R2_w=pkt(id=5,off=14,r=74,umax=60,var_off=(0x0; 0x3c))
> > > R8=pkt(id=5,off=14,r=74,umax=60,var_off=(0x0; 0x3c))
> > > 283: (79) r3 = *(u64 *)(r10 -104)     ;
> > > R3_w=scalar(umax=60,var_off=(0x0; 0x3c)) R10=fp0
> > > 284: (85) call bpf_tcp_raw_gen_syncookie_ipv4#204
> > > invalid access to packet, off=14 size=0, R2(id=5,off=14,r=74)
> > > R2 offset is outside of the packet
> >
> > third arg to bpf_tcp_raw_gen_syncookie_ipv4() is defined as
> > ARG_CONST_SIZE, so is required to be strictly positive, which doesn't
> > seem to be "proven" to verifier. Please provided bigger log, it might
> > another instance of needing to sprinkle barrier_var() around.
> >
> > And maybe thinking about using ARG_CONST_SIZE_OR_ZERO instead of
> > ARG_CONST_SIZE.
>
> Here is the full log:
>
> https://gist.github.com/iii-i/8e20100c33ab6f0dffb5e6e51d1330e8
>
> Apparently we do indeed lose a constraint established by
> if (hdr->tcp_len < sizeof(*hdr->tcp)). But the naive

The test is too big and unfamiliar for me to figure this out. And the
problem is not upper bound, but lower bound. hdr->tcp_len is not
proven to be strictly greater than zero, which is what verifier
complains about. Not sure how it works on other arches right now.


But I see that bpf_csum_diff defines size arguments as
ARG_CONST_SIZE_OR_ZERO while bpf_tcp_raw_gen_syncookie_ipv4 has
ARG_CONST_SIZE. I generally found ARG_CONST_SIZE way too problematic
in practice, I'd say we should change it to ARG_CONST_SIZE_OR_ZERO.

>
> --- a/tools/testing/selftests/bpf/progs/xdp_synproxy_kern.c
> +++ b/tools/testing/selftests/bpf/progs/xdp_synproxy_kern.c
> @@ -401,6 +401,7 @@ static __always_inline int tcp_dissect(void *data,
> void *data_end,
>         hdr->tcp_len = hdr->tcp->doff * 4;
>         if (hdr->tcp_len < sizeof(*hdr->tcp))
>                 return XDP_DROP;
> +       barrier_var(hdr->tcp_len);
>
>         return XDP_TX;
>  }
> @@ -791,6 +792,7 @@ static __always_inline int syncookie_part2(void
> *ctx, void *data, void *data_end
>         hdr->tcp_len = hdr->tcp->doff * 4;
>         if (hdr->tcp_len < sizeof(*hdr->tcp))
>                 return XDP_ABORTED;
> +       barrier_var(hdr->tcp_len);
>
>         return hdr->tcp->syn ? syncookie_handle_syn(hdr, ctx, data,
> data_end, xdp) :
>                                syncookie_handle_ack(hdr);
>
> does not help.
>
> >
> > >
> > > None of these seem to be due to the new changes.
> > >
> > > Best regards,
> > > Ilya
> > >
> > > Ilya Leoshkevich (24):
> > >   selftests/bpf: Fix liburandom_read.so linker error
> > >   selftests/bpf: Fix symlink creation error
> > >   selftests/bpf: Fix fexit_stress on s390x
> > >   selftests/bpf: Fix trampoline_count on s390x
> > >   selftests/bpf: Fix kfree_skb on s390x
> > >   selftests/bpf: Set errno when urand_spawn() fails
> > >   selftests/bpf: Fix decap_sanity_ns cleanup
> > >   selftests/bpf: Fix verify_pkcs7_sig on s390x
> > >   selftests/bpf: Fix xdp_do_redirect on s390x
> > >   selftests/bpf: Fix cgrp_local_storage on s390x
> > >   selftests/bpf: Check stack_mprotect() return value
> > >   selftests/bpf: Increase SIZEOF_BPF_LOCAL_STORAGE_ELEM on s390x
> > >   selftests/bpf: Add a sign-extension test for kfuncs
> > >   selftests/bpf: Fix test_lsm on s390x
> > >   selftests/bpf: Fix test_xdp_adjust_tail_grow2 on s390x
> > >   selftests/bpf: Fix vmlinux test on s390x
> > >   libbpf: Read usdt arg spec with bpf_probe_read_kernel()
> > >   s390/bpf: Fix a typo in a comment
> > >   s390/bpf: Add expoline to tail calls
> > >   s390/bpf: Implement bpf_arch_text_poke()
> > >   bpf: btf: Add BTF_FMODEL_SIGNED_ARG flag
> > >   s390/bpf: Implement arch_prepare_bpf_trampoline()
> > >   s390/bpf: Implement bpf_jit_supports_subprog_tailcalls()
> > >   s390/bpf: Implement bpf_jit_supports_kfunc_call()
> > >
> > >  arch/s390/net/bpf_jit_comp.c                  | 708
> > > +++++++++++++++++-
> > >  include/linux/bpf.h                           |   8 +
> > >  include/linux/btf.h                           |  15 +-
> > >  kernel/bpf/btf.c                              |  16 +-
> > >  net/bpf/test_run.c                            |   9 +
> > >  tools/lib/bpf/usdt.bpf.h                      |  33 +-
> > >  tools/testing/selftests/bpf/Makefile          |   7 +-
> > >  tools/testing/selftests/bpf/netcnt_common.h   |   6 +-
> > >  .../selftests/bpf/prog_tests/bpf_cookie.c     |   6 +-
> > >  .../bpf/prog_tests/cgrp_local_storage.c       |   2 +-
> > >  .../selftests/bpf/prog_tests/decap_sanity.c   |   2 +-
> > >  .../selftests/bpf/prog_tests/fexit_stress.c   |   6 +-
> > >  .../selftests/bpf/prog_tests/kfree_skb.c      |   2 +-
> > >  .../selftests/bpf/prog_tests/kfunc_call.c     |   1 +
> > >  .../selftests/bpf/prog_tests/test_lsm.c       |   3 +-
> > >  .../bpf/prog_tests/trampoline_count.c         |   4 +
> > >  tools/testing/selftests/bpf/prog_tests/usdt.c |   1 +
> > >  .../bpf/prog_tests/verify_pkcs7_sig.c         |   9 +
> > >  .../bpf/prog_tests/xdp_adjust_tail.c          |   7 +-
> > >  .../bpf/prog_tests/xdp_do_redirect.c          |   4 +
> > >  .../selftests/bpf/progs/kfunc_call_test.c     |  18 +
> > >  tools/testing/selftests/bpf/progs/lsm.c       |   7 +-
> > >  .../bpf/progs/test_verify_pkcs7_sig.c         |  12 +-
> > >  .../selftests/bpf/progs/test_vmlinux.c        |   4 +-
> > >  .../bpf/progs/test_xdp_adjust_tail_grow.c     |   8 +-
> > >  25 files changed, 816 insertions(+), 82 deletions(-)
> > >
> > > --
> > > 2.39.1
> > >
>

Re: [PATCH bpf-next 00/24] Support bpf trampoline for s390x

[Ilya Leoshkevich]

On Fri, 2023-01-27 at 09:24 -0800, Andrii Nakryiko wrote:
> On Fri, Jan 27, 2023 at 8:51 AM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > On Wed, 2023-01-25 at 16:45 -0800, Andrii Nakryiko wrote:
> > > On Wed, Jan 25, 2023 at 1:39 PM Ilya Leoshkevich
> > > <iii@linux.ibm.com>
> > > wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > This series implements poke, trampoline, kfunc, mixing subprogs
> > > > and
> > > > tailcalls, and fixes a number of tests on s390x.
> > > > 
> > > > The following failures still remain:
> > > > 
> > > > #52      core_read_macros:FAIL
> > > > Uses BPF_PROBE_READ(), shouldn't there be
> > > > BPF_PROBE_READ_KERNEL()?
> > > 
> > > BPF_PROBE_READ(), similarly to BPF_CORE_READ() both use
> > > bpf_probe_read_kernel() internally, as it's most common use case.
> > > We
> > > have separate BPF_PROBE_READ_USER() and BPF_CORE_READ_USER() for
> > > when
> > > bpf_probe_read_user() has to be used.
> > 
> > At least purely from the code perspective, BPF_PROBE_READ() seems
> > to
> > delegate to bpf_probe_read(). The following therefore helps with
> > this
> > test:
> > 
> > --- a/tools/lib/bpf/bpf_core_read.h
> > +++ b/tools/lib/bpf/bpf_core_read.h
> > @@ -364,7 +364,7 @@ enum bpf_enum_value_kind {
> > 
> >  /* Non-CO-RE variant of BPF_CORE_READ_INTO() */
> >  #define BPF_PROBE_READ_INTO(dst, src, a, ...) ({
> > \
> > -       ___core_read(bpf_probe_read, bpf_probe_read,
> > \
> > +       ___core_read(bpf_probe_read_kernel, bpf_probe_read_kernel,
> > \
> >                      dst, (src), a, ##__VA_ARGS__)
> > \
> >  })
> > 
> > @@ -400,7 +400,7 @@ enum bpf_enum_value_kind {
> > 
> >  /* Non-CO-RE variant of BPF_CORE_READ_STR_INTO() */
> >  #define BPF_PROBE_READ_STR_INTO(dst, src, a, ...) ({
> > \
> > -       ___core_read(bpf_probe_read_str, bpf_probe_read,
> > \
> > +       ___core_read(bpf_probe_read_kernel_str,
> > bpf_probe_read_kernel,
> > \
> >                      dst, (src), a, ##__VA_ARGS__)
> > \
> >  })
> > 
> > but I'm not sure if there are backward compatibility concerns, or
> > if
> > libbpf is supposed to rewrite this when
> > !ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE.
> 
> Oh, this is just a bug, it was an omission when we converted
> BPF_CORE_READ to bpf_probe_read_kernel. If you look at bpf_core_read
> definition, it is using bpf_probe_read_kernel, which is also the
> intent for BPF_PROBE_READ. Let's fix this.
> 
> And there is no backwards compat concerns because libbpf will
> automatically convert calls to bpf_probe_read_{kernel,user}[_str] to
> bpf_probe_read[_str] if host kernel doesn't yet support kernel or
> user
> specific variants.

Thanks for confirming! I will include the fix in v2.

> > > > #82      get_stack_raw_tp:FAIL
> > > > get_stack_print_output:FAIL:user_stack corrupted user stack
> > > > Known issue:
> > > > We cannot reliably unwind userspace on s390x without DWARF.
> > > 
> > > like in principle, or frame pointers (or some equivalent) needs
> > > to be
> > > configured for this to work?
> > > 
> > > Asking also in the context of [0], where s390x was excluded. If
> > > there
> > > is actually a way to enable frame pointer-based stack unwinding
> > > on
> > > s390x, would be nice to hear from an expert.
> > > 
> > >   [0] https://pagure.io/fesco/issue/2923
> > 
> > For DWARFless unwinding we have -mbackchain (not to be confused
> > with
> > -fno-omit-frame-pointer, which we also have, but which just hurts
> > performance without providing tangible benefits).
> > -mbackchain has a few problems though:
> > 
> > - It's not atomic. Here is a typical prologue with -mbackchain:
> > 
> >         1: stmg    %r11,%r15,88(%r15)  # save non-volatile
> > registers
> >         2: lgr     %r14,%r15           # %r14 = sp
> >         3: lay     %r15,-160(%r15)     # sp -= 160
> >         4: stg     %r14,0(%r15)        # *(void **)sp = %r14
> > 
> >   The invariant here is that *(void **)%r15 is always a pointer to
> > the
> >   next frame. This means that if we unwind from (4), we are totally
> >   broken. This does not happen if we unwind from any other
> > instruction,
> >   but still.
> > 
> > - Unwinding from (1)-(3) is not particularly good either. PSW
> > points to
> >   the callee, but R15 points to the caller's frame.
> > 
> > - Unwinding leaf functions is like the previous problem, but worse:
> >   they often do not establish a stack frame at all, so PSW and R15
> > are
> >   out of sync for the entire duration of the call.
> > 
> > Therefore .eh_frame-based unwinding is preferred, since it covers
> > all
> > these corner cases and is therefore reliable. From what I
> > understand,
> > adding .eh_frame unwinding to the kernel is not desirable. In an
> > internal discussion we had another idea though: would it be
> > possible to
> > have an eBPF program that does .eh_frame parsing and unwinding? I
> > understand that it can be technically challenging at the moment,
> > but
> > the end result would not be exploitable by crafting malicious
> > .eh_frame sections, won't loop endlessly, will have good
> > performance,
> > etc.
> 
> Thanks for details. This was all discussed at length in Fedora
> -fno-omit-frame-pointer discussion I linked above, so no real need to
> go over this again. .eh_frame-based unwinding on BPF side is
> possible,
> but only for processes that you knew about (and preprocessed) before
> you started profiling session. Pre-processing is memory and
> cpu-intensive operation on busy systems, and they will miss any
> processes started during profiling. So as a general approach for
> system-wide profiling it leaves a lot to be desired.

Thanks for the explanation, I'll read the thread and come back if I
get some new ideas not listed here.

> Should we enable -mbackchain in our CI for s390x to be able to
> capture
> stack traces (even if on some instructions they might be incomplete
> or
> outright broken)?

Let's do it, I don't have anything against this.

[...]
> > > 
> 

> > Here is the full log:
> > 
> > https://gist.github.com/iii-i/8e20100c33ab6f0dffb5e6e51d1330e8
> > 
> > Apparently we do indeed lose a constraint established by
> > if (hdr->tcp_len < sizeof(*hdr->tcp)). But the naive
> 
> The test is too big and unfamiliar for me to figure this out. And the
> problem is not upper bound, but lower bound. hdr->tcp_len is not
> proven to be strictly greater than zero, which is what verifier
> complains about. Not sure how it works on other arches right now.
> 
> 
> But I see that bpf_csum_diff defines size arguments as
> ARG_CONST_SIZE_OR_ZERO while bpf_tcp_raw_gen_syncookie_ipv4 has
> ARG_CONST_SIZE. I generally found ARG_CONST_SIZE way too problematic
> in practice, I'd say we should change it to ARG_CONST_SIZE_OR_ZERO.

Yes, this helps, and doesn't seem to introduce issues, since the 
minimum length is enforced inside this function anyway. I will include
the change for bpf_tcp_raw_gen_syncookie_ipv{4,6} in v2; I guess some
other functions may benefit from this as well, but this is outside the
scope of this series.

[...]