On Thu, Jan 4, 2024 at 12:58 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
>
> My point was that it's hard to accidentally forget to "generalize"
> type if you were supporting sk_buff, and suddenly started calling it
> with xdp_md.
>
> From my POV, if I'm a user, and I declare an argument as long and
> annotate it as __arg_ctx, then I know what I'm doing and I'd hate for
> some smart-ass library to double-guess me dictating what exact
> incantation I should specify to make it happy.
But that's exactly what's happening!
The smart-ass libbpf ignores the type in 'struct sk_buff *skb __arg_ctx'
and replaces it with whatever is appropriate for prog type.
More below.
> static __attribute__ ((noinline))
> -int f0(int var, struct __sk_buff *skb)
> +int f0(int var, struct sk_buff *skb)
> {
> - return skb->len;
> + return 0;
> }
>
> __attribute__ ((noinline))
> @@ -20,7 +19,7 @@ int f1(struct __sk_buff *skb)
>
> __sink(buf[MAX_STACK - 1]);
>
> - return f0(0, skb) + skb->len;
> + return f0(0, (void*)skb) + skb->len;
This is static f0. Not sure what you're trying to say.
I don't think btf_get_prog_ctx_type() logic applies here.
> I'll say even more, with libbpf's PT_REGS_xxx() macros you don't even
> need to know about pt_regs vs user_pt_regs difference, as macros
> properly force-cast arguments, depending on architecture. So in your
> BPF code you can just pass `struct pt_regs *` around just fine across
> multiple architectures as long as you only use PT_REGS_xxx() macros
> and then pass that context to helpers (to get stack trace,
> bpf_perf_event_output, etc).
Pretty much. For some time the kernel recognized bpf_user_pt_regs_t
as PTR_TO_CTX for kprobe.
And the users who needed global prog verification with ctx
already used that feature.
We even have helper macros to typeof to correct btf type.
>From selftests:
_weak int kprobe_typedef_ctx_subprog(bpf_user_pt_regs_t *ctx)
{
return bpf_get_stack(ctx, &stack, sizeof(stack), 0);
}
SEC("?kprobe")
__success
int kprobe_typedef_ctx(void *ctx)
{
return kprobe_typedef_ctx_subprog(ctx);
}
#define pt_regs_struct_t typeof(*(__PT_REGS_CAST((struct pt_regs *)NULL)))
__weak int kprobe_struct_ctx_subprog(pt_regs_struct_t *ctx)
{
return bpf_get_stack((void *)ctx, &stack, sizeof(stack), 0);
}
SEC("?kprobe")
__success
int kprobe_resolved_ctx(void *ctx)
{
return kprobe_struct_ctx_subprog(ctx);
}
__PT_REGS_CAST is arch dependent and typeof makes it seen with
correct btf_id and the kernel knows it's PTR_TO_CTX.
All that works. No need for __arg_ctx.
I'm sure you know this.
I'm only explaining for everybody else to follow.
> No one even knows about bpf_user_pt_regs_t, I had to dig it up from
> kernel source code and let users know what exact type name to use for
> global subprog.
Few people know that global subprogs are verified differently than static.
That's true, but I bet people that knew also used the right type for ctx.
If you're saying that __arg_ctx is making it easier for users
to use global subprogs I certainly agree, but it's not
something that was mandatory for uniform global progs.
__arg_ctx main value is for polymorphic subprogs.
An add-on value is ease-of-use for existing non polymorphic subrpogs.
I'm saying that in the above example working code:
__weak int kprobe_typedef_ctx_subprog(bpf_user_pt_regs_t *ctx)
should _not_ be allowed to be replaced with:
__weak int kprobe_typedef_ctx_subprog(struct pt_regs *ctx __arg_ctx)
Unfortunately in the newest kernel/libbpf patches allowed it and
this way both kernel and libbpf are silently breaking C type
matching rules and general expectations of C language.
Consider these variants:
1.
__weak int kprobe_typedef_ctx_subprog(struct pt_regs *ctx __arg_ctx)
{ PT_REGS_PARM1(ctx); }
2.
__weak int kprobe_typedef_ctx_subprog(void *ctx __arg_ctx)
{ struct pt_regs *regs = ctx; PT_REGS_PARM1(regs); }
3.
__weak int kprobe_typedef_ctx_subprog(bpf_user_pt_regs_t *ctx)
{ PT_REGS_PARM1(ctx); }
In 1 and 3 the caller has to type cast to correct type.
In 2 the caller can pass anything without type cast.
In C when the user writes: void foo(int *p)
it knows that it can access it as pointer to int in the callee
and it's caller's job to pass correct pointer into it.
When caller type casts something else to 'int *' it's caller's fault
if things don't work.
Now when user writes:
void foo(void *p) { int *i = p;
the caller can pass anything into foo() and callee's fault
to assume that 'void *' is 'int *'.
These are the C rules that we're breaking with __arg_ctx.
In 2 it's clear to callee that any ctx argument could have been passed
and type cast to 'struct pt_regs *' it's callee's responsibility.
In 3 the users know that only bpf_user_pt_regs_t will be passed in.
But 1 (the current kernel and libbpf) breaks these C rules.
The C language tells prog writer to expect that only 'struct pt_regs *'
will be passed, but the kernel/libbpf allows any ctx to be passed in.
Hence 1 should be disallowed.
The 'void *' case 2 we extend in the future to truly support polymorphism:
__weak int subprog(void *ctx __arg_ctx)
{
__u32 ctx_btf_id = bpf_core_typeof(*ctx);
if (ctx_btf_id == bpf_core_type_id_kernel(struct sk_buff)) {
struct sk_buff *skb = ctx;
..
} else if (ctx_btf_id == bpf_core_type_id_kernel(struct xdp_buff)) {
struct xdp_buff *xdp = ctx;
and it will conform to C rules. It's on callee side to do the right
thing with 'void *'.
