Re: [bpf-next PATCH 07/10] bpf: test_verifier, bpf_get_stack return value add <0
From: Alexei Starovoitov <hidden>
Date: 2020-03-26 06:33:42
Also in:
bpf
Subsystem:
bpf [general] (safe dynamic programs and tools), bpf [selftests] (test runners & infrastructure), kernel selftest framework, the rest · Maintainers:
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi, Shuah Khan, Linus Torvalds
On Tue, Mar 24, 2020 at 10:39:55AM -0700, John Fastabend wrote:
quoted hunk ↗ jump to hunk
diff --git a/tools/testing/selftests/bpf/verifier/bpf_get_stack.c b/tools/testing/selftests/bpf/verifier/bpf_get_stack.c index f24d50f..24aa6a0 100644 --- a/tools/testing/selftests/bpf/verifier/bpf_get_stack.c +++ b/tools/testing/selftests/bpf/verifier/bpf_get_stack.c@@ -7,7 +7,7 @@ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), BPF_LD_MAP_FD(BPF_REG_1, 0), BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem), - BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 28), + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 29), BPF_MOV64_REG(BPF_REG_7, BPF_REG_0), BPF_MOV64_IMM(BPF_REG_9, sizeof(struct test_val)), BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),@@ -16,6 +16,7 @@ BPF_MOV64_IMM(BPF_REG_4, 256), BPF_EMIT_CALL(BPF_FUNC_get_stack), BPF_MOV64_IMM(BPF_REG_1, 0), + BPF_JMP32_REG(BPF_JSLT, BPF_REG_0, BPF_REG_1, 20), BPF_MOV64_REG(BPF_REG_8, BPF_REG_0), BPF_ALU64_IMM(BPF_LSH, BPF_REG_8, 32), BPF_ALU64_IMM(BPF_ARSH, BPF_REG_8, 32),
Yep. The test is wrong. But this is cheating ;) JSLT should be after shifts. The test should have been written as:
diff --git a/tools/testing/selftests/bpf/verifier/bpf_get_stack.c b/tools/testing/selftests/bpf/verifier/bpf_get_stack.c
index f24d50f09dbe..be0758c1bfbd 100644
--- a/tools/testing/selftests/bpf/verifier/bpf_get_stack.c
+++ b/tools/testing/selftests/bpf/verifier/bpf_get_stack.c@@ -19,7 +19,7 @@ BPF_MOV64_REG(BPF_REG_8, BPF_REG_0), BPF_ALU64_IMM(BPF_LSH, BPF_REG_8, 32), BPF_ALU64_IMM(BPF_ARSH, BPF_REG_8, 32), - BPF_JMP_REG(BPF_JSLT, BPF_REG_1, BPF_REG_8, 16), + BPF_JMP_REG(BPF_JSGT, BPF_REG_1, BPF_REG_8, 16), BPF_ALU64_REG(BPF_SUB, BPF_REG_9, BPF_REG_8), BPF_MOV64_REG(BPF_REG_2, BPF_REG_7), BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_8),
That was the intent of the test. But in such form it won't work with the current patch set, but it should. More so the patches 1 and 5 make test_progs pass, but test_progs-no_alu32 fails tests 20 and 61. Because clang generates the following: 14: (85) call bpf_get_stack#67 15: (b7) r1 = 0 16: (bf) r8 = r0 17: (67) r8 <<= 32 18: (c7) r8 s>>= 32 19: (6d) if r1 s> r8 goto pc+16 (which is exactly what bpf_get_stack.c test tried to capture) I guess no_alu32 may be passing for you because you have that special clang optimization that replaces <<32 s>>32 with more efficient insn, but not everyone will be using new clang, so we have to teach verifier recognize <<32 s>>32. Thankfully with your new infra for s32 it should be easy to do. In scalar_min_max_lsh() we don't need to do dst_reg->smax_value = S64_MAX; When we have _positive_ s32_max_value we can set smax_value = s32_max_value << 32 and I think that will be correct. scalar_min_max_arsh() shouldn't need any changes. And the verifier will restore valid smax_value after <<32 s>>32 sequence. And this test will pass (after fixing s/JSLT/JSGT/) and test_progs-no_alu32 will do too. wdyt?