Re: [PATCH bpf-next 1/3] libbpf: rework feature-probing APIs

[Dave Marchevsky <davemarchevsky@fb.com>]

On 12/16/21 7:41 PM, Andrii Nakryiko wrote:   
> On Thu, Dec 16, 2021 at 4:10 PM Dave Marchevsky <davemarchevsky@fb.com> 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@fb.com>
>>> Cc: Julia Kartseva <hex@fb.com>
>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>> ---
>>
>> [...]
>>
>>> 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@fb.com>

>>
>>> +     return 1; /* assume supported */
>>>  }
>>>
>>
>> [...]