On 02/01/2018 03:41 PM, Jesper Dangaard Brouer wrote: > On Thu, 1 Feb 2018 11:59:29 +0100 > Daniel Borkmann <dan...@iogearbox.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