On 12/16/21 7:41 PM, Andrii Nakryiko wrote: > 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. Ah, I think there's a misunderstanding. I meant "In verifier.c, could you point out that this is done here". But I agree with your point that this is unnecessary given the selftests added later in the series. Acked-by: Dave Marchevsky <davemarchevsky@xxxxxx> >> >>> + return 1; /* assume supported */ >>> } >>> >> >> [...]
