[v4,bpf-next,2/4] libbpf: support libbpf-provided extern variables

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.

Set of possible values is determined by declared type of extern variable.
Supported types of variables are:
- Tristate values. Are represented as `enum libbpf_tristate`. Accepted values
  are **strictly** 'y', 'n', or 'm', which are represented as TRI_YES, TRI_NO,
  or TRI_MODULE, respectively.
- Boolean values. Are represented as bool (_Bool) types. Accepted values are
  'y' and 'n' only, turning into true/false values, respectively.
- Single-character values. Can be used both as a substritute for
  bool/tristate, or as a small-range integer:
  - 'y'/'n'/'m' are represented as is, as characters 'y', 'n', or 'm';
  - integers in a range [-128, 127] or [0, 255] (depending on signedness of
    char in target architecture) are recognized and represented with
    respective values of char type.
- Strings. String values are declared as fixed-length char arrays. String of
  up to that length will be accepted and put in first N bytes of char array,
  with the rest of bytes zeroed out. If config string value is longer than
  space alloted, it will be truncated and warning message emitted. Char array
  is always zero terminated. String literals in config have to be enclosed in
  double quotes, just like C-style string literals.
- Integers. 8-, 16-, 32-, and 64-bit integers are supported, both signed and
  unsigned variants. Libbpf enforces parsed config value to be in the
  supported range of corresponding integer type. Integers values in config can
  be:
  - decimal integers, with optional + and - signs;
  - hexadecimal integers, prefixed with 0x or 0X;
  - octal integers, starting with 0.

Config file itself is searched in /boot/config-$(uname -r) location with
fallback to /proc/config.gz, unless config path is specified explicitly
through bpf_object_open_opts' kernel_config_path option. Both gzipped and
plain text formats are supported. Libbpf adds explicit dependency on zlib
because of this, but this shouldn't be a problem, given libelf already depends
on zlib.

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>
---
 include/uapi/linux/btf.h             |   3 +-
 tools/include/uapi/linux/btf.h       |   7 +-
 tools/lib/bpf/Makefile               |  15 +-
 tools/lib/bpf/bpf_helpers.h          |   9 +
 tools/lib/bpf/btf.c                  |   9 +-
 tools/lib/bpf/libbpf.c               | 738 +++++++++++++++++++++++++--
 tools/lib/bpf/libbpf.h               |  12 +-
 tools/testing/selftests/bpf/Makefile |   2 +-
 8 files changed, 729 insertions(+), 66 deletions(-)

[...]

> +static bool sym_is_extern(const GElf_Sym *sym)
> +{
> +	int bind = GELF_ST_BIND(sym->st_info);
> +	/* externs are symbols w/ type=NOTYPE, bind=GLOBAL|WEAK, section=UND */
> +	return sym->st_shndx == SHN_UNDEF &&
> +	       (bind == STB_GLOBAL || bind == STB_WEAK) &&
> +	       GELF_ST_TYPE(sym->st_info) == STT_NOTYPE;
> +}

Will this also match function declarations marked as extern? I've
started looking into how to handle this for the static/dynamic linking
use cases and am wondering whether it makes sense to pull in this series
and build on that?

-Toke

On 12/14/19 4:50 AM, Toke Høiland-Jørgensen wrote:
> [...]
> 
>> +static bool sym_is_extern(const GElf_Sym *sym)
>> +{
>> +	int bind = GELF_ST_BIND(sym->st_info);
>> +	/* externs are symbols w/ type=NOTYPE, bind=GLOBAL|WEAK, section=UND */
>> +	return sym->st_shndx == SHN_UNDEF &&
>> +	       (bind == STB_GLOBAL || bind == STB_WEAK) &&
>> +	       GELF_ST_TYPE(sym->st_info) == STT_NOTYPE;
>> +}
> 
> Will this also match function declarations marked as extern? I've

Yes. They are treated the same as other extern variables in symbol table.

> started looking into how to handle this for the static/dynamic linking
> use cases and am wondering whether it makes sense to pull in this series
> and build on that?
> 
> -Toke
>

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.

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.

> +	}
> +	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);
[...]

On Fri, Dec 13, 2019 at 05:47:08PM -0800, Andrii Nakryiko wrote:
[...]
> Config file itself is searched in /boot/config-$(uname -r) location with
> fallback to /proc/config.gz, unless config path is specified explicitly
> through bpf_object_open_opts' kernel_config_path option. Both gzipped and
> plain text formats are supported. Libbpf adds explicit dependency on zlib
> because of this, but this shouldn't be a problem, given libelf already depends
> on zlib.

Hm, given this seems to break the build and is not an essential feature,
can't we use the feature detection from tooling infra which you invoke
anyway to compile out bpf_object__read_kernel_config() internals and return
an error there? Build could warn perf-style what won't be available for
the user in that case.

https://patchwork.ozlabs.org/patch/1210213/

Also, does libbpf.pc.template need updating wrt zlib?

On Mon, Dec 16, 2019 at 4:43 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On Fri, Dec 13, 2019 at 05:47:08PM -0800, Andrii Nakryiko wrote:
> [...]
> > Config file itself is searched in /boot/config-$(uname -r) location with
> > fallback to /proc/config.gz, unless config path is specified explicitly
> > through bpf_object_open_opts' kernel_config_path option. Both gzipped and
> > plain text formats are supported. Libbpf adds explicit dependency on zlib
> > because of this, but this shouldn't be a problem, given libelf already depends
> > on zlib.
>
> Hm, given this seems to break the build and is not an essential feature,
> can't we use the feature detection from tooling infra which you invoke
> anyway to compile out bpf_object__read_kernel_config() internals and return
> an error there? Build could warn perf-style what won't be available for
> the user in that case.
>
> https://patchwork.ozlabs.org/patch/1210213/

libz is a dependency of libelf, so this doesn't really add any new
dependencies. Everywhere where libbpf could be built, both libelf and
libz should be present already. Unfortunately now that libz is
directly used by libbpf, though, it needs to be specified explicitly
in compiler invocation, which I missed for samples/bpf, sorry about
that.

>
> Also, does libbpf.pc.template need updating wrt zlib?

Yeah, wasn't aware of it, will post a follow-up patch adding -lz there. Thanks!

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);
> [...]

On 12/16/19 8:29 PM, Andrii Nakryiko wrote:
> 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.

Ok, just curious, do you have a concrete example e.g. for tc/xdp networking
programs where you have a specific CONFIG_* extern that you're using in your
programs currently?

> 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.

We do set the map with .extern contents as read-only memory as well in
libbpf as I understand it. So same dead code optimization from the
verifier as well in this case. It feels the line with passing external
config via .rodata variables vs .extern variables gets therefore a bit
blurry.

Are we saying that in libbpf convention is that .extern contents must
*only* ever be kernel-provided but cannot come from some other configuration
data such as env/daemon?

[...]
> 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).

Yes, that makes more sense, but then we are already potentially colliding
with current CONFIG_* variables once we handle dynamically / statically
linked BPF programs. Perhaps that's my confusion in the first place. Would
have been good if 166750bc1dd2 had a discussion on that as part of the
commit message.

So, naive question, what's the rationale of not using .rodata variables
for CONFIG_* case and how do we handle these .extern collisions in future?
Should these vars rather have had some sort of annotation or be moved into
special ".extern.config" section or the like where we explicitly know that
these are handled differently so they don't collide with future ".extern"
content once we have linked BPF programs?

> 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.

Ok.

Thanks,
Daniel

On Tue, Dec 17, 2019 at 6:42 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 12/16/19 8:29 PM, Andrii Nakryiko wrote:
> > 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.
>
> Ok, just curious, do you have a concrete example e.g. for tc/xdp networking
> programs where you have a specific CONFIG_* extern that you're using in your
> programs currently?

So one good example will be CONFIG_HZ, used by latest Martin's work on
tcp_congestion_ops, as well as used by other networking BPF programs
for network stats.

>
> > 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.
>
> We do set the map with .extern contents as read-only memory as well in
> libbpf as I understand it. So same dead code optimization from the
> verifier as well in this case. It feels the line with passing external
> config via .rodata variables vs .extern variables gets therefore a bit
> blurry.
>
> Are we saying that in libbpf convention is that .extern contents must
> *only* ever be kernel-provided but cannot come from some other configuration
> data such as env/daemon?

You are right that .rodata and .extern are both read-only and allow
dead code optimization, which was a requirement for both. The line is
between who "owns" the data. .rodata should be parameters owned and
set up by BPF program and its userspace counterpart. While .extern is
something that's owned and provided by some external party. It is
libbpf for Kconfig, in the future it might be also kernel variables
(e.g., jiffies). With static and dynamic linking, extern variables
will be variables defined in some other BPF object, that we are
linking with.

>
> [...]
> > 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).
>
> Yes, that makes more sense, but then we are already potentially colliding
> with current CONFIG_* variables once we handle dynamically / statically
> linked BPF programs. Perhaps that's my confusion in the first place. Would
> have been good if 166750bc1dd2 had a discussion on that as part of the
> commit message.
>
> So, naive question, what's the rationale of not using .rodata variables
> for CONFIG_* case and how do we handle these .extern collisions in future?
> Should these vars rather have had some sort of annotation or be moved into
> special ".extern.config" section or the like where we explicitly know that
> these are handled differently so they don't collide with future ".extern"
> content once we have linked BPF programs?

Yes, name collision is a possibility, which means users should
restrain from using LINUX_KERNEL_VERSION and CONFIG_XXX names for
their variables. But if that is ever actually the problem, the way to
resolve this collision/ambiguity would be to put externs in a separate
sections. It's possible to annotate extern variable with custom
section.

But I guess putting Kconfig-provided externs into ".extern.kconfig"
might be a good idea, actually. That will make it possible to have
writable externs in the future.

>
> > 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.
>
> Ok.
>
> Thanks,
> Daniel

On 12/17/19 8:03 PM, Andrii Nakryiko wrote:
> On Tue, Dec 17, 2019 at 6:42 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 12/16/19 8:29 PM, Andrii Nakryiko wrote:
>>> 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:
[...]
>>> 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).
>>
>> Yes, that makes more sense, but then we are already potentially colliding
>> with current CONFIG_* variables once we handle dynamically / statically
>> linked BPF programs. Perhaps that's my confusion in the first place. Would
>> have been good if 166750bc1dd2 had a discussion on that as part of the
>> commit message.
>>
>> So, naive question, what's the rationale of not using .rodata variables
>> for CONFIG_* case and how do we handle these .extern collisions in future?
>> Should these vars rather have had some sort of annotation or be moved into
>> special ".extern.config" section or the like where we explicitly know that
>> these are handled differently so they don't collide with future ".extern"
>> content once we have linked BPF programs?
> 
> Yes, name collision is a possibility, which means users should
> restrain from using LINUX_KERNEL_VERSION and CONFIG_XXX names for
> their variables. But if that is ever actually the problem, the way to
> resolve this collision/ambiguity would be to put externs in a separate
> sections. It's possible to annotate extern variable with custom
> section.
> 
> But I guess putting Kconfig-provided externs into ".extern.kconfig"
> might be a good idea, actually. That will make it possible to have
> writable externs in the future.

Yep, and as mentioned it will make it more clear that these get special
loader treatment as opposed to regular externs we need to deal with in
future. A '.extern.kconfig' section sounds good to me and the BPF helper
header could provide a __kconfig annotation for that as well.

On Tue, Dec 17, 2019 at 08:50:31PM +0100, Daniel Borkmann wrote:
> > 
> > Yes, name collision is a possibility, which means users should
> > restrain from using LINUX_KERNEL_VERSION and CONFIG_XXX names for
> > their variables. But if that is ever actually the problem, the way to
> > resolve this collision/ambiguity would be to put externs in a separate
> > sections. It's possible to annotate extern variable with custom
> > section.
> > 
> > But I guess putting Kconfig-provided externs into ".extern.kconfig"
> > might be a good idea, actually. That will make it possible to have
> > writable externs in the future.
> 
> Yep, and as mentioned it will make it more clear that these get special
> loader treatment as opposed to regular externs we need to deal with in
> future. A '.extern.kconfig' section sounds good to me and the BPF helper
> header could provide a __kconfig annotation for that as well.

I think annotating all extern vars into special section name will be quite
cumbersome from bpf program writer pov.
imo capital case extern variables LINUX_KERNEL_VERSION and CONFIG_XXX are
distinct enough and make it clear they should come from something other than
normal C. Traditional C coding style uses all capital letters for macroses. So
all capital extern variables are unlikely to conflict with any normal extern
vars. Like vars in vmlinux and vars in other bpf elf files.

On 12/17/19 9:16 PM, Alexei Starovoitov wrote:
> On Tue, Dec 17, 2019 at 08:50:31PM +0100, Daniel Borkmann wrote:
>>>
>>> Yes, name collision is a possibility, which means users should
>>> restrain from using LINUX_KERNEL_VERSION and CONFIG_XXX names for
>>> their variables. But if that is ever actually the problem, the way to
>>> resolve this collision/ambiguity would be to put externs in a separate
>>> sections. It's possible to annotate extern variable with custom
>>> section.
>>>
>>> But I guess putting Kconfig-provided externs into ".extern.kconfig"
>>> might be a good idea, actually. That will make it possible to have
>>> writable externs in the future.
>>
>> Yep, and as mentioned it will make it more clear that these get special
>> loader treatment as opposed to regular externs we need to deal with in
>> future. A '.extern.kconfig' section sounds good to me and the BPF helper
>> header could provide a __kconfig annotation for that as well.
> 
> I think annotating all extern vars into special section name will be quite
> cumbersome from bpf program writer pov.
> imo capital case extern variables LINUX_KERNEL_VERSION and CONFIG_XXX are
> distinct enough and make it clear they should come from something other than
> normal C. Traditional C coding style uses all capital letters for macroses. So
> all capital extern variables are unlikely to conflict with any normal extern
> vars. Like vars in vmlinux and vars in other bpf elf files.

But still, how many of the LINUX_KERNEL_VERSION or CONFIG_XXX vars are actually
used per program. I bet just a handful. And I don't think adding a __kconfig is
cumbersome, it would make it more self-documenting in fact, denoting that this
var is not treated the usual way once prog linking is in place. Even if all
capital letters. Tomorrow, we'd be adding 'extern unsigned long jiffies' as
another potential example, and then it gets even more confusing on the 'collision'
side with regular BPF ELF. Same here, instead of __kconfig, this could have a
__vmlinux or __kernel annotation in order to document its source for the loader
(and developer) more clearly and also gives flexibility wrt ".extern.xyz"
subsections on how we want to map them.

On Tue, Dec 17, 2019 at 3:37 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 12/17/19 9:16 PM, Alexei Starovoitov wrote:
> > On Tue, Dec 17, 2019 at 08:50:31PM +0100, Daniel Borkmann wrote:
> >>>
> >>> Yes, name collision is a possibility, which means users should
> >>> restrain from using LINUX_KERNEL_VERSION and CONFIG_XXX names for
> >>> their variables. But if that is ever actually the problem, the way to
> >>> resolve this collision/ambiguity would be to put externs in a separate
> >>> sections. It's possible to annotate extern variable with custom
> >>> section.
> >>>
> >>> But I guess putting Kconfig-provided externs into ".extern.kconfig"
> >>> might be a good idea, actually. That will make it possible to have
> >>> writable externs in the future.
> >>
> >> Yep, and as mentioned it will make it more clear that these get special
> >> loader treatment as opposed to regular externs we need to deal with in
> >> future. A '.extern.kconfig' section sounds good to me and the BPF helper
> >> header could provide a __kconfig annotation for that as well.
> >
> > I think annotating all extern vars into special section name will be quite
> > cumbersome from bpf program writer pov.
> > imo capital case extern variables LINUX_KERNEL_VERSION and CONFIG_XXX are
> > distinct enough and make it clear they should come from something other than
> > normal C. Traditional C coding style uses all capital letters for macroses. So
> > all capital extern variables are unlikely to conflict with any normal extern
> > vars. Like vars in vmlinux and vars in other bpf elf files.
>
> But still, how many of the LINUX_KERNEL_VERSION or CONFIG_XXX vars are actually
> used per program. I bet just a handful. And I don't think adding a __kconfig is
> cumbersome, it would make it more self-documenting in fact, denoting that this
> var is not treated the usual way once prog linking is in place. Even if all
> capital letters. Tomorrow, we'd be adding 'extern unsigned long jiffies' as
> another potential example, and then it gets even more confusing on the 'collision'
> side with regular BPF ELF. Same here, instead of __kconfig, this could have a
> __vmlinux or __kernel annotation in order to document its source for the loader
> (and developer) more clearly and also gives flexibility wrt ".extern.xyz"
> subsections on how we want to map them.

Sounds reasonable and clean enough. Let me play a bit with this and
see how this all plays together.