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

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, 23 Apr 2021 16:35:09 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.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 Fri, Apr 23, 2021 at 4:26 PM Yonghong Song <yhs@...com> wrote:
>
>
>
> On 4/23/21 4:05 PM, Alexei Starovoitov wrote:
> > On Fri, Apr 23, 2021 at 2:56 PM Yonghong Song <yhs@...com> wrote:
> >>>>>
> >>>>> -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.
> >>
> >> Okay this should be fine. selftests/bpf specific. I just feel that
> >> some people may get confused if they write/see a single program in
> >> selftest and they have to use obj_varname format and thinking this
> >> is a new standard, but actually it is due to static linking buried
> >> in Makefile. Maybe add a note in selftests/README.rst so we
> >> can point to people if there is confusion.
> >
> > I'm not sure I understand.
> > Are you saying that
> > bpftool gen object out_file.o in_file.o
> > is no longer equivalent to llvm-strip ?
>
> This is more about BTF and ELF.
> Give a simple example,
> $ cat t1.c
> volatile static int aa = 10;
> int foo() { return aa; }
> $ clang -O2 -g -c -target bpf t1.c
>
> Using bpftool compiled with this patch:
> $ bpftool gen object output.o t1.o
> $ llvm-readelf -s t1.o | grep aa
>       3: 0000000000000000     4 OBJECT  LOCAL  DEFAULT     4 aa
> $ llvm-readelf -s output.o | grep aa
>       3: 0000000000000000     4 OBJECT  LOCAL  DEFAULT     4 aa
>
> $ bpftool btf dump file t1.o | grep aa
> [5] VAR 'aa' type_id=4, linkage=static
> $ bpftool btf dump file output.o | grep aa
> [5] VAR 't1..aa' type_id=4, linkage=static

Right. That's how I understood it.

> So yes you are right, this will affect skeleton user
> if you use static linker with single file.
>
> > Since during that step static vars will get their names mangled?
> > So a good chunk of code that uses skeleton right now should either
> > 1. don't do the linking step
> > or
> > 2. adjust their code to use global vars
> > or
> > 3. adjust the usage of skel.h in their corresponding user code
> >    to accommodate mangled static names?
> > Did it get it right?

It feels that the strategy of mangling every static name is too harsh.
Maybe sub-skeleton is an answer?
Something that would do a sub-skeleton for each file?
Mainly to keep "bpftool gen object output.o t1.o" from messing with btf names.
Maybe linking of btf-s from multiple .o should somehow scope them?
I don't have a concrete suggestion yet.