On Sun, 2023-04-16 at 16:28 -0700, Yonghong Song wrote:
> Add a selftest to ensure subreg equality if source register
> upper 32bit is 0. Without previous patch, the new test will
> fail verification.
>
> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> ---
> .../selftests/bpf/prog_tests/verifier.c | 2 ++
> .../selftests/bpf/progs/verifier_reg_equal.c | 27 +++++++++++++++++++
> 2 files changed, 29 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/progs/verifier_reg_equal.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> index 73dff693d411..25bc8958dbfe 100644
> --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> @@ -31,6 +31,7 @@
> #include "verifier_meta_access.skel.h"
> #include "verifier_raw_stack.skel.h"
> #include "verifier_raw_tp_writable.skel.h"
> +#include "verifier_reg_equal.skel.h"
> #include "verifier_ringbuf.skel.h"
> #include "verifier_spill_fill.skel.h"
> #include "verifier_stack_ptr.skel.h"
> @@ -95,6 +96,7 @@ void test_verifier_masking(void) { RUN(verifier_masking); }
> void test_verifier_meta_access(void) { RUN(verifier_meta_access); }
> void test_verifier_raw_stack(void) { RUN(verifier_raw_stack); }
> void test_verifier_raw_tp_writable(void) { RUN(verifier_raw_tp_writable); }
> +void test_verifier_reg_equal(void) { RUN(verifier_reg_equal); }
> void test_verifier_ringbuf(void) { RUN(verifier_ringbuf); }
> void test_verifier_spill_fill(void) { RUN(verifier_spill_fill); }
> void test_verifier_stack_ptr(void) { RUN(verifier_stack_ptr); }
> diff --git a/tools/testing/selftests/bpf/progs/verifier_reg_equal.c b/tools/testing/selftests/bpf/progs/verifier_reg_equal.c
> new file mode 100644
> index 000000000000..91e42dec89ad
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/verifier_reg_equal.c
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_misc.h"
> +
> +SEC("socket")
> +__description("check w reg equal if r reg upper32 bits 0")
> +__success
> +__naked void subreg_equality(void)
> +{
> + asm volatile (" \
> + call %[bpf_ktime_get_ns]; \
> + *(u64 *)(r10 - 8) = r0; \
> + r2 = *(u32 *)(r10 - 8); \
> + w3 = w2; \
> + if w2 < 9 goto l0_%=; \
> + exit; \
> +l0_%=: if r3 < 9 goto l1_%=; \
> + r0 -= r1; \
> +l1_%=: exit; \
> +" :
> + : __imm(bpf_ktime_get_ns)
> + : __clobber_all);
> +}
> +
> +char _license[] SEC("license") = "GPL";
Maybe add a few comments in the test case?
E.g.:
--- a/tools/testing/selftests/bpf/progs/verifier_reg_equal.c
+++ b/tools/testing/selftests/bpf/progs/verifier_reg_equal.c
@@ -13,10 +13,16 @@ __naked void subreg_equality(void)
call %[bpf_ktime_get_ns]; \
*(u64 *)(r10 - 8) = r0; \
r2 = *(u32 *)(r10 - 8); \
+ /* At this point upper 4-bytes of r2 are 0, \
+ * thus the w3 = w2 should propagate register id, \
+ * so that w2 < 9 comparison would also propagate \
+ * range for r3. \
+ */ \
w3 = w2; \
if w2 < 9 goto l0_%=; \
exit; \
l0_%=: if r3 < 9 goto l1_%=; \
+ /* r1 read is illegal at this point */ \
r0 -= r1; \
l1_%=: exit; \
" :
Also, do we need a negative test?
(E.g. like this one but with r2 = r0 w/o u32 read from stack).
