Message-ID: <20180201154127.17ec8996@redhat.com>
Date:   Thu, 1 Feb 2018 15:41:27 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Daniel Borkmann <daniel@...earbox.net>
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, brouer@...hat.com
Subject: Re: [bpf-next PATCH 0/5] tools/libbpf improvements and selftests

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.

$ ./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

If the ELF objfile is non-broken, it will in "non-quiet" mode also list
the programs and maps found in the file:

$ ./test_libbpf_open  ../../testing/selftests/bpf/test_xdp.o
Open BPF ELF-file with libbpf: ../../testing/selftests/bpf/test_xdp.o
Prog (count:1) section_name: xdp_tx_iptunnel
Map (count:1) name: rxcnt
Map (count:2) name: vip2tnl
Close BPF ELF-file with libbpf: ../../testing/selftests/bpf/test_xdp.o



> Regarding the TODO comments:
> 
> +# TODO: fix libbpf to load noinline functions
> +# [warning] libbpf: incorrect bpf_call opcode
> +#libbpf_open_file test_l4lb_noinline.o
> 
> Right, so this would require a newer llvm version that supports calls.
> Maybe there could be a fallback to turn the noinline into __always_inline
> for older llvms with dumping a warning to the user that this case cannot
> properly be tested due to old llvm version.

Okay, good to know... guess it will be a while before we can enable
this test then.

> +# 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?


> +# 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.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Powered by blists - more mailing lists