From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
Networking <netdev@vger.kernel.org>,
Alexei Starovoitov <ast@fb.com>, Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v4 bpf-next 2/4] libbpf: support libbpf-provided extern variables
Date: Mon, 16 Dec 2019 11:29:29 -0800 [thread overview]
Message-ID: <CAEf4Bzbx+2Fot9NYzGJS-pUF5x5zvcfBnb7fcO_s9_gCQQVuLg@mail.gmail.com> (raw)
In-Reply-To: <20191216111736.GA14887@linux.fritz.box>
On Mon, Dec 16, 2019 at 3:17 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On Fri, Dec 13, 2019 at 05:47:08PM -0800, Andrii Nakryiko wrote:
> > Add support for extern variables, provided to BPF program by libbpf. Currently
> > the following extern variables are supported:
> > - LINUX_KERNEL_VERSION; version of a kernel in which BPF program is
> > executing, follows KERNEL_VERSION() macro convention, can be 4- and 8-byte
> > long;
> > - CONFIG_xxx values; a set of values of actual kernel config. Tristate,
> > boolean, strings, and integer values are supported.
> >
> [...]
> >
> > All detected extern variables, are put into a separate .extern internal map.
> > It, similarly to .rodata map, is marked as read-only from BPF program side, as
> > well as is frozen on load. This allows BPF verifier to track extern values as
> > constants and perform enhanced branch prediction and dead code elimination.
> > This can be relied upon for doing kernel version/feature detection and using
> > potentially unsupported field relocations or BPF helpers in a CO-RE-based BPF
> > program, while still having a single version of BPF program running on old and
> > new kernels. Selftests are validating this explicitly for unexisting BPF
> > helper.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> [...]
> > +static int bpf_object__resolve_externs(struct bpf_object *obj,
> > + const char *config_path)
> > +{
> > + bool need_config = false;
> > + struct extern_desc *ext;
> > + int err, i;
> > + void *data;
> > +
> > + if (obj->nr_extern == 0)
> > + return 0;
> > +
> > + data = obj->maps[obj->extern_map_idx].mmaped;
> > +
> > + for (i = 0; i < obj->nr_extern; i++) {
> > + ext = &obj->externs[i];
> > +
> > + if (strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> > + void *ext_val = data + ext->data_off;
> > + __u32 kver = get_kernel_version();
> > +
> > + if (!kver) {
> > + pr_warn("failed to get kernel version\n");
> > + return -EINVAL;
> > + }
> > + err = set_ext_value_num(ext, ext_val, kver);
> > + if (err)
> > + return err;
> > + pr_debug("extern %s=0x%x\n", ext->name, kver);
> > + } else if (strncmp(ext->name, "CONFIG_", 7) == 0) {
> > + need_config = true;
> > + } else {
> > + pr_warn("unrecognized extern '%s'\n", ext->name);
> > + return -EINVAL;
> > + }
>
> I don't quite like that this is (mainly) tracing-only specific, and that
> for everything else we just bail out - there is much more potential than
> just completing above vars. But also, there is also no way to opt-out
> for application developers of /this specific/ semi-magic auto-completion
> of externs.
What makes you think it's tracing only? While non-tracing apps
probably don't need to care about LINUX_KERNEL_VERSION, all of the
CONFIG_ stuff is useful and usable for any type of application.
As for opt-out, you can easily opt out by not using extern variables.
>
> bpf_object__resolve_externs() should be changed instead to invoke a
> callback obj->resolve_externs(). Former can be passed by the application
> developer to allow them to take care of extern resolution all by themself,
> and if no callback has been passed, then we default to the one above
> being set as obj->resolve_externs.
Can you elaborate on the use case you have in mind? The way I always
imagined BPF applications provide custom read-only parameters to BPF
side is through using .rodata variables. With skeleton it's super easy
to initialize them before BPF program is loaded, and their values will
be well-known by verifier and potentially optimized.
E.g., with skeleton, it becomes trivial. E.g., on BPF side:
const volatile int custom_ipv4;
const volatile bool feature_X_enabled;
...
if (custom_ipv4 && in_ipv4 != custom_ipv4)
return 0;
if (feature_X_enabled) {
/* do something fancy */
}
Then on userspace side:
/* instantiate skeleton */
skel = my_prog__open();
skel->rodata->custom_ipv4 = IP_AS_INT(1, 2, 3, 4);
if (/* should enable feature X*/)
skel->rodata->feature_X_enabled = true;
my_prog__load(); /* load, verify, eliminate dead code and optimize */
So for application-specific stuff, there isn't really a need to use
externs to do that. Furthermore, I think allowing using externs as
just another way to specify application-specific configuration is
going to create a problem, potentially, as we'll have higher
probability of collisions with kernel-provided extersn (variables
and/or functions), or even externs provided by other
dynamically/statically linked BPF programs (once we have dynamic and
static linking, of course).
So if you still insist we need user to provide custom extern-parsing
logic, can you please elaborate on the use case details?
BTW, from discussion w/ Alexei on another thread, I think I'm going to
change kconfig_path option to just `kconfig`, which will specify
additional config in Kconfig format. This could be used by
applications to provide their own config, augmenting Kconfig with
custom overrides.
>
> > + }
> > + if (need_config) {
> > + err = bpf_object__read_kernel_config(obj, config_path, data);
> > + if (err)
> > + return -EINVAL;
> > + }
> > + for (i = 0; i < obj->nr_extern; i++) {
> > + ext = &obj->externs[i];
> > +
> > + if (!ext->is_set && !ext->is_weak) {
> > + pr_warn("extern %s (strong) not resolved\n", ext->name);
> > + return -ESRCH;
> > + } else if (!ext->is_set) {
> > + pr_debug("extern %s (weak) not resolved, defaulting to zero\n",
> > + ext->name);
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
> > {
> > struct bpf_object *obj;
> > @@ -4126,6 +4753,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
> > obj->loaded = true;
> >
> > err = bpf_object__probe_caps(obj);
> > + err = err ? : bpf_object__resolve_externs(obj, obj->kconfig_path);
> > err = err ? : bpf_object__sanitize_and_load_btf(obj);
> > err = err ? : bpf_object__sanitize_maps(obj);
> > err = err ? : bpf_object__create_maps(obj);
> [...]
next prev parent reply other threads:[~2019-12-16 19:29 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-14 1:47 [PATCH v4 bpf-next 0/4] Add libbpf-provided extern variables support Andrii Nakryiko
2019-12-14 1:47 ` [PATCH v4 bpf-next 1/4] libbpf: extract internal map names into constants Andrii Nakryiko
2019-12-14 1:47 ` [PATCH v4 bpf-next 2/4] libbpf: support libbpf-provided extern variables Andrii Nakryiko
2019-12-14 12:50 ` Toke Høiland-Jørgensen
2019-12-14 20:27 ` Yonghong Song
2019-12-16 11:17 ` Daniel Borkmann
2019-12-16 19:29 ` Andrii Nakryiko [this message]
2019-12-17 14:42 ` Daniel Borkmann
2019-12-17 19:03 ` Andrii Nakryiko
2019-12-17 19:50 ` Daniel Borkmann
2019-12-17 20:16 ` Alexei Starovoitov
2019-12-17 23:37 ` Daniel Borkmann
2019-12-18 0:08 ` Andrii Nakryiko
2019-12-16 12:43 ` Daniel Borkmann
2019-12-16 18:19 ` Andrii Nakryiko
2019-12-14 1:47 ` [PATCH v4 bpf-next 3/4] bpftool: generate externs datasec in BPF skeleton Andrii Nakryiko
2019-12-14 1:47 ` [PATCH v4 bpf-next 4/4] selftests/bpf: add tests for libbpf-provided externs Andrii Nakryiko
2019-12-16 0:52 ` [PATCH v4 bpf-next 0/4] Add libbpf-provided extern variables support Alexei Starovoitov
2019-12-16 1:47 ` Andrii Nakryiko
2019-12-16 4:42 ` Alexei Starovoitov
2019-12-16 19:34 ` Andrii Nakryiko
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=CAEf4Bzbx+2Fot9NYzGJS-pUF5x5zvcfBnb7fcO_s9_gCQQVuLg@mail.gmail.com \
--to=andrii.nakryiko@gmail.com \
--cc=andriin@fb.com \
--cc=ast@fb.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=netdev@vger.kernel.org \
/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).