On Thu, Jan 4, 2024 at 10:37 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Wed, Jan 3, 2024 at 9:39 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Wed, Jan 3, 2024 at 5:39 PM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> > >
> > > This limitation was the reason to add btf_decl_tag("arg:ctx"), making
> > > the actual argument type not important, so that user can just define
> > > "generic" signature:
> > >
> > > __noinline int global_subprog(void *ctx __arg_ctx) { ... }
> >
> > I still think that this __arg_ctx only makes sense with 'void *'.
> > Blind rewrite of ctx is a foot gun.
> >
> > I've tried the following:
> >
> > 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 9a06e5eb1fbe..0e5f5205d4a8 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
> > @@ -106,9 +106,9 @@ int perf_event_ctx(void *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)
> > +__weak int subprog_ctx_tag(long ctx __arg_ctx)
> > {
> > - return bpf_get_stack(ctx, stack, sizeof(stack), 0);
> > + return bpf_get_stack((void *)ctx, stack, sizeof(stack), 0);
> > }
> >
> > struct my_struct { int x; };
> > @@ -131,7 +131,7 @@ 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);
> > + return subprog_ctx_tag((long)ctx) +
> > subprog_multi_ctx_tags(ctx, &x, ctx);
> > }
> >
> > and it "works".
>
> Yeah, but you had to actively force casting everywhere *and* you still
> had to consciously add __arg_ctx, right? If a user wants to subvert
> the type system, they will do it. It's C, after all. But if they just
> accidentally use sk_buff ctx and call it from the XDP program with
> xdp_buff/xdp_md, the compiler will call out type mismatch.
I could have used long everywhere and avoided casts.
> >
> > Both kernel and libbpf should really limit it to 'void *'.
> >
> > In the other email I suggested to allow types that match expected
> > based on prog type, but even that is probably a danger zone as well.
> > The correct type would already be detected by the verifier,
> > so extra __arg_ctx is pointless.
> > It makes sense only for such polymorphic functions and those
> > better use 'void *' and don't dereference it.
> >
> > I think this can be a follow up.
>
> Not really just polymorphic functions. Think about subprog
> specifically for the fentry program, as one example. You *need*
> __arg_ctx just to make context passing work, but you also want
> non-`void *` type to access arguments.
>
> int subprog(u64 *args __arg_ctx) { ... }
>
> SEC("fentry")
> int BPF_PROG(main_prog, ...)
> {
> return subprog(ctx);
> }
>
> Similarly, tracepoint programs, you'd have:
>
> int subprog(struct syscall_trace_enter* ctx __arg_ctx) { ... }
>
> SEC("tracepoint/syscalls/sys_enter_kill")
> int main_prog(struct syscall_trace_enter* ctx)
> {
> return subprog(ctx);
> }
>
> So that's one group of cases.
But the above two are not supported by libbpf
since it doesn't handle "tracing" and "tracepoint" prog types
in global_ctx_map.
I suspect the kernel sort-of supports above, but in a dangerous
and broken way.
My point is that users must not use __arg_ctx in these two cases.
fentry (tracing prog type) wants 'void *' in the kernel to
match to ctx.
So the existing mechanism (prior to arg_ctx in the kernel)
should already work.
> Another special case are networking programs, where both "__sk_buff"
> and "sk_buff" are allowed, same for "xdp_buff" and "xdp_md".
what do you mean both?
networking bpf prog must only use __sk_buff and that is one and
only supported ctx.
Using 'struct sk_buff *ctx __arg_ctx' will be a bad bug.
Since offsets will be all wrong while ctx rewrite will apply garbage
and will likely fail.
> Also, kprobes are special, both "struct bpf_user_pt_regs_t" and
> *typedef* "bpf_user_pt_regs_t" are supported. But in practice users
> will often just use `struct pt_regs *ctx`, actually.
Same thing. The global bpf prog has to use bpf_user_pt_regs_t
to be properly recognized as ctx arg type.
Nothing special. Using 'struct pt_regs * ctx __arg_ctx' and blind
rewrite will cause similar hard to debug bugs when
bpf_user_pt_regs_t doesn't match pt_regs that bpf prog sees
at compile time.
