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.
>
> 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.
Another special case are networking programs, where both "__sk_buff"
and "sk_buff" are allowed, same for "xdp_buff" and "xdp_md".
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.
There might be some other edges I don't yet realize.
In short, I think any sort of enforcement will just cause unnecessary
pain, while seemingly fixing some problem that doesn't seem to be a
problem in practice.
