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

[Dave Marchevsky <davemarchevsky@fb.com>]

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?
Also, maybe the comment in this particular instance is incorrect?
(EOPNOTSUPP -> 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.

> +	return 1; /* assume supported */
>  }
>  

[...]