[v2,bpf-next,9/9] selftests/bpf: add arg:ctx cases to test_global_funcs tests

Commit Message

Andrii Nakryiko Jan. 2, 2024, 7 p.m. UTC

Add a few extra cases of global funcs with context arguments. This time
rely on "arg:ctx" decl_tag (__arg_ctx macro), but put it next to
"classic" cases where context argument has to be of an exact type that
BPF verifier expects (e.g., bpf_user_pt_regs_t for kprobe/uprobe).

Colocating all these cases separately from other global func args that
rely on arg:xxx decl tags (in verifier_global_subprogs.c) allows for
simpler backwards compatibility testing on old kernels. All the cases in
test_global_func_ctx_args.c are supposed to work on older kernels, which
was manually validated during development.

Acked-by: Jiri Olsa <[e-mail]>
Signed-off-by: Andrii Nakryiko <[e-mail]>
---
 .../bpf/progs/test_global_func_ctx_args.c     | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)

Comments

Eduard Zingerman Jan. 3, 2024, 8:57 p.m. UTC | #1

On Tue, 2024-01-02 at 11:00 -0800, Andrii Nakryiko wrote:
> Add a few extra cases of global funcs with context arguments. This time
> rely on "arg:ctx" decl_tag (__arg_ctx macro), but put it next to
> "classic" cases where context argument has to be of an exact type that
> BPF verifier expects (e.g., bpf_user_pt_regs_t for kprobe/uprobe).
> 
> Colocating all these cases separately from other global func args that
> rely on arg:xxx decl tags (in verifier_global_subprogs.c) allows for
> simpler backwards compatibility testing on old kernels. All the cases in
> test_global_func_ctx_args.c are supposed to work on older kernels, which
> was manually validated during development.
> 
> Acked-by: Jiri Olsa <[e-mail]>
> Signed-off-by: Andrii Nakryiko <[e-mail]>
> ---

At first I thought that we would need to add a new CI job that would
run these tests for some older kernel version.

However, the transformation of the sub-program parameters happens
unconditionally. So it should be possible to read BTF for some of the
programs after they are loaded and check if transformation is applied
as expected. Thus allowing to check __arg_ctx handling on libbpf side
w/o the need to run on old kernel.
I think it's worth it to add such test, wdyt?

Andrii Nakryiko Jan. 3, 2024, 11:17 p.m. UTC | #2

On Wed, Jan 3, 2024 at 12:57 PM Eduard Zingerman <[e-mail]> wrote:
>
> On Tue, 2024-01-02 at 11:00 -0800, Andrii Nakryiko wrote:
> > Add a few extra cases of global funcs with context arguments. This time
> > rely on "arg:ctx" decl_tag (__arg_ctx macro), but put it next to
> > "classic" cases where context argument has to be of an exact type that
> > BPF verifier expects (e.g., bpf_user_pt_regs_t for kprobe/uprobe).
> >
> > Colocating all these cases separately from other global func args that
> > rely on arg:xxx decl tags (in verifier_global_subprogs.c) allows for
> > simpler backwards compatibility testing on old kernels. All the cases in
> > test_global_func_ctx_args.c are supposed to work on older kernels, which
> > was manually validated during development.
> >
> > Acked-by: Jiri Olsa <[e-mail]>
> > Signed-off-by: Andrii Nakryiko <[e-mail]>
> > ---
>
> At first I thought that we would need to add a new CI job that would
> run these tests for some older kernel version.

libbpf CI will test this on 5.15 kernel, so we will have regression tests

>
> However, the transformation of the sub-program parameters happens
> unconditionally. So it should be possible to read BTF for some of the
> programs after they are loaded and check if transformation is applied
> as expected. Thus allowing to check __arg_ctx handling on libbpf side
> w/o the need to run on old kernel.

Yes, but it's bpf_prog_info to get func_info (actually two calls due
to how API is), parse func_info (pain without libbpf internal helpers
from libbpf_internal.h, and with it's more coupling) to find subprog's
func BTF ID and then check BTF.

It's so painful that I don't think it's worth it given we'll test this
in libbpf CI (and I did manual check locally as well).

Also, nothing actually prevents us from not doing this if the kernel
supports __arg_ctx natively, which is just a painful feature detector
to write, using low-level APIs, which is why I decided that it's
simpler to just do this unconditionally.

> I think it's worth it to add such test, wdyt?
>

I feel like slacking and not adding this :) This will definitely fail
in libbpf CI, if it's wrong.

Eduard Zingerman Jan. 3, 2024, 11:51 p.m. UTC | #3

On Wed, 2024-01-03 at 15:17 -0800, Andrii Nakryiko wrote:
[...]
> > However, the transformation of the sub-program parameters happens
> > unconditionally. So it should be possible to read BTF for some of the
> > programs after they are loaded and check if transformation is applied
> > as expected. Thus allowing to check __arg_ctx handling on libbpf side
> > w/o the need to run on old kernel.
> 
> Yes, but it's bpf_prog_info to get func_info (actually two calls due
> to how API is), parse func_info (pain without libbpf internal helpers
> from libbpf_internal.h, and with it's more coupling) to find subprog's
> func BTF ID and then check BTF.
> 
> It's so painful that I don't think it's worth it given we'll test this
> in libbpf CI (and I did manual check locally as well).
> 
> Also, nothing actually prevents us from not doing this if the kernel
> supports __arg_ctx natively, which is just a painful feature detector
> to write, using low-level APIs, which is why I decided that it's
> simpler to just do this unconditionally.

I agree that there is no need for feature detection in this case.

> > I think it's worth it to add such test, wdyt?
> > 
> 
> I feel like slacking and not adding this :) This will definitely fail
> in libbpf CI, if it's wrong.

Very few people look at libbpf CI results and those results would be
available only after sync.

Idk, I think that some form of testing is necessary for kernel CI.
Either this, or an additional job that executes selected set of tests
on old kernel.

Andrii Nakryiko Jan. 4, 2024, 12:26 a.m. UTC | #4

On Wed, Jan 3, 2024 at 3:51 PM Eduard Zingerman <[e-mail]> wrote:
>
> On Wed, 2024-01-03 at 15:17 -0800, Andrii Nakryiko wrote:
> [...]
> > > However, the transformation of the sub-program parameters happens
> > > unconditionally. So it should be possible to read BTF for some of the
> > > programs after they are loaded and check if transformation is applied
> > > as expected. Thus allowing to check __arg_ctx handling on libbpf side
> > > w/o the need to run on old kernel.
> >
> > Yes, but it's bpf_prog_info to get func_info (actually two calls due
> > to how API is), parse func_info (pain without libbpf internal helpers
> > from libbpf_internal.h, and with it's more coupling) to find subprog's
> > func BTF ID and then check BTF.
> >
> > It's so painful that I don't think it's worth it given we'll test this
> > in libbpf CI (and I did manual check locally as well).
> >
> > Also, nothing actually prevents us from not doing this if the kernel
> > supports __arg_ctx natively, which is just a painful feature detector
> > to write, using low-level APIs, which is why I decided that it's
> > simpler to just do this unconditionally.
>
> I agree that there is no need for feature detection in this case.
>

ok

> > > I think it's worth it to add such test, wdyt?
> > >
> >
> > I feel like slacking and not adding this :) This will definitely fail
> > in libbpf CI, if it's wrong.
>
> Very few people look at libbpf CI results and those results would be
> available only after sync.
>
> Idk, I think that some form of testing is necessary for kernel CI.
> Either this, or an additional job that executes selected set of tests
> on old kernel.

Alright, I'll add a test then.

Eduard Zingerman Jan. 4, 2024, 12:28 a.m. UTC | #5

On Wed, 2024-01-03 at 16:26 -0800, Andrii Nakryiko wrote:
[...]
> > > > I think it's worth it to add such test, wdyt?
> > > > 
> > > 
> > > I feel like slacking and not adding this :) This will definitely fail
> > > in libbpf CI, if it's wrong.
> > 
> > Very few people look at libbpf CI results and those results would be
> > available only after sync.
> > 
> > Idk, I think that some form of testing is necessary for kernel CI.
> > Either this, or an additional job that executes selected set of tests
> > on old kernel.
> 
> Alright, I'll add a test then.

Thank you.

Patch

diff --git a/tools/testing/selftests/bpf/progs/test_global_func_ctx_args.c b/tools/testing/selftests/bpf/progs/test_global_func_ctx_args.c
index 7faa8eef0598..9a06e5eb1fbe 100644
--- a/tools/testing/selftests/bpf/progs/test_global_func_ctx_args.c
+++ b/tools/testing/selftests/bpf/progs/test_global_func_ctx_args.c
@@ -102,3 +102,52 @@  int perf_event_ctx(void *ctx)
 {
 	return perf_event_ctx_subprog(ctx);
 }
+
+/* this global subprog can be now called from many types of entry progs, each
+ * with different context type
+ */
+__weak int subprog_ctx_tag(void *ctx __arg_ctx)
+{
+	return bpf_get_stack(ctx, stack, sizeof(stack), 0);
+}
+
+struct my_struct { int x; };
+
+__weak int subprog_multi_ctx_tags(void *ctx1 __arg_ctx,
+				  struct my_struct *mem,
+				  void *ctx2 __arg_ctx)
+{
+	if (!mem)
+		return 0;
+
+	return bpf_get_stack(ctx1, stack, sizeof(stack), 0) +
+	       mem->x +
+	       bpf_get_stack(ctx2, stack, sizeof(stack), 0);
+}
+
+SEC("?raw_tp")
+__success __log_level(2)
+int arg_tag_ctx_raw_tp(void *ctx)
+{
+	struct my_struct x = { .x = 123 };
+
+	return subprog_ctx_tag(ctx) + subprog_multi_ctx_tags(ctx, &x, ctx);
+}
+
+SEC("?perf_event")
+__success __log_level(2)
+int arg_tag_ctx_perf(void *ctx)
+{
+	struct my_struct x = { .x = 123 };
+
+	return subprog_ctx_tag(ctx) + subprog_multi_ctx_tags(ctx, &x, ctx);
+}
+
+SEC("?kprobe")
+__success __log_level(2)
+int arg_tag_ctx_kprobe(void *ctx)
+{
+	struct my_struct x = { .x = 123 };
+
+	return subprog_ctx_tag(ctx) + subprog_multi_ctx_tags(ctx, &x, ctx);
+}