| 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
| ||
|
Message-ID: <CAGGp+cHaV1EMXqeQvKN-p5gEZWcSgGfcbKimcS+C8u=dfeU=1Q@mail.gmail.com>
Date: Fri, 12 Jul 2019 09:53:12 +0200
From: Krzesimir Nowak <krzesimir@...volk.io>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: Andrii Nakryiko <andriin@...com>, Kernel Team <kernel-team@...com>,
Alexei Starovoitov <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 4:43 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Thu, Jul 11, 2019 at 5:13 AM Krzesimir Nowak <krzesimir@...volk.io> wrote:
> >
> > 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?
>
> Can't do that because then retval/retval_unpriv/data won't be
> accessible as a normal field of struct bpf_test. It has to be in
> anonymous structs/unions, unfortunately.
>
Ah, right.
Meh.
I tried something like this:
#define BPF_DATA_STRUCT \
struct { \
uint32_t retval, retval_unpriv; \
union { \
__u8 data[TEST_DATA_LEN]; \
__u64 data64[TEST_DATA_LEN / 8]; \
}; \
}
and then:
union {
BPF_DATA_STRUCT;
BPF_DATA_STRUCT retvals[MAX_TEST_RUNS];
};
And that seems to compile at least. But question is: is this
acceptably ugly or unacceptably ugly? :)
> I tried the following, but that also didn't work:
>
> union {
> struct bpf_test_retval {
> uint32_t retval, retval_unpriv;
> union {
> __u8 data[TEST_DATA_LEN];
> __u64 data64[TEST_DATA_LEN / 8];
> };
> };
> struct bpf_test_retval retvals[MAX_TEST_RUNS];
> };
>
> This also made retval/retval_unpriv to not behave as normal fields of
> struct bpf_test.
>
>
> >
> > > + 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
--
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
Powered by blists - more mailing lists