On Thu, Dec 21, 2023 at 5:26 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Wed, Dec 20, 2023 at 03:31:26PM -0800, Andrii Nakryiko 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) { ... }
>
> Just realized that we probably need to enforce in both libbpf doing
> rewrite and in the kernel that __arg_ctx is either valid
> 'struct correct_type_for_bpf_prog *' or 'void *'.
>
> Otherwise the user will get surprising behavior when
> int foo(struct __sk_buff *ctx __arg_ctx)
> {
> ctx->len;
> }
> will get rewritten to 'struct pt_regs *ctx' based on prog type
> while all asm instructions inside prog were compiled with 'struct __sk_buff'
> and CO_RE performs relocations against that type.
Nothing really prevents users from misusing types even today, so it
doesn't seem like a common problem, luckily.
But really the problem is that some program types don't have an
associated struct name at all, but still a valid context. Like for
LSM/TRACING programs it's a u64[]/u64 *. For tracepoints context is
defined as just plain u64 (according to bpf_ctx_convert), and so on.
Oh, and there is KPROBE program type, where it's (typedef)
bpf_user_pt_regs_t, and for backwards compatibility reasons we also
allow basically non-existing `struct bpf_user_pt_regs_t`.
So it gets messy. Either way, I have a patch set coming up for
kernel-side __arg_xxx tags support, so let's discuss it there?
>
> > +static struct {
> > + enum bpf_prog_type prog_type;
> > + const char *ctx_name;
> > +} global_ctx_map[] = {
> > + { BPF_PROG_TYPE_CGROUP_DEVICE, "bpf_cgroup_dev_ctx" },
> > + { BPF_PROG_TYPE_CGROUP_SKB, "__sk_buff" },
> > + { BPF_PROG_TYPE_CGROUP_SOCK, "bpf_sock" },
> > + { BPF_PROG_TYPE_CGROUP_SOCK_ADDR, "bpf_sock_addr" },
> > + { BPF_PROG_TYPE_CGROUP_SOCKOPT, "bpf_sockopt" },
> > + { BPF_PROG_TYPE_CGROUP_SYSCTL, "bpf_sysctl" },
> > + { BPF_PROG_TYPE_FLOW_DISSECTOR, "__sk_buff" },
> > + { BPF_PROG_TYPE_KPROBE, "bpf_user_pt_regs_t" },
> > + { BPF_PROG_TYPE_LWT_IN, "__sk_buff" },
> > + { BPF_PROG_TYPE_LWT_OUT, "__sk_buff" },
> > + { BPF_PROG_TYPE_LWT_SEG6LOCAL, "__sk_buff" },
> > + { BPF_PROG_TYPE_LWT_XMIT, "__sk_buff" },
> > + { BPF_PROG_TYPE_NETFILTER, "bpf_nf_ctx" },
> > + { BPF_PROG_TYPE_PERF_EVENT, "bpf_perf_event_data" },
> > + { BPF_PROG_TYPE_RAW_TRACEPOINT, "bpf_raw_tracepoint_args" },
> > + { BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE, "bpf_raw_tracepoint_args" },
> > + { BPF_PROG_TYPE_SCHED_ACT, "__sk_buff" },
> > + { BPF_PROG_TYPE_SCHED_CLS, "__sk_buff" },
> > + { BPF_PROG_TYPE_SK_LOOKUP, "bpf_sk_lookup" },
> > + { BPF_PROG_TYPE_SK_MSG, "sk_msg_md" },
> > + { BPF_PROG_TYPE_SK_REUSEPORT, "sk_reuseport_md" },
> > + { BPF_PROG_TYPE_SK_SKB, "__sk_buff" },
> > + { BPF_PROG_TYPE_SOCK_OPS, "bpf_sock_ops" },
> > + { BPF_PROG_TYPE_SOCKET_FILTER, "__sk_buff" },
> > + { BPF_PROG_TYPE_XDP, "xdp_md" },
>
> We already share the .c files (like relo_core.c) between kernel and libbpf
> let's share here as well to avoid copy paste.
> All of the above is available in include/linux/bpf_types.h
True, but libbpf sources are built both as part of the kernel repo and
separately on Github, so we'll need to start syncing
include/linux/bpf_types.h into tools/include, so that's a bit of
inconvenience.
But most of all I don't want to do it for a few other reasons.
This table was manually constructed by inspecting struct bpf_ctx_convert with:
bpftool btf dump file /sys/kernel/btf/vmlinux format c | rg
bpf_ctx_convert -A65 | rg _prog
And it has to be manual because of other program types that don't have
associated struct for context. So even if there was bpf_types.h, we
can't use it as is.
But, if your concern is maintainability of this, I don't think that's
a problem at all. Even if we add a new program type with its own
struct name for context, we don't even have to extend this table
(though we might, if we want to), because at that point kernel is
guaranteed to have in-kernel native support for __arg_ctx, so libbpf
doesn't have to do type rewriting.
Also, this probably is the first explicit table that shows which
struct names should be used for global subprog context argument (if
not using __arg_ctx, of course). Which is not really a reason per se,
but it beats reading kernel code, and (non-trivially) figuring out
that one needs to look how struct bpf_ctx_convert is generated, etc.
Having this explicit table is much easier to link as a reference for
those special context type names.
>
> > + /* clone fn/fn_proto, unless we already did it for another arg */
> > + if (func_rec->type_id == orig_fn_id) {
>
> It feels that body of this 'if' can be factored out as a separate helper function.
>
Sure, I'll try to factor it out.
> > -static int
> > -bpf_object__load_progs(struct bpf_object *obj, int log_level)
> > +static int bpf_object_load_progs(struct bpf_object *obj, int log_level)
>
> pls keep __ convention.
replied on another patch, I'll do a conversion to consistent naming in
the next revision
