Re: [PATCH v2 bpf-next 2/6] libbpf: rename static variables during linking

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Apr 23, 2021 at 1:24 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 4/23/21 11:53 AM, Andrii Nakryiko wrote:
> > Prepend <obj_name>.. prefix to each static variable in BTF info during static
> > linking. This makes them uniquely named for the sake of BPF skeleton use,
> > allowing to read/write static BPF variables from user-space. This uniqueness
> > guarantee depends on each linked file name uniqueness, of course. Double dots
> > separator was chosen both to be different (but similar) to the separator that
> > Clang is currently using for static variables defined inside functions as well
> > as to generate a natural (in libbpf parlance, at least) obj__var naming pattern
> > in BPF skeleton. Static linker also checks for static variable to already
> > contain ".." separator and skips the rename to allow multi-pass linking and not
> > keep making variable name ever increasing, if derived object name is changing on
> > each pass (as is the case for selftests).
> >
> > This patch also adds opts to bpf_linker__add_file() API, which currently
> > allows to override object name for a given file and could be extended with other
> > per-file options in the future. This is not a breaking change because
> > bpf_linker__add_file() isn't yet released officially.
> >
> > This patch also includes fixes to few selftests that are already using static
> > variables. They have to go in in the same patch to not break selftest build.
>
> "in in" => "in"

heh, I knew this would be confusing :) it's "go in" a verb and "in the
same patch" as where they go into. But I'll re-phrase in the next
version.

>
> > Keep in mind, this static variable rename only happens during static linking.
> > For any existing user of BPF skeleton using static variables nothing changes,
> > because those use cases are using variable names generated by Clang. Only new
> > users utilizing static linker might need to adjust BPF skeleton use, which
> > currently will be always new use cases. So ther is no risk of breakage.
> >
> > static_linked selftests is modified to also validate conflicting static variable
> > names are handled correctly both during static linking and in BPF skeleton.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >   tools/bpf/bpftool/gen.c                       |   2 +-
> >   tools/lib/bpf/libbpf.h                        |  12 +-
> >   tools/lib/bpf/linker.c                        | 121 +++++++++++++++++-
> >   .../selftests/bpf/prog_tests/skeleton.c       |   8 +-
> >   .../selftests/bpf/prog_tests/static_linked.c  |   8 +-
> >   .../selftests/bpf/progs/bpf_iter_test_kern4.c |   4 +-
> >   .../selftests/bpf/progs/test_check_mtu.c      |   4 +-
> >   .../selftests/bpf/progs/test_cls_redirect.c   |   4 +-
> >   .../bpf/progs/test_snprintf_single.c          |   2 +-
> >   .../selftests/bpf/progs/test_sockmap_listen.c |   4 +-
> >   .../selftests/bpf/progs/test_static_linked1.c |   6 +-
> >   .../selftests/bpf/progs/test_static_linked2.c |   4 +-
> >   12 files changed, 151 insertions(+), 28 deletions(-)
> >

[...]

> > +static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
> > +                             const struct bpf_linker_file_opts *opts,
> > +                             struct src_obj *obj)
> >   {
> >   #if __BYTE_ORDER == __LITTLE_ENDIAN
> >       const int host_endianness = ELFDATA2LSB;
> > @@ -549,6 +613,14 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
> >
> >       obj->filename = filename;
> >
> > +     if (OPTS_GET(opts, object_name, NULL)) {
> > +             strncpy(obj->obj_name, opts->object_name, MAX_OBJ_NAME_LEN);
> > +             obj->obj_name[MAX_OBJ_NAME_LEN - 1] = '\0';
>
> Looks we don't have examples/selftests which actually use this option.
> The only place to use bpf_linker__add_file() is bpftool which did not
> have option to overwrite the obj file name.
>
> The code looks fine to me though.

Right, I was a bit lazy in adding this to bpftool, but I'm sure we'll
want to support overriding obj file name, at least to resolve object
file name conflicts. One other problem is syntax, I haven't thought
through what's the best way to do this with bpftool. Something like
this would do it:

bpftool gen object dest.o input1.o=custom_obj_name
path/to/file2.o=another_custom_obj_name

But this is too bike-shedding a topic which I want to avoid for now.

>
> > +     } else {
> > +             get_obj_name(obj->obj_name, filename);
> > +     }
> > +     obj->obj_name_len = strlen(obj->obj_name);
> > +
> >       obj->fd = open(filename, O_RDONLY);
> >       if (obj->fd < 0) {
> >               err = -errno;
> > @@ -2264,6 +2336,47 @@ static int linker_append_btf(struct bpf_linker *linker, struct src_obj *obj)
> >                               obj->btf_type_map[i] = glob_sym->btf_id;
> >                               continue;
> >                       }
> > +             } else if (btf_is_var(t) && btf_var(t)->linkage == BTF_VAR_STATIC) {
> > +                     /* Static variables are renamed to include
> > +                      * "<obj_name>.." prefix (note double dots), similarly
> > +                      * to how static variables inside functions are named
> > +                      * "<func_name>.<var_name>" by compiler. This allows to
> > +                      * have  unique identifiers for static variables across
> > +                      * all linked object files (assuming unique filenames,
> > +                      * of course), which BPF skeleton relies on.
> > +                      *
> > +                      * So worst case static variable inside the function
> > +                      * will have the form "<obj_name>..<func_name>.<var_name"
> <var_name  => <var_name>

good catch, will fix

> > +                      * and will get sanitized by BPF skeleton generation
> > +                      * logic to a field with <obj_name>__<func_name>_<var_name>
> > +                      * name. Typical static variable will have a
> > +                      * <obj_name>__<var_name> name, implying arguably nice
> > +                      * per-file scoping.
> > +                      *
> > +                      * If static var name already contains '..', though,
> > +                      * don't rename it, because it was already renamed by
> > +                      * previous linker passes.
> > +                      */
> > +                     name = btf__str_by_offset(obj->btf, t->name_off);
> > +                     if (!strstr(name, "..")) {
> > +                             char new_name[MAX_VAR_NAME_LEN];
> > +
> > +                             memcpy(new_name, obj->obj_name, obj->obj_name_len);
> > +                             new_name[obj->obj_name_len] = '.';
> > +                             new_name[obj->obj_name_len + 1] = '.';
> > +                             new_name[obj->obj_name_len + 2] = '\0';
> > +                             /* -3 is for '..' separator and terminating '\0' */
> > +                             strncat(new_name, name, MAX_VAR_NAME_LEN - obj->obj_name_len - 3);
> > +
> > +                             id = btf__add_str(obj->btf, new_name);
> > +                             if (id < 0)
> > +                                     return id;
> > +
> > +                             /* btf__add_str() might invalidate t, so re-fetch */
> > +                             t = btf__type_by_id(obj->btf, i);
> > +
> > +                             ((struct btf_type *)t)->name_off = id;
> > +                     }
> >               }
> >

[...]

> > diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c
> > index ee49493dc125..43bf8ec8ae79 100644
> > --- a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c
> > +++ b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c
> > @@ -9,8 +9,8 @@ __u32 map1_id = 0, map2_id = 0;
> >   __u32 map1_accessed = 0, map2_accessed = 0;
> >   __u64 map1_seqnum = 0, map2_seqnum1 = 0, map2_seqnum2 = 0;
> >
> > -static volatile const __u32 print_len;
> > -static volatile const __u32 ret1;
> > +volatile const __u32 print_len = 0;
> > +volatile const __u32 ret1 = 0;
>
> I am little bit puzzled why bpf_iter_test_kern4.c is impacted. I think
> this is not in a static link test, right? The same for a few tests below.

All the selftests are passed through a static linker, so it will
append obj_name to each static variable. So I just minimized use of
static variables to avoid too much code churn. If this variable was
static, it would have to be accessed as
skel->rodata->bpf_iter_test_kern4__print_len, for example.

>
> >
> >   SEC("iter/bpf_map")
> >   int dump_bpf_map(struct bpf_iter__bpf_map *ctx)
> > diff --git a/tools/testing/selftests/bpf/progs/test_check_mtu.c b/tools/testing/selftests/bpf/progs/test_check_mtu.c
> > index c4a9bae96e75..71184af57749 100644
> > --- a/tools/testing/selftests/bpf/progs/test_check_mtu.c
> > +++ b/tools/testing/selftests/bpf/progs/test_check_mtu.c
> > @@ -11,8 +11,8 @@
> >   char _license[] SEC("license") = "GPL";
> >

[...]