netdev - Re: [PATCH bpf-next] selftests/bpf: remove logic duplication 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGGp+cETuvWUwET=6Mq5sWTJhi5+Rs2bw8xNP2NYZXAAuc6-Og@mail.gmail.com>
Date:   Thu, 11 Jul 2019 14:13:07 +0200
From:   Krzesimir Nowak <krzesimir@...volk.io>
To:     Andrii Nakryiko <andriin@...com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>, kernel-team@...com,
        ast@...com, Daniel Borkmann <daniel@...earbox.net>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next] selftests/bpf: remove logic duplication in test_verifier.c

On Thu, Jul 11, 2019 at 3:08 AM Andrii Nakryiko <andriin@...com> wrote:
>
> test_verifier tests can specify single- and multi-runs tests. Internally
> logic of handling them is duplicated. Get rid of it by making single run
> retval specification to be a first retvals spec.
>
> Cc: Krzesimir Nowak <krzesimir@...volk.io>
> Signed-off-by: Andrii Nakryiko <andriin@...com>

Looks good, one nit below.

Acked-by: Krzesimir Nowak <krzesimir@...volk.io>

> ---
>  tools/testing/selftests/bpf/test_verifier.c | 37 ++++++++++-----------
>  1 file changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index b0773291012a..120ecdf4a7db 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -86,7 +86,7 @@ struct bpf_test {
>         int fixup_sk_storage_map[MAX_FIXUPS];
>         const char *errstr;
>         const char *errstr_unpriv;
> -       uint32_t retval, retval_unpriv, insn_processed;
> +       uint32_t insn_processed;
>         int prog_len;
>         enum {
>                 UNDEF,
> @@ -95,16 +95,24 @@ struct bpf_test {
>         } result, result_unpriv;
>         enum bpf_prog_type prog_type;
>         uint8_t flags;
> -       __u8 data[TEST_DATA_LEN];
>         void (*fill_helper)(struct bpf_test *self);
>         uint8_t runs;
> -       struct {
> -               uint32_t retval, retval_unpriv;
> -               union {
> -                       __u8 data[TEST_DATA_LEN];
> -                       __u64 data64[TEST_DATA_LEN / 8];
> +       union {
> +               struct {

Maybe consider moving the struct definition outside to further the
removal of the duplication?

> +                       uint32_t retval, retval_unpriv;
> +                       union {
> +                               __u8 data[TEST_DATA_LEN];
> +                               __u64 data64[TEST_DATA_LEN / 8];
> +                       };
>                 };
> -       } retvals[MAX_TEST_RUNS];
> +               struct {
> +                       uint32_t retval, retval_unpriv;
> +                       union {
> +                               __u8 data[TEST_DATA_LEN];
> +                               __u64 data64[TEST_DATA_LEN / 8];
> +                       };
> +               } retvals[MAX_TEST_RUNS];
> +       };
>         enum bpf_attach_type expected_attach_type;
>  };
>
> @@ -949,17 +957,8 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>                 uint32_t expected_val;
>                 int i;
>
> -               if (!test->runs) {
> -                       expected_val = unpriv && test->retval_unpriv ?
> -                               test->retval_unpriv : test->retval;
> -
> -                       err = do_prog_test_run(fd_prog, unpriv, expected_val,
> -                                              test->data, sizeof(test->data));
> -                       if (err)
> -                               run_errs++;
> -                       else
> -                               run_successes++;
> -               }
> +               if (!test->runs)
> +                       test->runs = 1;
>
>                 for (i = 0; i < test->runs; i++) {
>                         if (unpriv && test->retvals[i].retval_unpriv)
> --
> 2.17.1
>


-- 
Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
Registergericht/Court of registration: Amtsgericht Charlottenburg
Registernummer/Registration number: HRB 171414 B
Ust-ID-Nummer/VAT ID number: DE302207000