From: Dave Marchevsky <davemarchevsky@fb.com>
To: Andrii Nakryiko <andrii@kernel.org>, <bpf@vger.kernel.org>,
<ast@kernel.org>, <daniel@iogearbox.net>
Cc: <kernel-team@fb.com>, Julia Kartseva <hex@fb.com>
Subject: Re: [PATCH bpf-next 1/3] libbpf: rework feature-probing APIs
Date: Thu, 16 Dec 2021 19:10:03 -0500 [thread overview]
Message-ID: <587861ab-e072-c448-c649-ddc6e51353d6@fb.com> (raw)
In-Reply-To: <20211216070442.1492204-2-andrii@kernel.org>
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 */
> }
>
[...]
next prev parent reply other threads:[~2021-12-17 0:10 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-16 7:04 [PATCH bpf-next 0/3] Revamp and fix libbpf's feature-probing APIs Andrii Nakryiko
2021-12-16 7:04 ` [PATCH bpf-next 1/3] libbpf: rework " Andrii Nakryiko
2021-12-17 0:10 ` Dave Marchevsky [this message]
2021-12-17 0:41 ` Andrii Nakryiko
2021-12-17 7:07 ` Dave Marchevsky
2021-12-16 7:04 ` [PATCH bpf-next 2/3] selftests/bpf: add libbpf feature-probing API selftests Andrii Nakryiko
2021-12-17 0:21 ` Dave Marchevsky
2021-12-17 0:42 ` Andrii Nakryiko
2021-12-17 17:09 ` Andrii Nakryiko
2021-12-16 7:04 ` [PATCH bpf-next 3/3] bpftool: reimplement large insn size limit feature probing Andrii Nakryiko
2021-12-17 0:36 ` Dave Marchevsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=587861ab-e072-c448-c649-ddc6e51353d6@fb.com \
--to=davemarchevsky@fb.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=hex@fb.com \
--cc=kernel-team@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).