On Tue, Mar 24, 2020 at 10:39:55AM -0700, John Fastabend wrote:
> 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?
