netdev - Re: [PATCH bpf-next v5 9/9] selftest/bpf: Implement tests for bpftool gen min_core

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 3 Feb 2022 16:17:09 -0500
From:   Mauricio Vásquez Bernal <mauricio@...volk.io>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Quentin Monnet <quentin@...valent.com>,
        Rafael David Tinoco <rafaeldtinoco@...il.com>,
        Lorenzo Fontana <lorenzo.fontana@...stic.co>,
        Leonardo Di Donato <leonardo.didonato@...stic.co>
Subject: Re: [PATCH bpf-next v5 9/9] selftest/bpf: Implement tests for bpftool
 gen min_core_btf

On Wed, Feb 2, 2022 at 2:50 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Fri, Jan 28, 2022 at 3:23 PM Mauricio Vásquez Bernal
> <mauricio@...volk.io> wrote:
> >
> > On Fri, Jan 28, 2022 at 5:33 PM Mauricio Vásquez <mauricio@...volk.io> wrote:
> > >
> > > This commit implements some integration tests for "BTFGen". The goal
> > > of such tests is to verify that the generated BTF file contains the
> > > expected types.
> > >
> >
> > This is not an exhaustive list of test cases. I'm not sure if this is
> > the approach we should follow to implement such tests, it seems to me
> > that checking each generated BTF file by hand is a lot of work but I
> > don't have other ideas to simplify it.
> >
> > I considered different options to write these tests:
> > 1. Use core_reloc_types.h to create a "source" BTF file with a lot of
> > types, then run BTFGen for all test_core_reloc_*.o files and use the
> > generated BTF file as btf_src_file in core_reloc.c. In other words,
> > re-run all test_core_reloc tests using a generated BTF file as source
> > instead of the "btf__core_reloc_" #name ".o" one. I think this test is
> > great because it tests the full functionality and actually checks that
> > the programs are able to run using the generated file. The problem is
> > how do we test that the BTFGen is creating an optimized file? Just
> > copying the source file without any modification will make all those
> > tests pass. We could check that the generated file is small (by
> > checking the size or the number of types) but it doesn't seem a very
> > reliable approach to me.
>
> I think this second run after minimizing BTF is a good idea. I
> wouldn't bother to check for "minimal BTF" for this case.
>

Right. Do you want this to be part of this series or can we merge later on?

> > 2. We could write some .c files with the types we expect to have on
> > the generated file and compare it with the generated file. The issue
> > here is that comparing those BTF files doesn't seem to be too
> > trivial...
>
> But I would add few realistic examples that use various combinations
> of CO-RE relocations against Linux types. Then minimize BTF and
> validate that BTF is what we expect.
>

What do you mean by "realistic examples"? Aren't the BPF programs
(that use core_reloc_types.h) I added in this commit good enough for
this test?

> As for how to compare BTFs. I've been wanting to do something like
> btf__normalize() API to renumber and resort all the BTF types into
> some "canonical" order, so that two BTFs can be actually compared and
> diffed. It might be finally the time to do that.
>
> The big complication is your decision to dump all the fields of types
> that are used by type-based relocations. I'm not convinced that's the
> best way to do this. I'd keep empty struct/union for such cases,
> actually. That would minimize the number of types and thus BTF in
> general. It also will simplify the logic of emitting minimized BTF a
> bit (all_types checks won't be necessary, I think).
>
> As I also mentioned in previous patches, for types that are only
> referenced through pointer, I'd emit FWD declaration only. Or at best
> empty struct/union.
>
> With all that, after btf__minimize() operation, comparing BTFs would
> be actually pretty easy, because we'll know the order of each type,

Why do we know the order of each type? I think the order of the types
in the generated BTF files depends on:
1. The order or the relocations on the BPF object. (I'm not sure if
the compiler generates them in the same order as they appear in the
code)
2. BTFGen implementation: types are added recursively and there is
also a hashmap in between.
3. How bpftool is invoked. bpftool gen min_core_btf ... OBJ1 OBJ2 vs
bpftool gen min_core_btf ... OBJ2 OBJ1.

What I'm saying is that given a source BTF file and a BPF object I
don't know what is the order of the output BTF file. I know we could
run the test, check the generated output and use it for the test but
it seems like "cheating" to me...

Am I missing something?

> so
> using the
> VALIDATE_RAW_BTF() (see prog_tests/btf_dedup_split.c) the tests will
> be easy and clean.
>
>
> One last thing, let's not add a new test binary (test_bpftool), let's
> keep adding more tests into test_progs.
>

Will do.

> >
> > Do you have any suggestions about it? Thanks!