netdev - Re: [PATCH bpf-next] selftests/bpf: enable (uncomment) all tests in test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20181022190853.4f4b1f89@redhat.com>
Date:   Mon, 22 Oct 2018 19:08:53 +0200
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Quentin Monnet <quentin.monnet@...ronome.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        oss-drivers@...ronome.com, brouer@...hat.com
Subject: Re: [PATCH bpf-next] selftests/bpf: enable (uncomment) all tests in
 test_libbpf.sh

On Mon, 22 Oct 2018 11:00:27 +0100
Quentin Monnet <quentin.monnet@...ronome.com> wrote:

> 2018-10-21 23:04 UTC+0200 ~ Jesper Dangaard Brouer <brouer@...hat.com>
> > On Sun, 21 Oct 2018 16:37:08 +0100
> > Quentin Monnet <quentin.monnet@...ronome.com> wrote:
> >   
> >> 2018-10-21 11:57 UTC+0200 ~ Jesper Dangaard Brouer <brouer@...hat.com>  
> >>> On Sat, 20 Oct 2018 23:00:24 +0100
> >>> Quentin Monnet <quentin.monnet@...ronome.com> wrote:
> >>>     
> >>
> >> [...]
> >>  
> >>>> --- a/tools/testing/selftests/bpf/test_libbpf.sh
> >>>> +++ b/tools/testing/selftests/bpf/test_libbpf.sh
> >>>> @@ -33,17 +33,11 @@ trap exit_handler 0 2 3 6 9
> >>>>   
> >>>>   libbpf_open_file test_l4lb.o
> >>>>   
> >>>> -# TODO: fix libbpf to load noinline functions
> >>>> -# [warning] libbpf: incorrect bpf_call opcode
> >>>> -#libbpf_open_file test_l4lb_noinline.o
> >>>> +libbpf_open_file test_l4lb_noinline.o
> >>>>   
> >>>> -# 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
> >>>> +libbpf_open_file test_xdp_meta.o
> >>>>   
> >>>> -# TODO: fix libbpf to handle .eh_frame
> >>>> -# [warning] libbpf: relocation failed: no section(10)
> >>>> -#libbpf_open_file ../../../../samples/bpf/tracex3_kern.o
> >>>> +libbpf_open_file ../../../../samples/bpf/tracex3_kern.o    
> >>>
> >>> I don't like the ../../../../samples/bpf/ reference (even-through I
> >>> added this TODO), as the kselftests AFAIK support installing the
> >>> selftests and then this tests will fail.
> >>> Maybe we can find another example kern.o file?
> >>> (which isn't compiled with -target bpf)    
> >>
> >> Hi Jesper, yeah maybe making the test rely on something from samples/bpf
> >> instead of just the selftests/bpf directory is not a good idea. But
> >> there is no program compiled without the "-target-bpf" in that
> >> directory. What we could do is explicitly compile one without the flag
> >> in the Makefile, as in the patch below, but I am not sure that doing so
> >> is acceptable?  
> > 
> > I think it makes sense to have a test program compiled without the
> > "-target-bpf", as that will happen for users.  And I guess we can add
> > some more specific test that are related to "-target-bpf".  
> 
> Alright, I can repost my second version that takes a test out of the
> default target for building BPF programs, after the merge window.
> 

Okay, guess there is no rush in getting this in now, and we can wait
for after the merge window.


> >> Or should tests for libbpf have a directory of their own,
> >> with another Makefile?  
> > 
> > Hmm, I'm not sure about that idea.
> > 
> > I did plan by naming the test "libbpf_open_file", what we add more
> > libbpf_ prefixed tests to the test_libbpf.sh script, which should
> > cover more aspects of the _base_ libbpf functionality.
> >   
> >> Another question regarding the test with test_xdp_meta.o: does the fix I
> >> suggested (setting a version in the .C file) makes sense, or did you
> >> leave this test for testing someday that libbpf would be able to open
> >> even programs that do not set a version (in which case this is still not
> >> the case if program type is not provided, and in fact my fix ruins
> >> everything? :s).  
> > 
> > Well, yes.  I was hinting if we should relax the version requirement
> > for e.g. XDP BPF progs.  
> 
> This is already the case. What happens for this test is that we never
> tell libbpf that this program is XDP, we just ask it to open the ELF
> file and the whole time libbpf treats it as a program of type
> BPF_PROG_TYPE_UNSPEC. So we can fix the BPF source (by adding a version)
> or we can fix test_libbpf_open.c (to tell libbpf this is XDP), but I
> don't believe there is anything to add to libbpf in that regard. I think
> we could simply remove the test on test_xdp_meta.o from test_libbpf.h,
> actually. What is you opinion?

Yes, we should likely just drop the test then.

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