On Mon, Dec 16, 2019 at 3:17 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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@xxxxxx>
> [...]
> > +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);
> [...]
