Message-ID: <916e63e2-392f-2e22-327d-635deb3687c8@iogearbox.net>
Date:   Tue, 6 Feb 2018 20:05:43 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jesper Dangaard Brouer <brouer@...hat.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     netdev@...r.kernel.org, Daniel Borkmann <borkmann@...earbox.net>,
        wangnan0@...wei.com, jakub.kicinski@...ronome.com, joe@....org,
        acme@...hat.com, eric@...it.org, yhs@...com
Subject: Re: [bpf-next V2 PATCH 5/5] tools/libbpf: handle issues with bpf ELF
 objects containing .eh_frames

On 02/06/2018 06:03 PM, Jesper Dangaard Brouer wrote:
> On Tue, 6 Feb 2018 08:00:59 -0800 Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
>> On Tue, Feb 06, 2018 at 03:54:28PM +0100, Jesper Dangaard Brouer wrote:
>>> If clang >= 4.0.1 is missing the option '-target bpf', it will cause
>>> llc/llvm to create two ELF sections for "Exception Frames", with
>>> section names '.eh_frame' and '.rel.eh_frame'.
>>>
>>> The BPF ELF loader library libbpf fails when loading files with these
>>> sections.  The other in-kernel BPF ELF loader in samples/bpf/bpf_load.c,
>>> handle this gracefully. And iproute2 loader also seems to work with these
>>> "eh" sections.
>>>
>>> The issue in libbpf is caused by bpf_object__elf_collect() skip the
>>> '.eh_frame' and thus doesn't create an internal data structure
>>> pointing to this ELF section index.  Later when the relocation section
>>> '.rel.eh_frame' is processed, it tries to find the '.eh_frame' via the
>>> ELF section idx, which is that fails (in bpf_object__collect_reloc).
>>>
>>> I couldn't find a way to see that the '.rel.eh_frame' was irrelevant
>>> (that is only determined by looking at the section it reference, which
>>> we no longer have info available on).  
>>
>> but does this approach work for all extra sections and relocations emitted
>> when source is compiled with -g ?
> 
> No, but I plan to follow up and do a more complete solution later. This
> is a workaround to get the Suricata use-case working and also that
> samples/bpf/ can be loaded.

Aside from a needed fix in any case, is there a specifc reason why Suricata
cannot rely on 'clang -target bpf'? Is it asm inline headers in your case?

>> To address this case bpf_load.c does:
>>   if (shdr.sh_type == SHT_REL) {
>>           struct bpf_insn *insns;
>>
>>           /* locate prog sec that need map fixup (relocations) */
>>           if (get_sec(elf, shdr.sh_info, &ehdr, &shname_prog,
>>                       &shdr_prog, &data_prog))
>>                   continue;
>>
>>           if (shdr_prog.sh_type != SHT_PROGBITS ||
>>               !(shdr_prog.sh_flags & SHF_EXECINSTR))
>>                   continue;
>>
>> why the same approach is not applicable here?
> 
> As described above bpf_object__elf_collect() skip the "real" section
> that the relo-section want to lookup (based on the same kind of
> check), but libbpf is now missing the section idx in its internal
> structures... and thus the relo lookup of the idx fails. (bpf_load.c
> does the lookup in the ELF obj directly, thus it does not have this
> problem).

Out of curiosity, I just double checked iproute2 loader (examples/bpf/):

$ clang -O2 -g -emit-llvm -c bpf_cyclic.c -o - | llc -march=bpf -mcpu=probe -filetype=obj -o bpf_cyclic.o
$ readelf -a bpf_cyclic.o | grep "\["
  [Nr] Name              Type             Address           Offset
  [ 0]                   NULL             0000000000000000  00000000
  [ 1] .strtab           STRTAB           0000000000000000  000016b0
  [ 2] .text             PROGBITS         0000000000000000  00000040
  [ 3] 0xabccba/0        PROGBITS         0000000000000000  00000040
  [ 4] .rel0xabccba/0    REL              0000000000000000  00001120
  [ 5] classifier        PROGBITS         0000000000000000  000000e8
  [ 6] .relclassifier    REL              0000000000000000  00001130
  [ 7] maps              PROGBITS         0000000000000000  00000118
  [ 8] license           PROGBITS         0000000000000000  0000013c
  [ 9] .debug_str        PROGBITS         0000000000000000  00000140
  [10] .debug_loc        PROGBITS         0000000000000000  000003d5
  [11] .rel.debug_loc    REL              0000000000000000  00001140
  [12] .debug_abbrev     PROGBITS         0000000000000000  0000045a
  [13] .debug_info       PROGBITS         0000000000000000  0000055c
  [14] .rel.debug_info   REL              0000000000000000  000011c0
  [15] .debug_ranges     PROGBITS         0000000000000000  0000088c
  [16] .rel.debug_ranges REL              0000000000000000  000015d0
  [17] .debug_macinfo    PROGBITS         0000000000000000  000008ec
  [18] .debug_pubnames   PROGBITS         0000000000000000  000008ed
  [19] .rel.debug_pubnam REL              0000000000000000  00001650
  [20] .debug_pubtypes   PROGBITS         0000000000000000  00000954
  [21] .rel.debug_pubtyp REL              0000000000000000  00001660
  [22] .eh_frame         PROGBITS         0000000000000000  000009c0
  [23] .rel.eh_frame     REL              0000000000000000  00001670
  [24] .debug_line       PROGBITS         0000000000000000  00000a10
  [25] .rel.debug_line   REL              0000000000000000  00001690
  [26] .symtab           SYMTAB           0000000000000000  00000b08
# tc qdisc add dev lo clsact
# tc filter add dev lo ingress bpf da obj bpf_cyclic.o
# tc filter show dev lo ingress
filter protocol all pref 49152 bpf chain 0
filter protocol all pref 49152 bpf chain 0 handle 0x1 bpf_cyclic.o:[classifier] direct-action not_in_hw id 6 tag 736a8a004dead229

So no problems. What it does internally is pretty similar to what Alexei
described; for programs, they need to have ELF section header type of
SHT_PROGBITS and section header flags must match on SHF_EXECINSTR in
the relocation parsing.

Now, picking out two, and looking at the flags:

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
[...]
  [ 5] classifier        PROGBITS         0000000000000000  000000e8
       0000000000000030  0000000000000000  AX       0     0     8
[...]
  [22] .eh_frame         PROGBITS         0000000000000000  000009c0
       0000000000000050  0000000000000000   A       0     0     8
[...]
Key to Flags:
  W (write), A (alloc), X (execute), M (merge), S (strings)
  I (info), L (link order), G (group), T (TLS), E (exclude), x (unknown)
  O (extra OS processing required) o (OS specific), p (processor specific)

So .eh_frame doesn't even have SHF_EXECINSTR set. Why it cannot test on
this? Doing strcmp(name, ".rel.eh_frame") == 0 test is indeed a bit
fragile in the sense that we would also need to strcmp() all the others
listed above since libbpf could trip over them just as well. When you
check the SHT_REL sections, the target section index sits in GElf_Shdr's
.sh_info, so the only thing that would need to be done in this case is
to look up the ELF section header with index from .sh_info, get the
GElf_Shdr section header and check for a match on SHT_PROGBITS/SHF_EXECINSTR,
otherwise skip that SHT_REL section. A direct lookup of the index in
the obj would not require any complex section/index tracking or larger
rework in libbpf, hmm, what am I missing?

>> I guess we can apply this workaround as-is but it looks incomplete.
> 
> Yes, it is a workaround to move forward... it requires a larger change
> to libbpf, so it stores idx'es of skipped sections.

Thanks,
Daniel

Powered by blists - more mailing lists