On 02/06/2018 06:03 PM, Jesper Dangaard Brouer wrote:
> On Tue, 6 Feb 2018 08:00:59 -0800 Alexei Starovoitov
> <alexei.starovoi...@gmail.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
