netdev - Re: [PATCH v2 bpf-next] selftests/bpf: merge most of test_btf into 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: <20200916002033.vmn5bensn6r47t4x@kafai-mbp>
Date:   Tue, 15 Sep 2020 17:20:55 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
CC:     Andrii Nakryiko <andriin@...com>, <bpf@...r.kernel.org>,
        <netdev@...r.kernel.org>, <ast@...com>, <daniel@...earbox.net>,
        <andrii.nakryiko@...il.com>, <kernel-team@...com>, <yhs@...com>
Subject: Re: [PATCH v2 bpf-next] selftests/bpf: merge most of test_btf into
 test_progs

On Tue, Sep 15, 2020 at 04:37:50PM -0700, Alexei Starovoitov wrote:
> On Mon, Sep 14, 2020 at 06:43:41PM -0700, Andrii Nakryiko wrote:
> > Move almost 200 tests from test_btf into test_progs framework to be exercised
> > regularly. Pretty-printing tests were left alone and renamed into
> > test_btf_pprint because they are very slow and were not even executed by
> > default with test_btf.
> 
> I think would be good to run them by default.
> The following trivial tweak makes them fast:
> diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
> index c75fc6447186..589afd4f0e47 100644
> --- a/tools/testing/selftests/bpf/test_btf.c
> +++ b/tools/testing/selftests/bpf/test_btf.c
> @@ -4428,7 +4428,7 @@ static struct btf_raw_test pprint_test_template[] = {
>         .value_size = sizeof(struct pprint_mapv),
>         .key_type_id = 3,       /* unsigned int */
>         .value_type_id = 16,    /* struct pprint_mapv */
> -       .max_entries = 128 * 1024,
> +       .max_entries = 128,
>  },
> 
>  {
> @@ -4493,7 +4493,7 @@ static struct btf_raw_test pprint_test_template[] = {
>         .value_size = sizeof(struct pprint_mapv),
>         .key_type_id = 3,       /* unsigned int */
>         .value_type_id = 16,    /* struct pprint_mapv */
> -       .max_entries = 128 * 1024,
> +       .max_entries = 128,
>  },
> 
>  {
> @@ -4564,7 +4564,7 @@ static struct btf_raw_test pprint_test_template[] = {
>         .value_size = sizeof(struct pprint_mapv),
>         .key_type_id = 3,       /* unsigned int */
>         .value_type_id = 16,    /* struct pprint_mapv */
> -       .max_entries = 128 * 1024,
> +       .max_entries = 128,
>  },
> 
> Martin,
> do you remember why you picked such large numbers of entries
> for the test?
It has been a while.  iirc, I think there was no particular reason but
there was only one pprint test and then a few had been added since
then.

> If I read the code correctly smaller number doesn't reduce the test coverage.
Indeed.  Please go ahead to reduce the max_entries.  Sorry for the pain.

> 
> > All the test_btf tests that were moved are modeled as proper sub-tests in
> > test_progs framework for ease of debugging and reporting.
> > 
> > No functional or behavioral changes were intended, I tried to preserve
> > original behavior as close to the original as possible. `test_progs -v` will
> > activate "always_log" flag to emit BTF validation log.
> > 
> > Signed-off-by: Andrii Nakryiko <andriin@...com>
> > ---
> > 
> > v1->v2:
> >  - pretty-print BTF tests were renamed test_btf -> test_btf_pprint, which
> >    allowed GIT to detect that majority of  test_btf code was moved into
> >    prog_tests/btf.c; so diff is much-much smaller;
> 
> Thanks. I hope with addition to pprint test the diff will be even smaller.
> I think it's worth to investigate why they're failing if moved to test_progs.
> I think they're the only tests that exercise seq_read logic.
> Clearly the bug:
> [   25.960993] WARNING: CPU: 2 PID: 1995 at kernel/bpf/hashtab.c:717 htab_map_get_next_key+0x7fc/0xab0
> is still there.
> If pprint tests were part of test_progs we would have caught that earlier.
> 
> Yonghong,
> please take a look at that issue.