On Tue, 2022-12-20 at 13:18 -0800, Andrii Nakryiko wrote:
> On Fri, Dec 16, 2022 at 6:17 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> >
> > A simple program that allocates a bunch of unique register ids than
> > branches. The goal is to confirm that idmap used in verifier.c:check_ids()
> > has sufficient capacity to verify that branches converge to a same state.
> >
> > Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> > ---
> > .../selftests/bpf/prog_tests/verifier.c | 12 +++
> > .../selftests/bpf/progs/check_ids_limits.c | 77 +++++++++++++++++++
> > 2 files changed, 89 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/verifier.c
> > create mode 100644 tools/testing/selftests/bpf/progs/check_ids_limits.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > new file mode 100644
> > index 000000000000..3933141928a7
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > @@ -0,0 +1,12 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include <test_progs.h>
> > +
> > +#include "check_ids_limits.skel.h"
> > +
> > +#define TEST_SET(skel) \
> > + void test_##skel(void) \
> > + { \
> > + RUN_TESTS(skel); \
> > + }
>
> Let's not use such trivial macros, please. It makes grepping for tests
> much harder and saves 1 line of code only. Let's define funcs
> explicitly?
>
> I'm also surprised it works at all (it does, right?), because Makefile
Nope, it doesn't work and it is embarrassing. I've tested w/o this
macro and only added it before final tests run. And didn't check the log.
Thank you for catching it. Will remove this macro.
> is grepping explicitly for `void (serial_)test_xxx` pattern when
> generating a list of tests. So this shouldn't have worked, unless I'm
> missing something.
>
> > +
> > +TEST_SET(check_ids_limits)
> > diff --git a/tools/testing/selftests/bpf/progs/check_ids_limits.c b/tools/testing/selftests/bpf/progs/check_ids_limits.c
> > new file mode 100644
> > index 000000000000..36c4a8bbe8ca
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/check_ids_limits.c
> > @@ -0,0 +1,77 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include "bpf_misc.h"
> > +
> > +struct map_struct {
> > + __uint(type, BPF_MAP_TYPE_ARRAY);
> > + __uint(max_entries, 1);
> > + __type(key, int);
> > + __type(value, int);
> > +} map SEC(".maps");
> > +
> > +/* Make sure that verifier.c:check_ids() can handle (almost) maximal
> > + * number of ids.
> > + */
> > +SEC("?raw_tp")
> > +__naked __test_state_freq __log_level(2) __msg("43 to 45: safe")
>
> it's not clear what's special about 43 -> 45 jump?
>
> can we also validate that id=69 was somewhere in verifier output?
> which would require multiple __msg support, of course.
>
> > +int allocate_many_ids(void)
> > +{
> > + /* Use bpf_map_lookup_elem() as a way to get a bunch of values
> > + * with unique ids.
> > + */
> > +#define __lookup(dst) \
> > + "r1 = %[map] ll;" \
> > + "r2 = r10;" \
> > + "r2 += -8;" \
> > + "call %[bpf_map_lookup_elem];" \
> > + dst " = r0;"
> > + asm volatile(
> > + "r0 = 0;"
> > + "*(u64*)(r10 - 8) = r0;"
> > + "r7 = r10;"
> > + "r8 = 0;"
> > + /* Spill 64 bpf_map_lookup_elem() results to stack,
> > + * each lookup gets its own unique id.
> > + */
> > + "write_loop:"
> > + "r7 += -8;"
> > + "r8 += -8;"
> > + __lookup("*(u64*)(r7 + 0)")
> > + "if r8 != -512 goto write_loop;"
> > + /* No way to source unique ids for r1-r5 as these
> > + * would be clobbered by bpf_map_lookup_elem call,
> > + * so make do with 64+5 unique ids.
> > + */
> > + __lookup("r6")
> > + __lookup("r7")
> > + __lookup("r8")
> > + __lookup("r9")
> > + __lookup("r0")
> > + /* Create a branching point for states comparison. */
> > +/* 43: */ "if r0 != 0 goto skip_one;"
> > + /* Read all registers and stack spills to make these
> > + * persist in the checkpoint state.
> > + */
> > + "r0 = r0;"
> > + "skip_one:"
>
> where you trying to just create a checkpoint here? given
> __test_state_freq the simplest way would be just
>
> goto +0;
>
> no?
>
> > +/* 45: */ "r0 = r6;"
> > + "r0 = r7;"
> > + "r0 = r8;"
> > + "r0 = r9;"
> > + "r0 = r10;"
> > + "r1 = 0;"
> > + "read_loop:"
> > + "r0 += -8;"
> > + "r1 += -8;"
> > + "r2 = *(u64*)(r0 + 0);"
> > + "if r1 != -512 goto read_loop;"
> > + "r0 = 0;"
> > + "exit;"
> > + :
> > + : __imm(bpf_map_lookup_elem),
> > + __imm_addr(map)
> > + : __clobber_all);
> > +#undef __lookup
> > +}
> > --
> > 2.38.2
> >
