Message ID | 20181213034110.24361-2-jakub.kicinski@netronome.com |
---|---|
State | Changes Requested, archived |
Delegated to: | BPF Maintainers |
Headers | show |
Series | bpf: improve BPF_JSET test coverage and verifier handling | expand |
On 12/13/2018 04:41 AM, Jakub Kicinski wrote: > We seem to have no JSET instruction test, and LLVM does not > generate it at all, so let's add a simple hand-coded test > to make sure JIT implementations are correct. > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > --- > tools/testing/selftests/bpf/test_progs.c | 71 ++++++++++++++++++++++++ > 1 file changed, 71 insertions(+) > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > index 126fc624290d..d9b7197fdf94 100644 > --- a/tools/testing/selftests/bpf/test_progs.c > +++ b/tools/testing/selftests/bpf/test_progs.c > @@ -1882,6 +1882,76 @@ static void test_queue_stack_map(int type) > bpf_object__close(obj); > } > > +static void test_jset(void) > +{ > + /* LLVM does not seem to support JSET-like instructions so by hand.. */ > + static const struct bpf_insn insns[] = { > + /* r0 = 0 */ > + BPF_MOV64_IMM(BPF_REG_0, 0), > + /* prep for direct packet access via r2 */ > + BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1, > + offsetof(struct __sk_buff, data)), > + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1, > + offsetof(struct __sk_buff, data_end)), > + BPF_MOV64_REG(BPF_REG_4, BPF_REG_2), > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, 8), > + BPF_JMP_REG(BPF_JLE, BPF_REG_4, BPF_REG_3, 1), > + BPF_EXIT_INSN(), > + > + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_2, 0), > + > + /* reg, bit 63 or bit 0 set, taken */ > + BPF_LD_IMM64(BPF_REG_8, 0x8000000000000001), > + BPF_JMP_REG(BPF_JSET, BPF_REG_7, BPF_REG_8, 1), > + BPF_EXIT_INSN(), > + > + /* reg, bit 62, not taken */ > + BPF_LD_IMM64(BPF_REG_8, 0x4000000000000000), > + BPF_JMP_REG(BPF_JSET, BPF_REG_7, BPF_REG_8, 1), > + BPF_JMP_IMM(BPF_JA, 0, 0, 1), > + BPF_EXIT_INSN(), > + > + /* imm, any bit set, taken */ > + BPF_JMP_IMM(BPF_JSET, BPF_REG_7, -1, 1), > + BPF_EXIT_INSN(), > + > + /* imm, bit 31 set, taken */ > + BPF_JMP_IMM(BPF_JSET, BPF_REG_7, 0x80000000, 1), > + BPF_EXIT_INSN(), > + > + /* all good - return r0 == 2 */ > + BPF_MOV64_IMM(BPF_REG_0, 2), > + BPF_EXIT_INSN(), > + }; > + __u32 duration = 0, retval; > + __u64 data[8] = {}; > + int err, prog_fd; > + > + prog_fd = bpf_load_program(BPF_PROG_TYPE_SCHED_CLS, insns, > + ARRAY_SIZE(insns), "GPL", 0, NULL, 0); > + if (CHECK(prog_fd < 0, "jset", "load fd %d errno %d\n", prog_fd, errno)) > + return; > + > +#define TEST(val, name, res) \ > + do { \ > + data[0] = (val); \ > + err = bpf_prog_test_run(prog_fd, 1, data, sizeof(data), \ > + NULL, NULL, &retval, &duration); \ > + CHECK(err || retval != (res), (name), \ > + "err %d errno %d retval %d duration %d\n", \ > + err, errno, retval, duration); \ > + } while (0) > + > + TEST((1ULL << 63) | (1U << 31) | (1U << 0), "bit63+31+0", 2); > + TEST((1ULL << 63) | (1U << 31), "bit63+31", 2); > + TEST((1ULL << 31) | (1U << 0), "bit31+0", 2); > + TEST((__u32)-1, "u32", 2); > + TEST(~0x4000000000000000ULL, "~bit62", 2); > + TEST(0, "zero", 0); > + TEST(~0ULL, "all", 0); > +#undef TEST Could we rather extend the test_verifier infrastructure in order to be able to define data input for bpf_prog_test_run()? I think this would be very useful for future tests there as well and avoid having to duplicate or split functionality into test_progs.c instead. > +} > + > int main(void) > { > srand(time(NULL)); > @@ -1909,6 +1979,7 @@ int main(void) > test_reference_tracking(); > test_queue_stack_map(QUEUE); > test_queue_stack_map(STACK); > + test_jset(); > > printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt); > return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS; Thanks, Daniel
On Thu, 13 Dec 2018 16:17:37 +0100, Daniel Borkmann wrote: > Could we rather extend the test_verifier infrastructure in order to > be able to define data input for bpf_prog_test_run()? I think this > would be very useful for future tests there as well and avoid having > to duplicate or split functionality into test_progs.c instead. No strong feelings but if it's called test_verifier, and the sample puts no stress on the verifier it feels weird to put it there.. But okay, I will respin.. at some point :)
On 12/13/2018 07:52 PM, Jakub Kicinski wrote: > On Thu, 13 Dec 2018 16:17:37 +0100, Daniel Borkmann wrote: >> Could we rather extend the test_verifier infrastructure in order to >> be able to define data input for bpf_prog_test_run()? I think this >> would be very useful for future tests there as well and avoid having >> to duplicate or split functionality into test_progs.c instead. > > No strong feelings but if it's called test_verifier, and the sample puts > no stress on the verifier it feels weird to put it there.. But okay, I > will respin.. at some point :) Well, test_verifier is running every program that passes the verifier via bpf_prog_test_run() anyway to increase test coverage also for JIT and runtime rather than just plain verification itself. In that sense it includes testing what verifier has been rewritten, for example. I just noticed we already have this feature via 93731ef086ce ("bpf: migrate ebpf ld_abs/ld_ind tests to test_verifier"). ;-) So just adding the test code there should suffice.
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index 126fc624290d..d9b7197fdf94 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -1882,6 +1882,76 @@ static void test_queue_stack_map(int type) bpf_object__close(obj); } +static void test_jset(void) +{ + /* LLVM does not seem to support JSET-like instructions so by hand.. */ + static const struct bpf_insn insns[] = { + /* r0 = 0 */ + BPF_MOV64_IMM(BPF_REG_0, 0), + /* prep for direct packet access via r2 */ + BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1, + offsetof(struct __sk_buff, data)), + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1, + offsetof(struct __sk_buff, data_end)), + BPF_MOV64_REG(BPF_REG_4, BPF_REG_2), + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, 8), + BPF_JMP_REG(BPF_JLE, BPF_REG_4, BPF_REG_3, 1), + BPF_EXIT_INSN(), + + BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_2, 0), + + /* reg, bit 63 or bit 0 set, taken */ + BPF_LD_IMM64(BPF_REG_8, 0x8000000000000001), + BPF_JMP_REG(BPF_JSET, BPF_REG_7, BPF_REG_8, 1), + BPF_EXIT_INSN(), + + /* reg, bit 62, not taken */ + BPF_LD_IMM64(BPF_REG_8, 0x4000000000000000), + BPF_JMP_REG(BPF_JSET, BPF_REG_7, BPF_REG_8, 1), + BPF_JMP_IMM(BPF_JA, 0, 0, 1), + BPF_EXIT_INSN(), + + /* imm, any bit set, taken */ + BPF_JMP_IMM(BPF_JSET, BPF_REG_7, -1, 1), + BPF_EXIT_INSN(), + + /* imm, bit 31 set, taken */ + BPF_JMP_IMM(BPF_JSET, BPF_REG_7, 0x80000000, 1), + BPF_EXIT_INSN(), + + /* all good - return r0 == 2 */ + BPF_MOV64_IMM(BPF_REG_0, 2), + BPF_EXIT_INSN(), + }; + __u32 duration = 0, retval; + __u64 data[8] = {}; + int err, prog_fd; + + prog_fd = bpf_load_program(BPF_PROG_TYPE_SCHED_CLS, insns, + ARRAY_SIZE(insns), "GPL", 0, NULL, 0); + if (CHECK(prog_fd < 0, "jset", "load fd %d errno %d\n", prog_fd, errno)) + return; + +#define TEST(val, name, res) \ + do { \ + data[0] = (val); \ + err = bpf_prog_test_run(prog_fd, 1, data, sizeof(data), \ + NULL, NULL, &retval, &duration); \ + CHECK(err || retval != (res), (name), \ + "err %d errno %d retval %d duration %d\n", \ + err, errno, retval, duration); \ + } while (0) + + TEST((1ULL << 63) | (1U << 31) | (1U << 0), "bit63+31+0", 2); + TEST((1ULL << 63) | (1U << 31), "bit63+31", 2); + TEST((1ULL << 31) | (1U << 0), "bit31+0", 2); + TEST((__u32)-1, "u32", 2); + TEST(~0x4000000000000000ULL, "~bit62", 2); + TEST(0, "zero", 0); + TEST(~0ULL, "all", 0); +#undef TEST +} + int main(void) { srand(time(NULL)); @@ -1909,6 +1979,7 @@ int main(void) test_reference_tracking(); test_queue_stack_map(QUEUE); test_queue_stack_map(STACK); + test_jset(); printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt); return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
We seem to have no JSET instruction test, and LLVM does not generate it at all, so let's add a simple hand-coded test to make sure JIT implementations are correct. Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- tools/testing/selftests/bpf/test_progs.c | 71 ++++++++++++++++++++++++ 1 file changed, 71 insertions(+)