On Tue, Jan 2, 2024 at 9:06 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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.
Another headache I realized as I was reading someone else's patch is
all the #ifdef CONFIG_xxx checks, which we'd need to fake to even get
a full list of program types. In short, it's more trouble than it's
worth.
>
> 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
