* [PATCH bpf 1/2] s390/bpf: Let arch_prepare_bpf_trampoline return program size
2023-09-19 6:02 [PATCH bpf 0/2] s390/bpf: Fix arch_prepare_bpf_trampoline Song Liu
@ 2023-09-19 6:02 ` Song Liu
2023-09-19 9:04 ` Ilya Leoshkevich
2023-09-19 6:02 ` [PATCH bpf 2/2] selftests/bpf: Check bpf_cubic_acked() is called via struct_ops Song Liu
2023-09-19 10:10 ` [PATCH bpf 0/2] s390/bpf: Fix arch_prepare_bpf_trampoline patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Song Liu @ 2023-09-19 6:02 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, andrii, martin.lau, kernel-team, iii, Song Liu
arch_prepare_bpf_trampoline() for s390 currently returns 0 on success. This
is not a problem for regular trampoline. However, struct_ops relies on the
return value to advance "image" pointer:
bpf_struct_ops_map_update_elem() {
...
for_each_member(i, t, member) {
...
err = bpf_struct_ops_prepare_trampoline();
...
image += err;
}
}
When arch_prepare_bpf_trampoline returns 0 on success, all members of the
struct_ops will point to the same trampoline (the last one).
Fix this by returning the program size in arch_prepare_bpf_trampoline (on
success). This is the same behavior as other architectures.
Signed-off-by: Song Liu <song@kernel.org>
---
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 de2fb12120d2..2861e3360aff 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -2513,7 +2513,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
return -E2BIG;
}
- return ret;
+ return tjit.common.prg;
}
bool bpf_jit_supports_subprog_tailcalls(void)
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf 1/2] s390/bpf: Let arch_prepare_bpf_trampoline return program size
2023-09-19 6:02 ` [PATCH bpf 1/2] s390/bpf: Let arch_prepare_bpf_trampoline return program size Song Liu
@ 2023-09-19 9:04 ` Ilya Leoshkevich
0 siblings, 0 replies; 6+ messages in thread
From: Ilya Leoshkevich @ 2023-09-19 9:04 UTC (permalink / raw)
To: Song Liu, bpf; +Cc: ast, daniel, andrii, martin.lau, kernel-team
On Mon, 2023-09-18 at 23:02 -0700, Song Liu wrote:
> arch_prepare_bpf_trampoline() for s390 currently returns 0 on
> success. This
> is not a problem for regular trampoline. However, struct_ops relies
> on the
> return value to advance "image" pointer:
>
> bpf_struct_ops_map_update_elem() {
> ...
> for_each_member(i, t, member) {
> ...
> err = bpf_struct_ops_prepare_trampoline();
> ...
> image += err;
> }
> }
>
> When arch_prepare_bpf_trampoline returns 0 on success, all members of
> the
> struct_ops will point to the same trampoline (the last one).
>
> Fix this by returning the program size in arch_prepare_bpf_trampoline
> (on
> success). This is the same behavior as other architectures.
>
> Signed-off-by: Song Liu <song@kernel.org>
> ---
> arch/s390/net/bpf_jit_comp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks!
Fixes: 528eb2cb87bc ("s390/bpf: Implement
arch_prepare_bpf_trampoline()")
Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf 2/2] selftests/bpf: Check bpf_cubic_acked() is called via struct_ops
2023-09-19 6:02 [PATCH bpf 0/2] s390/bpf: Fix arch_prepare_bpf_trampoline Song Liu
2023-09-19 6:02 ` [PATCH bpf 1/2] s390/bpf: Let arch_prepare_bpf_trampoline return program size Song Liu
@ 2023-09-19 6:02 ` Song Liu
2023-09-19 9:05 ` Ilya Leoshkevich
2023-09-19 10:10 ` [PATCH bpf 0/2] s390/bpf: Fix arch_prepare_bpf_trampoline patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Song Liu @ 2023-09-19 6:02 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, kernel-team, iii, Song Liu,
Martin KaFai Lau
Test bpf_tcp_ca (in test_progs) checks multiple tcp_congestion_ops.
However, there isn't a test that verifies functions in the
tcp_congestion_ops is actually called. Add a check to verify that
bpf_cubic_acked is actually called during the test.
Suggested-by: Martin KaFai Lau <martin.lau@kernel.org>
Signed-off-by: Song Liu <song@kernel.org>
---
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c | 2 ++
tools/testing/selftests/bpf/progs/bpf_cubic.c | 3 +++
2 files changed, 5 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
index a53c254c6058..4aabeaa525d4 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
@@ -185,6 +185,8 @@ static void test_cubic(void)
do_test("bpf_cubic", NULL);
+ ASSERT_EQ(cubic_skel->bss->bpf_cubic_acked_called, 1, "pkts_acked called");
+
bpf_link__destroy(link);
bpf_cubic__destroy(cubic_skel);
}
diff --git a/tools/testing/selftests/bpf/progs/bpf_cubic.c b/tools/testing/selftests/bpf/progs/bpf_cubic.c
index d9660e7200e2..c997e3e3d3fb 100644
--- a/tools/testing/selftests/bpf/progs/bpf_cubic.c
+++ b/tools/testing/selftests/bpf/progs/bpf_cubic.c
@@ -490,6 +490,8 @@ static __always_inline void hystart_update(struct sock *sk, __u32 delay)
}
}
+int bpf_cubic_acked_called = 0;
+
void BPF_STRUCT_OPS(bpf_cubic_acked, struct sock *sk,
const struct ack_sample *sample)
{
@@ -497,6 +499,7 @@ void BPF_STRUCT_OPS(bpf_cubic_acked, struct sock *sk,
struct bictcp *ca = inet_csk_ca(sk);
__u32 delay;
+ bpf_cubic_acked_called = 1;
/* Some calls are for duplicates without timetamps */
if (sample->rtt_us < 0)
return;
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf 2/2] selftests/bpf: Check bpf_cubic_acked() is called via struct_ops
2023-09-19 6:02 ` [PATCH bpf 2/2] selftests/bpf: Check bpf_cubic_acked() is called via struct_ops Song Liu
@ 2023-09-19 9:05 ` Ilya Leoshkevich
0 siblings, 0 replies; 6+ messages in thread
From: Ilya Leoshkevich @ 2023-09-19 9:05 UTC (permalink / raw)
To: Song Liu, bpf
Cc: ast, daniel, andrii, martin.lau, kernel-team, Martin KaFai Lau
On Mon, 2023-09-18 at 23:02 -0700, Song Liu wrote:
> Test bpf_tcp_ca (in test_progs) checks multiple tcp_congestion_ops.
> However, there isn't a test that verifies functions in the
> tcp_congestion_ops is actually called. Add a check to verify that
> bpf_cubic_acked is actually called during the test.
>
> Suggested-by: Martin KaFai Lau <martin.lau@kernel.org>
> Signed-off-by: Song Liu <song@kernel.org>
> ---
> tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c | 2 ++
> tools/testing/selftests/bpf/progs/bpf_cubic.c | 3 +++
> 2 files changed, 5 insertions(+)
Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf 0/2] s390/bpf: Fix arch_prepare_bpf_trampoline
2023-09-19 6:02 [PATCH bpf 0/2] s390/bpf: Fix arch_prepare_bpf_trampoline Song Liu
2023-09-19 6:02 ` [PATCH bpf 1/2] s390/bpf: Let arch_prepare_bpf_trampoline return program size Song Liu
2023-09-19 6:02 ` [PATCH bpf 2/2] selftests/bpf: Check bpf_cubic_acked() is called via struct_ops Song Liu
@ 2023-09-19 10:10 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-09-19 10:10 UTC (permalink / raw)
To: Song Liu; +Cc: bpf, ast, daniel, andrii, martin.lau, kernel-team, iii
Hello:
This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Mon, 18 Sep 2023 23:02:56 -0700 you wrote:
> While working on trampoline, I found s390's arch_prepare_bpf_trampoline
> returns 0 on success, which breaks struct_ops. However, the CI doesn't
> catch this issue. Turns out test_progs:bpf_tcp_ca doesn't really test
> members of a struct_ops are actually called via the trampolines.
>
> 1/2 fixes arch_prepare_bpf_trampoline for s390.
> 2/2 adds a check to test_progs:bpf_tcp_ca to verify bpf_cubic_acked() is
> indeed called by the trampoline. Without 1/2, this check would fail on
> s390.
>
> [...]
Here is the summary with links:
- [bpf,1/2] s390/bpf: Let arch_prepare_bpf_trampoline return program size
https://git.kernel.org/bpf/bpf/c/cf094baa3e0f
- [bpf,2/2] selftests/bpf: Check bpf_cubic_acked() is called via struct_ops
https://git.kernel.org/bpf/bpf/c/48f5e7d3f730
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread