| 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
| ||
|
Date: Thu, 11 Jul 2019 14:17:44 +0200
From: Krzesimir Nowak <krzesimir@...volk.io>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: open list <linux-kernel@...r.kernel.org>,
Alban Crequy <alban@...volk.io>,
Iago López Galeiras <iago@...volk.io>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <jakub.kicinski@...ronome.com>,
Jesper Dangaard Brouer <hawk@...nel.org>,
John Fastabend <john.fastabend@...il.com>,
Stanislav Fomichev <sdf@...gle.com>,
Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
xdp-newbies@...r.kernel.org
Subject: Re: [bpf-next v3 05/12] selftests/bpf: Allow passing more information
to BPF prog test run
On Thu, Jul 11, 2019 at 3:17 AM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Mon, Jul 8, 2019 at 3:42 PM Krzesimir Nowak <krzesimir@...volk.io> wrote:
> >
> > The test case can now specify a custom length of the data member,
> > context data and its length, which will be passed to
> > bpf_prog_test_run_xattr. For backward compatilibity, if the data
> > length is 0 (which is what will happen when the field is left
> > unspecified in the designated initializer of a struct), then the
> > length passed to the bpf_prog_test_run_xattr is TEST_DATA_LEN.
> >
> > Also for backward compatilibity, if context data length is 0, NULL is
> > passed as a context to bpf_prog_test_run_xattr. This is to avoid
> > breaking other tests, where context data being NULL and context data
> > length being 0 is handled differently from the case where context data
> > is not NULL and context data length is 0.
> >
> > Custom lengths still can't be greater than hardcoded 64 bytes for data
> > and 192 for context data.
> >
> > 192 for context data was picked to allow passing struct
> > bpf_perf_event_data as a context for perf event programs. The struct
> > is quite large, because it contains struct pt_regs.
> >
> > Test runs for perf event programs will not allow the copying the data
> > back to data_out buffer, so they require data_out_size to be zero and
> > data_out to be NULL. Since test_verifier hardcodes it, make it
> > possible to override the size. Overriding the size to zero will cause
> > the buffer to be NULL.
> >
> > Changes since v2:
> > - Allow overriding the data out size and buffer.
> >
> > Signed-off-by: Krzesimir Nowak <krzesimir@...volk.io>
> > ---
> > tools/testing/selftests/bpf/test_verifier.c | 105 +++++++++++++++++---
> > 1 file changed, 93 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index 1640ba9f12c1..6f124cc4ee34 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -54,6 +54,7 @@
> > #define MAX_TEST_RUNS 8
> > #define POINTER_VALUE 0xcafe4all
> > #define TEST_DATA_LEN 64
> > +#define TEST_CTX_LEN 192
> >
> > #define F_NEEDS_EFFICIENT_UNALIGNED_ACCESS (1 << 0)
> > #define F_LOAD_WITH_STRICT_ALIGNMENT (1 << 1)
> > @@ -96,7 +97,12 @@ struct bpf_test {
> > enum bpf_prog_type prog_type;
> > uint8_t flags;
> > __u8 data[TEST_DATA_LEN];
> > + __u32 data_len;
> > + __u8 ctx[TEST_CTX_LEN];
> > + __u32 ctx_len;
> > void (*fill_helper)(struct bpf_test *self);
> > + bool override_data_out_len;
> > + __u32 overridden_data_out_len;
> > uint8_t runs;
> > struct {
> > uint32_t retval, retval_unpriv;
> > @@ -104,6 +110,9 @@ struct bpf_test {
> > __u8 data[TEST_DATA_LEN];
> > __u64 data64[TEST_DATA_LEN / 8];
> > };
> > + __u32 data_len;
> > + __u8 ctx[TEST_CTX_LEN];
> > + __u32 ctx_len;
> > } retvals[MAX_TEST_RUNS];
> > };
> >
> > @@ -818,21 +827,35 @@ static int set_admin(bool admin)
> > }
> >
> > static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> > - void *data, size_t size_data)
> > + void *data, size_t size_data, void *ctx,
> > + size_t size_ctx, u32 *overridden_data_out_size)
> > {
> > - __u8 tmp[TEST_DATA_LEN << 2];
> > - __u32 size_tmp = sizeof(tmp);
> > - int saved_errno;
> > - int err;
> > struct bpf_prog_test_run_attr attr = {
> > .prog_fd = fd_prog,
> > .repeat = 1,
> > .data_in = data,
> > .data_size_in = size_data,
> > - .data_out = tmp,
> > - .data_size_out = size_tmp,
> > + .ctx_in = ctx,
> > + .ctx_size_in = size_ctx,
> > };
> > + __u8 tmp[TEST_DATA_LEN << 2];
> > + __u32 size_tmp = sizeof(tmp);
> > + __u32 size_buf = size_tmp;
> > + __u8 *buf = tmp;
> > + int saved_errno;
> > + int err;
> >
> > + if (overridden_data_out_size)
> > + size_buf = *overridden_data_out_size;
> > + if (size_buf > size_tmp) {
> > + printf("FAIL: out data size (%d) greater than a buffer size (%d) ",
> > + size_buf, size_tmp);
> > + return -EINVAL;
> > + }
> > + if (!size_buf)
> > + buf = NULL;
> > + attr.data_size_out = size_buf;
> > + attr.data_out = buf;
> > if (unpriv)
> > set_admin(true);
> > err = bpf_prog_test_run_xattr(&attr);
> > @@ -956,13 +979,45 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> > if (!alignment_prevented_execution && fd_prog >= 0) {
> > uint32_t expected_val;
> > int i;
> > + __u32 size_data;
> > + __u32 size_ctx;
> > + bool bad_size;
> > + void *ctx;
> > + __u32 *overridden_data_out_size;
> >
> > if (!test->runs) {
> > + if (test->data_len > 0)
> > + size_data = test->data_len;
> > + else
> > + size_data = sizeof(test->data);
> > + if (test->override_data_out_len)
> > + overridden_data_out_size = &test->overridden_data_out_len;
> > + else
> > + overridden_data_out_size = NULL;
> > + size_ctx = test->ctx_len;
> > + bad_size = false;
>
> I hated all this duplication of logic, which with this patch becomes
> even more expansive, so I removed it. Please see [0]. Can you please
> apply that patch and add all this new logic only once?
>
> [0] https://patchwork.ozlabs.org/patch/1130601/
Will do.
>
> > 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 (size_data > sizeof(test->data)) {
> > + printf("FAIL: data size (%u) greater than TEST_DATA_LEN (%lu) ", size_data, sizeof(test->data));
> > + bad_size = true;
> > + }
> > + if (size_ctx > sizeof(test->ctx)) {
> > + printf("FAIL: ctx size (%u) greater than TEST_CTX_LEN (%lu) ", size_ctx, sizeof(test->ctx));
>
> These look like way too long lines, wrap them?
Ah, yeah, these can be wrapped easily. Will do.
>
> > + bad_size = true;
> > + }
> > + if (size_ctx)
> > + ctx = test->ctx;
> > + else
> > + ctx = NULL;
>
> nit: single line:
>
> ctx = size_ctx ? test->ctx : NULL;
>
> > + if (bad_size)
> > + err = 1;
> > + else
> > + err = do_prog_test_run(fd_prog, unpriv, expected_val,
> > + test->data, size_data,
> > + ctx, size_ctx,
> > + overridden_data_out_size);
> > if (err)
> > run_errs++;
> > else
> > @@ -970,14 +1025,40 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> > }
> >
> > for (i = 0; i < test->runs; i++) {
> > + if (test->retvals[i].data_len > 0)
> > + size_data = test->retvals[i].data_len;
> > + else
> > + size_data = sizeof(test->retvals[i].data);
> > + if (test->override_data_out_len)
> > + overridden_data_out_size = &test->overridden_data_out_len;
> > + else
> > + overridden_data_out_size = NULL;
> > + size_ctx = test->retvals[i].ctx_len;
> > + bad_size = false;
> > if (unpriv && test->retvals[i].retval_unpriv)
> > expected_val = test->retvals[i].retval_unpriv;
> > else
> > expected_val = test->retvals[i].retval;
> >
> > - err = do_prog_test_run(fd_prog, unpriv, expected_val,
> > - test->retvals[i].data,
> > - sizeof(test->retvals[i].data));
> > + if (size_data > sizeof(test->retvals[i].data)) {
> > + printf("FAIL: data size (%u) at run %i greater than TEST_DATA_LEN (%lu) ", size_data, i + 1, sizeof(test->retvals[i].data));
> > + bad_size = true;
> > + }
> > + if (size_ctx > sizeof(test->retvals[i].ctx)) {
> > + printf("FAIL: ctx size (%u) at run %i greater than TEST_CTX_LEN (%lu) ", size_ctx, i + 1, sizeof(test->retvals[i].ctx));
> > + bad_size = true;
> > + }
> > + if (size_ctx)
> > + ctx = test->retvals[i].ctx;
> > + else
> > + ctx = NULL;
> > + if (bad_size)
> > + err = 1;
> > + else
> > + err = do_prog_test_run(fd_prog, unpriv, expected_val,
> > + test->retvals[i].data, size_data,
> > + ctx, size_ctx,
> > + overridden_data_out_size);
> > if (err) {
> > printf("(run %d/%d) ", i + 1, test->runs);
> > run_errs++;
> > --
> > 2.20.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
Powered by blists - more mailing lists