netdev - Re: [PATCH bpf-next 1/3] bpftool: add extra CO-RE mode to btf dump command

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]

Message-ID: <CAEf4BzYKf=+WNZv5HMv=W8robWWTab1L5NURAT=N7LQNW4oeGQ@mail.gmail.com>
Date:   Thu, 19 Dec 2019 13:07:38 -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 bpf-next 1/3] bpftool: add extra CO-RE mode to btf dump command

On Thu, Dec 19, 2019 at 9:06 AM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Wed, Dec 18, 2019 at 11:06:56PM -0800, Andrii Nakryiko wrote:
> > +     if (core_mode) {
> > +             printf("#if defined(__has_attribute) && __has_attribute(preserve_access_index)\n");
> > +             printf("#define __CLANG_BPF_CORE_SUPPORTED\n");
> > +             printf("#pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)\n");
> > +             printf("#endif\n\n");
>
> I think it's dangerous to automatically opt-out when clang is not new enough.
> bpf prog will compile fine, but it will be missing co-re relocations.
> How about doing something like:
>   printf("#ifdef NEEDS_CO_RE\n");
>   printf("#pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)\n");
>   printf("#endif\n\n");
> and emit it always when 'format c'.
> Then on the program side it will look:
> #define NEEDS_CO_RE
> #include "vmlinux.h"
> If clang is too old there will be a compile time error which is a good thing.
> Future features will have different NEEDS_ macros.

Wouldn't it be cleaner to separate vanilla C types dump vs
CO-RE-specific one? I'd prefer to have them separate and not require
every application to specify this #define NEEDS_CO_RE macro.
Furthermore, later we probably are going to add some additional
auto-generated types, definitions, etc, so plain C types dump and
CO-RE-specific one will deviate quite a bit. So it feels cleaner to
separate them now instead of polluting `format c` with irrelevant
noise.

I can unconditionally assume preserve_access_index availability,
though, because Clang 10 release is going to have all those features
needed for BPF CO-RE. I can also add nicer compiler error, if this
feature is not detected. Ok?

BTW, the reason I added this opt-out is because if you use
bpf_core_read() and BPF_CORE_READ() macros, you don't really need
those structs marked as relocatable. But again, I think it's fine to
just assume it has to be supported by compiler.