On Thu, Dec 16, 2021 at 4:10 PM Dave Marchevsky <davemarchevsky@xxxxxx> wrote: > > On 12/16/21 2:04 AM, Andrii Nakryiko wrote: > > Create three extensible alternatives to inconsistently named > > feature-probing APIs: > > - libbpf_probe_bpf_prog_type() instead of bpf_probe_prog_type(); > > - libbpf_probe_bpf_map_type() instead of bpf_probe_map_type(); > > - libbpf_probe_bpf_helper() instead of bpf_probe_helper(). > > > > Set up return values such that libbpf can report errors (e.g., if some > > combination of input arguments isn't possible to validate, etc), in > > addition to whether the feature is supported (return value 1) or not > > supported (return value 0). > > > > Also schedule deprecation of those three APIs. Also schedule deprecation > > of bpf_probe_large_insn_limit(). > > > > Also fix all the existing detection logic for various program and map > > types that never worked: > > - BPF_PROG_TYPE_LIRC_MODE2; > > - BPF_PROG_TYPE_TRACING; > > - BPF_PROG_TYPE_LSM; > > - BPF_PROG_TYPE_EXT; > > - BPF_PROG_TYPE_SYSCALL; > > - BPF_PROG_TYPE_STRUCT_OPS; > > - BPF_MAP_TYPE_STRUCT_OPS; > > - BPF_MAP_TYPE_BLOOM_FILTER. > > > > Above prog/map types needed special setups and detection logic to work. > > Subsequent patch adds selftests that will make sure that all the > > detection logic keeps working for all current and future program and map > > types, avoiding otherwise inevitable bit rot. > > > > [0] Closes: https://github.com/libbpf/libbpf/issues/312 > > > > Cc: Dave Marchevsky <davemarchevsky@xxxxxx> > > Cc: Julia Kartseva <hex@xxxxxx> > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > > --- > > [...] > > > diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c > > index 4bdec69523a7..65232bcaa84c 100644 > > --- a/tools/lib/bpf/libbpf_probes.c > > +++ b/tools/lib/bpf/libbpf_probes.c > > [...] > > > @@ -84,6 +92,38 @@ probe_load(enum bpf_prog_type prog_type, const struct bpf_insn *insns, > > case BPF_PROG_TYPE_KPROBE: > > opts.kern_version = get_kernel_version(); > > break; > > + case BPF_PROG_TYPE_LIRC_MODE2: > > + opts.expected_attach_type = BPF_LIRC_MODE2; > > + break; > > + case BPF_PROG_TYPE_TRACING: > > + case BPF_PROG_TYPE_LSM: > > + opts.log_buf = buf; > > + opts.log_size = sizeof(buf); > > + opts.log_level = 1; > > + if (prog_type == BPF_PROG_TYPE_TRACING) > > + opts.expected_attach_type = BPF_TRACE_FENTRY; > > + else > > + opts.expected_attach_type = BPF_MODIFY_RETURN; > > + opts.attach_btf_id = 1; > > + > > + exp_err = -EINVAL; > > + exp_msg = "attach_btf_id 1 is not a function"; > > + break; > > + case BPF_PROG_TYPE_EXT: > > + opts.log_buf = buf; > > + opts.log_size = sizeof(buf); > > + opts.log_level = 1; > > + opts.attach_btf_id = 1; > > + > > + exp_err = -EINVAL; > > + exp_msg = "Cannot replace kernel functions"; > > + break; > > + case BPF_PROG_TYPE_SYSCALL: > > + opts.prog_flags = BPF_F_SLEEPABLE; > > + break; > > + case BPF_PROG_TYPE_STRUCT_OPS: > > + exp_err = -524; /* -EOPNOTSUPP */ > > Why not use the ENOTSUPP macro here, and elsewhere in this patch? ENOTSUPP is kernel-internal code, so there is no #define ENOTSUPP available to user-space applications. > Also, maybe the comment in this particular instance is incorrect? > (EOPNOTSUPP -> ENOTSUPP) Yeah, I seem to constantly mess up comments. It should be -ENOTSUPP. > > > + break; > > case BPF_PROG_TYPE_UNSPEC: > > case BPF_PROG_TYPE_SOCKET_FILTER: > > case BPF_PROG_TYPE_SCHED_CLS: > > [...] > > > +int libbpf_probe_bpf_helper(enum bpf_prog_type prog_type, enum bpf_func_id helper_id, > > + const void *opts) > > +{ > > + struct bpf_insn insns[] = { > > + BPF_EMIT_CALL((__u32)helper_id), > > + BPF_EXIT_INSN(), > > + }; > > + const size_t insn_cnt = ARRAY_SIZE(insns); > > + char buf[4096]; > > + int ret; > > + > > + if (opts) > > + return libbpf_err(-EINVAL); > > + > > + /* we can't successfully load all prog types to check for BPF helper > > + * support, so bail out with -EOPNOTSUPP error > > + */ > > + switch (prog_type) { > > + case BPF_PROG_TYPE_TRACING: > > + case BPF_PROG_TYPE_EXT: > > + case BPF_PROG_TYPE_LSM: > > + case BPF_PROG_TYPE_STRUCT_OPS: > > + return -EOPNOTSUPP; > > + default: > > + break; > > + } > > + > > + buf[0] = '\0'; > > + ret = probe_prog_load(prog_type, insns, insn_cnt, buf, sizeof(buf), 0); > > + if (ret < 0) > > + return libbpf_err(ret); > > + > > + /* If BPF verifier doesn't recognize BPF helper ID (enum bpf_func_id) > > + * at all, it will emit something like "invalid func unknown#181". > > + * If BPF verifier recognizes BPF helper but it's not supported for > > + * given BPF program type, it will emit "unknown func bpf_sys_bpf#166". > > + * In both cases, provided combination of BPF program type and BPF > > + * helper is not supported by the kernel. > > + * In all other cases, probe_prog_load() above will either succeed (e.g., > > + * because BPF helper happens to accept no input arguments or it > > + * accepts one input argument and initial PTR_TO_CTX is fine for > > + * that), or we'll get some more specific BPF verifier error about > > + * some unsatisfied conditions. > > + */ > > + if (ret == 0 && (strstr(buf, "invalid func ") || strstr(buf, "unknown func "))) > > + return 0; > > Maybe worth adding a comment where these are logged in verifier.c, so that if > format is changed or a less brittle way of conveying this info is added, this > fn can be updated. Not sure I want to point out that this is done in verifier.c specifically. What if that code is moved somewhere else? Seems like a bit too detailed and specific comment to add. I was hoping the above big comment explains what we do here, thought? As for the need to change this, with selftests I added we'll get immediate selftests failure, so this won't bit rot and we'll be always aware when the detection breaks. > > > + return 1; /* assume supported */ > > } > > > > [...]
