On Wed, Apr 6, 2022 at 4:11 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, Apr 1, 2022 at 7:00 PM Joanne Koong <joannekoong@xxxxxx> wrote:
> >
> > From: Joanne Koong <joannelkoong@xxxxxxxxx>
> >
> > This patch adds tests for dynptrs. These include scenarios that the
> > verifier needs to reject, as well as some successful use cases of
> > dynptrs that should pass.
> >
> > Some of the failure scenarios include checking against invalid bpf_frees,
> > invalid writes, invalid reads, and invalid ringbuf API usages.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
>
> Great set of tests! Hard to keep reading 500+ lines of failing use
> cases, but seems like a lot of interesting corner cases are handled!
> Great job!
>
> > .../testing/selftests/bpf/prog_tests/dynptr.c | 303 ++++++++++
> > .../testing/selftests/bpf/progs/dynptr_fail.c | 527 ++++++++++++++++++
> > .../selftests/bpf/progs/dynptr_success.c | 147 +++++
> > 3 files changed, 977 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/dynptr.c
> > create mode 100644 tools/testing/selftests/bpf/progs/dynptr_fail.c
> > create mode 100644 tools/testing/selftests/bpf/progs/dynptr_success.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
> > new file mode 100644
> > index 000000000000..7107ebee3427
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
> > @@ -0,0 +1,303 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2022 Facebook */
> > +
> > +#include <test_progs.h>
> > +#include "dynptr_fail.skel.h"
> > +#include "dynptr_success.skel.h"
> > +
> > +size_t log_buf_sz = 1024 * 1024;
> > +
> > +enum fail_case {
> > + MISSING_FREE,
> > + MISSING_FREE_CALLBACK,
> > + INVALID_FREE1,
> > + INVALID_FREE2,
> > + USE_AFTER_FREE,
> > + MALLOC_TWICE,
> > + INVALID_MAP_CALL1,
> > + INVALID_MAP_CALL2,
> > + RINGBUF_INVALID_ACCESS,
> > + RINGBUF_INVALID_API,
> > + RINGBUF_OUT_OF_BOUNDS,
> > + DATA_SLICE_OUT_OF_BOUNDS,
> > + DATA_SLICE_USE_AFTER_FREE,
> > + INVALID_HELPER1,
> > + INVALID_HELPER2,
> > + INVALID_WRITE1,
> > + INVALID_WRITE2,
> > + INVALID_WRITE3,
> > + INVALID_WRITE4,
> > + INVALID_READ1,
> > + INVALID_READ2,
> > + INVALID_READ3,
> > + INVALID_OFFSET,
> > + GLOBAL,
> > + FREE_TWICE,
> > + FREE_TWICE_CALLBACK,
> > +};
>
> it might make sense to just pass the program name as a string instead,
> just like expected error message. This will allow more table-like
> subtest specification (I'll expand below)
>
[...]
> > +
> > + if (test__start_subtest("missing_free"))
> > + verify_fail(MISSING_FREE, obj_log_buf,
> > + "spi=0 is an unreleased dynptr");
> > +
>
> [...]
>
> > + if (test__start_subtest("free_twice_callback"))
> > + verify_fail(FREE_TWICE_CALLBACK, obj_log_buf,
> > + "arg #1 is an unacquired reference and hence cannot be released");
> > +
> > + if (test__start_subtest("success"))
> > + verify_success();
>
> so instead of manually coded set of tests, it's more "scalable" to go
> with table-driven approach. Something like
>
> struct {
> const char *prog_name;
> const char *exp_msg;
> } tests = {
> {"invalid_read2", "Expected an initialized dynptr as arg #3"},
> {"prog_success_ringbuf", NULL /* success case */},
> ...
> };
>
> then you can just succinctly:
>
> for (i = 0; i < ARRAY_SIZE(tests); i++) {
> if (!test__start_subtest(tests[i].prog_name))
> continue;
>
> if (tests[i].exp_msg)
> verify_fail(tests[i].prog_name, tests[i].exp_msg);
> else
> verify_success(tests[i].prog_name);
> }
>
> Then adding new cases would be only adding BPF code and adding a
> single line in the tests table.
>
Awesome!! I love this. This will make it a lot easier to read!
> > +
> > + free(obj_log_buf);
> > +}
[...]
>
> > +/* A dynptr can't be passed into a helper function at a non-zero offset */
> > +SEC("raw_tp/sys_nanosleep")
> > +int invalid_helper2(void *ctx)
> > +{
> > + struct bpf_dynptr ptr = {};
> > + char read_data[64] = {};
> > + __u64 x = 0;
> > +
> > + bpf_dynptr_from_mem(&x, sizeof(x), &ptr);
> > +
> > + /* this should fail */
> > + bpf_dynptr_read(read_data, sizeof(read_data), (void *)&ptr + 8, 0);
> > +
> > + return 0;
> > +}
> > +
> > +/* A data slice can't be accessed out of bounds */
> > +SEC("fentry/" SYS_PREFIX "sys_nanosleep")
>
> why switching to fentry here with this ugly SYS_PREFIX thingy?
Ooh thanks for spotting this, I forgot to switch this over. Will
definitely change this for v2!
>
[...]
