Date:   Thu, 1 Feb 2018 18:56:14 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     netdev@...r.kernel.org, Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        wangnan0@...wei.com, acme@...hat.com, joe@....org,
        jakub.kicinski@...ronome.com, eric@...it.org
Subject: Re: [bpf-next PATCH 0/5] tools/libbpf improvements and selftests

On 02/01/2018 03:41 PM, Jesper Dangaard Brouer wrote:
> On Thu, 1 Feb 2018 11:59:29 +0100
> Daniel Borkmann <daniel@...earbox.net> wrote:
> 
>> Hi Jesper,
>>
>> On 01/27/2018 06:26 PM, Jesper Dangaard Brouer wrote:
>>> While playing with using libbpf for the Suricata project, we had
>>> issues LLVM >= 4.0.1 generating ELF files that could not be loaded
>>> with libbpf (tools/lib/bpf/).
>>>
>>> During the troubleshooting phase, I wrote a test program and improved
>>> the debugging output in libbpf.  I turned this into a selftests
>>> program, and it also serves as a code example for libbpf in itself.
>>>
>>> I discovered that there are at least three ELF load issues with
>>> libbpf.  I left them as TODO comments in (tools/testing/selftests/bpf)
>>> test_libbpf.sh. I've only fixed the load issue with eh_frames.  We can
>>> work on the other issues later.  
>>
>> The series looks great, and given the fix, we should get this into bpf.
>>
>> Only one small request, could you move the test_libbpf_open.c into the
>> BPF selftests as well? We have test_libbpf.sh there which is the only
>> one making use of test_libbpf_open.c, so I think it fits much better if
>> we put them both into selftests. Otherwise rest is fine, thanks!
> 
> I'm happy that you noticed, but I will argue that the location of
> test_libbpf_open.c is the right place.
> 
> I deliberately placed test_libbpf_open.c in tools/lib/bpf/ which is
> together with the library that it uses, because it serves as an example
> of howto use the library libbpf.
> 
> Plus, it is functional on its own. When people on the mailing list
> report issues with libbpf, we can ask them to run the tool on their
> bpf-elf objfile and quickly figure out that is wrong.

Don't get me wrong, I'm not at all against such tool or test, I think
it's a great idea and needed. I just think that tools/lib/bpf/ is not
the right place to put it into lib directory. Right now, as you say,
it's a mixture of example code on how to use the lib, and tool at the
same time to dump/test load an object file with libbpf.

I think there are a couple of options depending on the main use case:
sample, pure test case, or tool. I think the latter would be the most
fitting and useful in fact. Often when having loader issues (like the
one you ran into in your last patch), then 'readelf -a' helps to check
out sections and their correlations, such a tool could display it more
user friendly, leaving out the unrelated bits, and could do a dry-load
as well at the same time.

Then lets integrate such an object dump into bpftool, and run it from
the BPF selftests. This would be most beneficial, imo. bpftool already
uses libbpf as a loader and it could provide a new subcommand for the
dump e.g. 'bpftool prog dump object FILE' to debug the contents of it.
At the same time we could also have a dry-run for loading the object,
and given 49a086c201a9 ("bpftool: implement prog load command"), we're
basically there already. It could be a 'bpftool probe OBJ' that would
dump the verifier log unconditionally and in case of success just
closes the prog and exits again. I think this would bring the most
benefit for users to have this functionality integrated in bpftool.
Could you change the few bits to integrate it there? That would be
really great.

> $ ./test_libbpf_open --debug ../../../samples/bpf/tracex1_kern.o 
> Open BPF ELF-file with libbpf: ../../../samples/bpf/tracex1_kern.o
> [debug] libbpf: loading ../../../samples/bpf/tracex1_kern.o
> [debug] libbpf: section(1) .strtab, size 119, link 0, flags 0, type=3
> [debug] libbpf: skip section(1) .strtab
> [debug] libbpf: section(2) .text, size 0, link 0, flags 6, type=1
> [debug] libbpf: skip section(2) .text
> [debug] libbpf: section(3) kprobe/__netif_receive_skb_core, size 352, link 0, flags 6, type=1
> [debug] libbpf: found program kprobe/__netif_receive_skb_core
> [debug] libbpf: section(4) .rodata.str1.1, size 15, link 0, flags 32, type=1
> [debug] libbpf: skip section(4) .rodata.str1.1
> [debug] libbpf: section(5) license, size 4, link 0, flags 3, type=1
> [debug] libbpf: license of ../../../samples/bpf/tracex1_kern.o is GPL
> [debug] libbpf: section(6) version, size 4, link 0, flags 3, type=1
> [debug] libbpf: kernel version of ../../../samples/bpf/tracex1_kern.o is 40f00
> [debug] libbpf: section(7) .eh_frame, size 40, link 0, flags 2, type=1
> [debug] libbpf: skip section(7) .eh_frame
> [debug] libbpf: section(8) .rel.eh_frame, size 16, link 9, flags 0, type=9
> [debug] libbpf: section(9) .symtab, size 144, link 1, flags 0, type=2
> [warning] libbpf: relocation failed: no section(7)
> Unable to load eBPF objects in file '../../../samples/bpf/tracex1_kern.o': Relocation failed
[...]
>> +# TODO: fix test_xdp_meta.c to load with libbpf
>> +# [warning] libbpf: test_xdp_meta.o doesn't provide kernel version
>> +#libbpf_open_file test_xdp_meta.o
>>
>> The kernel version is only required for tracing programs, although
>> it's just fine as well to put a dummy 'int _version SEC("version") = 1;'
>> here. The test_xdp_meta.sh uses iproute2 for loading into the kernel,
>> but no objections to add a version section there.
> 
> A lot of bpf-prog does not seems to set the version section, and other
> loaders seems to ignore this ... maybe we should remove this
> requirement from libbpf?

I don't really have a strong opinion on that one. For tracing programs,
this is basically required. I think it's good that bpf_object__validate()
would warn about it and return an explicit error (-LIBBPF_ERRNO__KVERSION)
to the user, in case of kernel, it's -EINVAL, which doesn't really say
much what went wrong (aka 'missing kernel version'). Maybe libbpf API
could support both options, though, so that in case of networking this
can just be ignored.

>> +# TODO: fix libbpf to handle .eh_frame
>> +# [warning] libbpf: relocation failed: no section(10)
>> +#libbpf_open_file ../../../../samples/bpf/tracex3_kern.o
>>
>> This is resolved in your last patch then, right? So the two could just
>> be swapped and this one uncommented, although it would require that
>> tracex3_kern.o has been built earlier.
> 
> Yes, after my patch, we can enable this test, but as you write this
> would require creating a Makefile build dependency, to samples/bpf,
> like we have for tools/lib/bpf ... but I find that rather ugly, so I
> didn't enable the test.
> 
> Either we should create a test-BPF prog in selftests/bpf/ that gets
> compiled in a wrong fashion, or say this case will be covered by other
> BPF-progs.

That makes sense, potentially the existing progs could be compiled and
test-loaded with both variants of invoking clang and passing to llc.

Thanks,
Daniel

Powered by blists - more mailing lists