netdev - Re: [PATCH bpf-next v3 3/8] libbpf: Add BPF_PROG_BIND_MAP syscall and use it on .metadata section

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:   Fri, 4 Sep 2020 16:18:13 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Stanislav Fomichev <sdf@...gle.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        YiFei Zhu <zhuyifei@...gle.com>,
        YiFei Zhu <zhuyifei1999@...il.com>,
        Andrey Ignatov <rdna@...com>
Subject: Re: [PATCH bpf-next v3 3/8] libbpf: Add BPF_PROG_BIND_MAP syscall and
 use it on .metadata section

On Thu, Sep 3, 2020 at 6:29 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Wed, Sep 02, 2020 at 07:31:33PM -0700, Andrii Nakryiko wrote:
> > On Fri, Aug 28, 2020 at 12:37 PM Stanislav Fomichev <sdf@...gle.com> wrote:
> > >
> > > From: YiFei Zhu <zhuyifei@...gle.com>
> > >
> > > The patch adds a simple wrapper bpf_prog_bind_map around the syscall.
> > > And when using libbpf to load a program, it will probe the kernel for
> > > the support of this syscall, and scan for the .metadata ELF section
> > > and load it as an internal map like a .data section.
> > >
> > > In the case that kernel supports the BPF_PROG_BIND_MAP syscall and
> > > a .metadata section exists, the map will be explicitly bound to
> > > the program via the syscall immediately after program is loaded.
> > > -EEXIST is ignored for this syscall.
> >
> > Here is the question I have. How important is it that all this
> > metadata is in a separate map? What if libbpf just PROG_BIND_MAP all
> > the maps inside a single BPF .o file to all BPF programs in that file?
> > Including ARRAY maps created for .data, .rodata and .bss, even if the
> > BPF program doesn't use any of the global variables? If it's too
> > extreme, we could do it only for global data maps, leaving explicit
> > map definitions in SEC(".maps") alone. Would that be terrible?
> > Conceptually it makes sense, because when you program in user-space,
> > you expect global variables to be there, even if you don't reference
> > it directly, right? The only downside is that you won't have a special
> > ".metadata" map, rather it will be part of ".rodata" one.
>
> That's an interesting idea.
> Indeed. If we have BPF_PROG_BIND_MAP command why do we need to create
> another map that behaves exactly like .rodata but has a different name?

That was exactly my thought when I re-read this patch set :)

> Wouldn't it be better to identify metadata elements some other way?
> Like by common prefix/suffix name of the variables or
> via grouping them under one structure with standard prefix?
> Like:
> struct bpf_prog_metadata_blahblah {
>   char compiler_name[];
>   int my_internal_prog_version;
> } = { .compiler_name[] = "clang v.12", ...};
>
> In the past we did this hack for 'version' and for 'license',
> but we did it because we didn't have BTF and there was no other way
> to determine the boundaries.
> I think libbpf can and should support multiple rodata sections with

Yep, that's coming, we already have a pretty common .rodata.str1.1
section emitted by Clang for some cases, which libbpf currently
ignores, but that should change. Creating a separate map for all such
small sections seems excessive, so my plan is to combine them and
their BTFs into one, as you assumed below.

> arbitrary names, but hardcoding one specific ".metadata" name?
> Hmm. Let's think through the implications.
> Multiple .o support and static linking is coming soon.
> When two .o-s with multiple bpf progs are statically linked libbpf
> won't have any choice but to merge them together under single
> ".metadata" section and single map that will be BPF_PROG_BIND_MAP-ed
> to different progs. Meaning that metadata applies to final elf file
> after linking. It's _not_ per program metadata.

Right, exactly.

> May be we should talk about problem statement and goals.
> Do we actually need metadata per program or metadata per single .o
> or metadata per final .o with multiple .o linked together?
> What is this metadata?

Yep, that's a very valid question. I've also CC'ed Andrey.

> If it's just unreferenced by program read only data then no special names or
> prefixes are needed. We can introduce BPF_PROG_BIND_MAP to bind any map to any
> program and it would be up to tooling to decide the meaning of the data in the
> map. For example, bpftool can choose to print all variables from all read only
> maps that match "bpf_metadata_" prefix, but it will be bpftool convention only
> and not hard coded in libbpf.

Agree as well. It feels a bit odd for libbpf to handle ".metadata"
specially, given libbpf itself doesn't care about its contents at all.

So thanks for bringing this up, I think this is an important discussion to have.