diff mbox series

[bpf-next,2/2] selftests/bpf: Add a selftest for checking subreg equality

Message ID 20230416232813.2389072-1-yhs@fb.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next,1/2] bpf: Improve verifier u32 scalar equality checking | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-7 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-16
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers warning 11 maintainers not CCed: eddyz87@gmail.com mykolal@fb.com song@kernel.org shuah@kernel.org sdf@google.com haoluo@google.com john.fastabend@gmail.com kpsingh@kernel.org jolsa@kernel.org martin.lau@linux.dev linux-kselftest@vger.kernel.org
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch warning WARNING: Avoid line continuations in quoted strings WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_verifier on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-16 fail Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 fail Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32 on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-21 fail Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 fail Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-31 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on s390x with gcc

Commit Message

Yonghong Song April 16, 2023, 11:28 p.m. UTC 
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@fb.com>
---
 .../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

Comments

Eduard Zingerman April 17, 2023, 5:44 p.m. UTC | #1 
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@fb.com>

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

> ---
>  .../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";
Eduard Zingerman April 17, 2023, 5:52 p.m. UTC | #2 
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@fb.com>
> ---
>  .../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).
Yonghong Song April 17, 2023, 9:54 p.m. UTC | #3 
On 4/17/23 10:52 AM, Eduard Zingerman wrote:
> 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@fb.com>
>> ---
>>   .../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).

Thanks for the suggestion. Will add comments for some
explanation and also add a negative test.

>
diff mbox series

Patch

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";