netdev - Re: [PATCH v4 bpf-next 0/4] Add libbpf-provided extern variables support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 15 Dec 2019 17:47:01 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v4 bpf-next 0/4] Add libbpf-provided extern variables support

On Sun, Dec 15, 2019 at 4:52 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Fri, Dec 13, 2019 at 05:47:06PM -0800, Andrii Nakryiko wrote:
> > It's often important for BPF program to know kernel version or some specific
> > config values (e.g., CONFIG_HZ to convert jiffies to seconds) and change or
> > adjust program logic based on their values. As of today, any such need has to
> > be resolved by recompiling BPF program for specific kernel and kernel
> > configuration. In practice this is usually achieved by using BCC and its
> > embedded LLVM/Clang. With such set up #ifdef CONFIG_XXX and similar
> > compile-time constructs allow to deal with kernel varieties.
> >
> > With CO-RE (Compile Once – Run Everywhere) approach, this is not an option,
> > unfortunately. All such logic variations have to be done as a normal
> > C language constructs (i.e., if/else, variables, etc), not a preprocessor
> > directives. This patch series add support for such advanced scenarios through
> > C extern variables. These extern variables will be recognized by libbpf and
> > supplied through extra .extern internal map, similarly to global data. This
> > .extern map is read-only, which allows BPF verifier to track its content
> > precisely as constants. That gives an opportunity to have pre-compiled BPF
> > program, which can potentially use BPF functionality (e.g., BPF helpers) or
> > kernel features (types, fields, etc), that are available only on a subset of
> > targeted kernels, while effectively eleminating (through verifier's dead code
> > detection) such unsupported functionality for other kernels (typically, older
> > versions). Patch #3 explicitly tests a scenario of using unsupported BPF
> > helper, to validate the approach.
> >
> > This patch set heavily relies on BTF type information emitted by compiler for
> > each extern variable declaration. Based on specific types, libbpf does strict
> > checks of config data values correctness. See patch #1 for details.
> >
> > Outline of the patch set:
> > - patch #1 does a small clean up of internal map names contants;
> > - patch #2 adds all of the libbpf internal machinery for externs support,
> >   including setting up BTF information for .extern data section;
> > - patch #3 adds support for .extern into BPF skeleton;
> > - patch #4 adds externs selftests, as well as enhances test_skeleton.c test to
> >   validate mmap()-ed .extern datasection functionality.
>
> Applied. Thanks.

Great, thanks!

>
> Looking at the tests that do mkstemp()+write() just to pass a file path
> as .kconfig_path option into bpf_object_open_opts() it feels that file only
> support for externs is unnecessary limiting. I think it will simplify

yeah, it was a bit painful :)

> tests and will make the whole extern support more flexible if in addition to
> kconfig_path bpf_object_open_opts() would support in-memory configuration.

I wanted to keep it simple for users, in case libbpf can't find config
file, to just specify its location. But given your feedback here, and
you mentioned previously that it would be nice to allow users to
specify custom kconfig-like configuration to be exposed as externs as
well, how about replacing .kconfig_path, which is a patch to config
file, with just .kconfig, which is the **contents** of config file.
That way we can support all of the above scenarios, if maybe sometime
with a tiny bit of extra work for users:

1. Override real kconfig with custom config (e.g., for testing
purposes) - just specify alternative contents.
2. Extend kconfig with some extra configuration - user will have to
read real kconfig, then append (or prepend, doesn't matter) custom
contents.

What I want to avoid is having multiple ways to do this, having to
decide whether to augment real Kconfig or completely override it, etc.
So one string-based config override is preferable for simplicity.
Agreed?