Message-ID: <CAEf4BzYZX9YJcoragK20cvQvr_tPTWYBQSRh7diKc1KoCtu4Dg@mail.gmail.com>
Date:   Mon, 26 Apr 2021 16:11:23 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Yonghong Song <yhs@...com>, Andrii Nakryiko <andrii@...nel.org>,
        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 v2 bpf-next 2/6] libbpf: rename static variables during linking

On Mon, Apr 26, 2021 at 3:34 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Mon, Apr 26, 2021 at 08:44:04AM -0700, Andrii Nakryiko wrote:
> >
> > >
> > > > Static maps are slightly different, because we use SEC() which marks
> > > > them as used, so they should always be present.
> > >
> > > yes. The used attribute makes the compiler keep the data,
> > > but it can still inline it and lose the reference in the .text.
> >
> > At least if the map is actually used with helpers (e.g.,
> > bpf_map_lookup_elem(&map, ...)) it would be invalid for compiler to do
> > anything crazy with that map reference, because compiler has no
> > visibility into what opaque helpers do with that memory. So I don't
> > think it can alias multiple maps, for instance. So I think static maps
> > should be fine.
>
> Yeah. That makes sense.
>
> > See above about passing a pointer to map into black box functions. I'd
> > bet that the compiler can't merge together two different references at
> > least because of that.
> >
> > For static maps, btw, just like for static functions and vars, there
> > is no symbol, it's an offset into .maps section. We use that offset to
> > identify the map itself.
>
> Ok. Sounds like there is a desire to expose both static and static volatile
> into skeleton.
> Sure, but let's make it such the linking step doesn't change the skeleton.
> Imagine a project that using single .bpf.c file and skeleton.
> It grows and wants to split itself into multiple .bpf.c.
> If such split would change the skeleton generated var/map names
> it would be annoying user experience.

It's surely not ideal, but it's a one-time step and only when user is
ready to switch to linker, so I don't see it as such a big problem.

>
> I see few options to avoid that:
> - keeping the btf names as-is during linking
> The final .o can have multiple vars and maps with the same name.
> The skeleton gen can see the name collision and disambiguate them.
> Here I think it's important to give users a choice. Blindly appending
> file name is not ideal.
> How to express it cleanly in .bpf.c? I don't know. SEC() would be a bit
> ugly. May be similar to core flavors? ___1 and ___2 ? Also not ideal.

___1 vs ___2 doesn't tell you which file you are accessing static
variable from, you need to go and figure out the order of linking. If
you look at bpf_linker__add_file() API, it has opts->object_name which
allows you to specify what should be used as <prefix>__. Sane default
seems to be the object name derived from filename, but it's possible
to override this. To allow end-users customize we can extend bpftool
to allow users to specify this. One way I was thinking would be
something like

bpftool gen object my_obj1.o=my_prefix1 my_obj2.o=my_prefix2

If user doesn't want prefixing (e.g., when linking multi-file BPF
library into a single .o) they would be able to disable this as:

bpftool gen object lib_file1.o= lib_file2.o= and so on

> - another option is to fail skeleton gen if names conflict.
> This way the users wold be able to link just fine and traditonal C style
> linker behavior will be preserved, but if the user wants a skeleton
> then the static map names across .bpf.c files shouldn't conflict.
> imo that's reasonable restriction.

There are two reasons to use static:
1. hide it from BPF code in other files (compilation units)
2. allow name conflicts (i.e., not care about anyone else accidentally
defining static variable with the same name)

I think both are important and I wouldn't want to give up #2. It
basically says: "no other file should interfere with my state neither
through naming or hijacking my state". Obviously it's impossible to
guard from user-space interference due to how BPF maps/progs are
visible to user-space, so those guarantees are mostly about BPF code
side.

Name prefixing only affects BPF skeleton generation and user-space use
of those static variables, both of which are highly-specific use
patterns "bridging two worlds", BPF and user-space. So I think it's
totally reasonable to specify that such variables will have naming
prefixes. Especially that BPF static variables inside functions
already use similar naming conventions and are similarly exposed in
BPF skeleton.

> - maybe adopt __hidden for vars and maps? Only not hidden (which is default now)
> would be seen in skeleton?

This is similar to the above, it gives up the ability to not care
about naming so much, because everything is forced to be global.

Powered by blists - more mailing lists