On Sat, Mar 25, 2023 at 9:59 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Fri, Mar 24, 2023 at 04:27:45PM -0700, Andrii Nakryiko wrote:
> > +static int guess_prog_type_by_ctx_name(const char *ctx_name,
> > + enum bpf_prog_type *prog_type,
> > + enum bpf_attach_type *attach_type)
> > +{
> > + /* We need to guess program type based on its declared context type.
> > + * This guess can't be perfect as many different program types might
> > + * share the same context type. So we can only hope to reasonably
> > + * well guess this and get lucky.
> > + *
> > + * Just in case, we support both UAPI-side type names and
> > + * kernel-internal names.
> > + */
> > + static struct {
> > + const char *uapi_name;
> > + const char *kern_name;
> > + enum bpf_prog_type prog_type;
> > + enum bpf_attach_type attach_type;
> > + } ctx_map[] = {
> > + /* __sk_buff is most ambiguous, for now we assume cgroup_skb */
> > + { "__sk_buff", "sk_buff", BPF_PROG_TYPE_CGROUP_SKB, BPF_CGROUP_INET_INGRESS },
> > + { "bpf_sock", "sock", BPF_PROG_TYPE_CGROUP_SOCK, BPF_CGROUP_INET4_POST_BIND },
> > + { "bpf_sock_addr", "bpf_sock_addr_kern", BPF_PROG_TYPE_CGROUP_SOCK_ADDR, BPF_CGROUP_INET4_BIND },
> > + { "bpf_sock_ops", "bpf_sock_ops_kern", BPF_PROG_TYPE_SOCK_OPS, BPF_CGROUP_SOCK_OPS },
> > + { "sk_msg_md", "sk_msg", BPF_PROG_TYPE_SK_MSG, BPF_SK_MSG_VERDICT },
> > + { "bpf_cgroup_dev_ctx", "bpf_cgroup_dev_ctx", BPF_PROG_TYPE_CGROUP_DEVICE, BPF_CGROUP_DEVICE },
> > + { "bpf_sysctl", "bpf_sysctl_kern", BPF_PROG_TYPE_CGROUP_SYSCTL, BPF_CGROUP_SYSCTL },
> > + { "bpf_sockopt", "bpf_sockopt_kern", BPF_PROG_TYPE_CGROUP_SOCKOPT, BPF_CGROUP_SETSOCKOPT },
> > + { "sk_reuseport_md", "sk_reuseport_kern", BPF_PROG_TYPE_SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT_OR_MIGRATE },
> > + { "bpf_sk_lookup", "bpf_sk_lookup_kern", BPF_PROG_TYPE_SK_LOOKUP, BPF_SK_LOOKUP },
> > + { "xdp_md", "xdp_buff", BPF_PROG_TYPE_XDP, BPF_XDP },
> > + /* tracing types with no expected attach type */
> > + { "bpf_user_pt_regs_t", "pt_regs", BPF_PROG_TYPE_KPROBE },
> > + { "bpf_perf_event_data", "bpf_perf_event_data_kern", BPF_PROG_TYPE_PERF_EVENT },
> > + { "bpf_raw_tracepoint_args", NULL, BPF_PROG_TYPE_RAW_TRACEPOINT },
> > + };
> > + int i;
> > +
> > + if (!ctx_name)
> > + return -EINVAL;
> > +
> > + for (i = 0; i < ARRAY_SIZE(ctx_map); i++) {
> > + if (strcmp(ctx_map[i].uapi_name, ctx_name) == 0 ||
> > + (!ctx_map[i].kern_name || strcmp(ctx_map[i].kern_name, ctx_name) == 0)) {
>
> I'm struggling to understand above A || (B || C) logic.
> () are redundant?
oh, shoot, should have been A || (B && C). Last minute refactoring
gone wrong. kern_name is optional (for bpf_raw_tracepoint_args), so
this is just to avoid strcmp() on NULL.
>
> > + *prog_type = ctx_map[i].prog_type;
> > + *attach_type = ctx_map[i].attach_type;
> > + return 0;
> > + }
> > + }
> > +
> > + return -ESRCH;
>
> This will never be hit, since "bpf_raw_tracepoint_args", NULL is last and
> it will always succeed.
>
> The idea is to always succeed in guessing and default to raw_tp ?
> ... and there should be only one entry with kern_name == NULL...
No-no, it's much simpler than that. It should have been A || (B && C)
above. If we can't guess, we shouldn't just assume raw_tp, that wasn't
the intent.
Unfortunately I didn't see your comments before sending v3, I'll send
v4 with a fix for NULL check.
> But it's not mentioned anywhere in the comments.
> A weird way to implement a default.
> I'm definitely missing something.
