[PATCH bpf-next] selftests/bpf: Fix test_varlen verification failure with latest llvm

[PATCH bpf-next] selftests/bpf: Fix test_varlen verification failure with latest llvm

[Yonghong Song]

With latest llvm15, test_varlen failed with the following verifier log:

  17: (85) call bpf_probe_read_kernel_str#115   ; R0_w=scalar(smin=-4095,smax=256)
  18: (bf) r1 = r0                      ; R0_w=scalar(id=1,smin=-4095,smax=256) R1_w=scalar(id=1,smin=-4095,smax=256)
  19: (67) r1 <<= 32                    ; R1_w=scalar(smax=1099511627776,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32_max=)
  20: (bf) r2 = r1                      ; R1_w=scalar(id=2,smax=1099511627776,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32)
  21: (c7) r2 s>>= 32                   ; R2=scalar(smin=-2147483648,smax=256)
  ; if (len >= 0) {
  22: (c5) if r2 s< 0x0 goto pc+7       ; R2=scalar(umax=256,var_off=(0x0; 0x1ff))
  ; payload4_len1 = len;
  23: (18) r2 = 0xffffc90000167418      ; R2_w=map_value(off=1048,ks=4,vs=1572,imm=0)
  25: (63) *(u32 *)(r2 +0) = r0         ; R0=scalar(id=1,smin=-4095,smax=256) R2_w=map_value(off=1048,ks=4,vs=1572,imm=0)
  26: (77) r1 >>= 32                    ; R1_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff))
  ; payload += len;
  27: (18) r6 = 0xffffc90000167424      ; R6_w=map_value(off=1060,ks=4,vs=1572,imm=0)
  29: (0f) r6 += r1                     ; R1_w=Pscalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R6_w=map_value(off=1060,ks=4,vs=1572,umax=4294967295,var_off=(0)
  ; len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in2[0]);
  30: (bf) r1 = r6                      ; R1_w=map_value(off=1060,ks=4,vs=1572,umax=4294967295,var_off=(0x0; 0xffffffff)) R6_w=map_value(off=1060,ks=4,vs=1572,um)
  31: (b7) r2 = 256                     ; R2_w=256
  32: (18) r3 = 0xffffc90000164100      ; R3_w=map_value(off=256,ks=4,vs=1056,imm=0)
  34: (85) call bpf_probe_read_kernel_str#115
  R1 unbounded memory access, make sure to bounds check any such access
  processed 27 insns (limit 1000000) max_states_per_insn 0 total_states 2 peak_states 2 mark_read 1
  -- END PROG LOAD LOG --
  libbpf: failed to load program 'handler32_signed'

The failure is due to
  20: (bf) r2 = r1                      ; R1_w=scalar(id=2,smax=1099511627776,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32)
  21: (c7) r2 s>>= 32                   ; R2=scalar(smin=-2147483648,smax=256)
  22: (c5) if r2 s< 0x0 goto pc+7       ; R2=scalar(umax=256,var_off=(0x0; 0x1ff))
  26: (77) r1 >>= 32                    ; R1_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff))
  29: (0f) r6 += r1                     ; R1_w=Pscalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R6_w=map_value(off=1060,ks=4,vs=1572,umax=4294967295,var_off=(0)
where r1 has conservative value range compared to r2 and r1 is used later.

In llvm, commit [1] triggered the above code generation and caused
verification failure.

It may take a while for llvm to address this issue. In the main time,
let us change the variable 'len' type to 'long' and adjust condition properly.
Tested with llvm14 and latest llvm15, both worked fine.

 [1] https://reviews.llvm.org/D126647

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/testing/selftests/bpf/progs/test_varlen.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_varlen.c b/tools/testing/selftests/bpf/progs/test_varlen.c
index 913acdffd90f..3987ff174f1f 100644
--- a/tools/testing/selftests/bpf/progs/test_varlen.c
+++ b/tools/testing/selftests/bpf/progs/test_varlen.c
@@ -41,20 +41,20 @@ int handler64_unsigned(void *regs)
 {
 	int pid = bpf_get_current_pid_tgid() >> 32;
 	void *payload = payload1;
-	u64 len;
+	long len;
 
 	/* ignore irrelevant invocations */
 	if (test_pid != pid || !capture)
 		return 0;
 
 	len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in1[0]);
-	if (len <= MAX_LEN) {
+	if (len >= 0) {
 		payload += len;
 		payload1_len1 = len;
 	}
 
 	len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in2[0]);
-	if (len <= MAX_LEN) {
+	if (len >= 0) {
 		payload += len;
 		payload1_len2 = len;
 	}
@@ -123,7 +123,7 @@ int handler32_signed(void *regs)
 {
 	int pid = bpf_get_current_pid_tgid() >> 32;
 	void *payload = payload4;
-	int len;
+	long len;
 
 	/* ignore irrelevant invocations */
 	if (test_pid != pid || !capture)
-- 
2.30.2

Re: [PATCH bpf-next] selftests/bpf: Fix test_varlen verification failure with latest llvm

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Mon, 13 Jun 2022 16:34:49 -0700 you wrote:
> With latest llvm15, test_varlen failed with the following verifier log:
> 
>   17: (85) call bpf_probe_read_kernel_str#115   ; R0_w=scalar(smin=-4095,smax=256)
>   18: (bf) r1 = r0                      ; R0_w=scalar(id=1,smin=-4095,smax=256) R1_w=scalar(id=1,smin=-4095,smax=256)
>   19: (67) r1 <<= 32                    ; R1_w=scalar(smax=1099511627776,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32_max=)
>   20: (bf) r2 = r1                      ; R1_w=scalar(id=2,smax=1099511627776,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32)
>   21: (c7) r2 s>>= 32                   ; R2=scalar(smin=-2147483648,smax=256)
>   ; if (len >= 0) {
>   22: (c5) if r2 s< 0x0 goto pc+7       ; R2=scalar(umax=256,var_off=(0x0; 0x1ff))
>   ; payload4_len1 = len;
>   23: (18) r2 = 0xffffc90000167418      ; R2_w=map_value(off=1048,ks=4,vs=1572,imm=0)
>   25: (63) *(u32 *)(r2 +0) = r0         ; R0=scalar(id=1,smin=-4095,smax=256) R2_w=map_value(off=1048,ks=4,vs=1572,imm=0)
>   26: (77) r1 >>= 32                    ; R1_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff))
>   ; payload += len;
>   27: (18) r6 = 0xffffc90000167424      ; R6_w=map_value(off=1060,ks=4,vs=1572,imm=0)
>   29: (0f) r6 += r1                     ; R1_w=Pscalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R6_w=map_value(off=1060,ks=4,vs=1572,umax=4294967295,var_off=(0)
>   ; len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in2[0]);
>   30: (bf) r1 = r6                      ; R1_w=map_value(off=1060,ks=4,vs=1572,umax=4294967295,var_off=(0x0; 0xffffffff)) R6_w=map_value(off=1060,ks=4,vs=1572,um)
>   31: (b7) r2 = 256                     ; R2_w=256
>   32: (18) r3 = 0xffffc90000164100      ; R3_w=map_value(off=256,ks=4,vs=1056,imm=0)
>   34: (85) call bpf_probe_read_kernel_str#115
>   R1 unbounded memory access, make sure to bounds check any such access
>   processed 27 insns (limit 1000000) max_states_per_insn 0 total_states 2 peak_states 2 mark_read 1
>   -- END PROG LOAD LOG --
>   libbpf: failed to load program 'handler32_signed'
> 
> [...]

Here is the summary with links:
  - [bpf-next] selftests/bpf: Fix test_varlen verification failure with latest llvm
    https://git.kernel.org/bpf/bpf-next/c/96752e1ec0e0

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next] selftests/bpf: Fix test_varlen verification failure with latest llvm

[Andrii Nakryiko]

On Mon, Jun 13, 2022 at 4:34 PM Yonghong Song <yhs@fb.com> wrote:
>
> With latest llvm15, test_varlen failed with the following verifier log:
>
>   17: (85) call bpf_probe_read_kernel_str#115   ; R0_w=scalar(smin=-4095,smax=256)
>   18: (bf) r1 = r0                      ; R0_w=scalar(id=1,smin=-4095,smax=256) R1_w=scalar(id=1,smin=-4095,smax=256)
>   19: (67) r1 <<= 32                    ; R1_w=scalar(smax=1099511627776,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32_max=)
>   20: (bf) r2 = r1                      ; R1_w=scalar(id=2,smax=1099511627776,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32)
>   21: (c7) r2 s>>= 32                   ; R2=scalar(smin=-2147483648,smax=256)
>   ; if (len >= 0) {
>   22: (c5) if r2 s< 0x0 goto pc+7       ; R2=scalar(umax=256,var_off=(0x0; 0x1ff))
>   ; payload4_len1 = len;
>   23: (18) r2 = 0xffffc90000167418      ; R2_w=map_value(off=1048,ks=4,vs=1572,imm=0)
>   25: (63) *(u32 *)(r2 +0) = r0         ; R0=scalar(id=1,smin=-4095,smax=256) R2_w=map_value(off=1048,ks=4,vs=1572,imm=0)
>   26: (77) r1 >>= 32                    ; R1_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff))
>   ; payload += len;
>   27: (18) r6 = 0xffffc90000167424      ; R6_w=map_value(off=1060,ks=4,vs=1572,imm=0)
>   29: (0f) r6 += r1                     ; R1_w=Pscalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R6_w=map_value(off=1060,ks=4,vs=1572,umax=4294967295,var_off=(0)
>   ; len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in2[0]);
>   30: (bf) r1 = r6                      ; R1_w=map_value(off=1060,ks=4,vs=1572,umax=4294967295,var_off=(0x0; 0xffffffff)) R6_w=map_value(off=1060,ks=4,vs=1572,um)
>   31: (b7) r2 = 256                     ; R2_w=256
>   32: (18) r3 = 0xffffc90000164100      ; R3_w=map_value(off=256,ks=4,vs=1056,imm=0)
>   34: (85) call bpf_probe_read_kernel_str#115
>   R1 unbounded memory access, make sure to bounds check any such access
>   processed 27 insns (limit 1000000) max_states_per_insn 0 total_states 2 peak_states 2 mark_read 1
>   -- END PROG LOAD LOG --
>   libbpf: failed to load program 'handler32_signed'
>
> The failure is due to
>   20: (bf) r2 = r1                      ; R1_w=scalar(id=2,smax=1099511627776,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32)
>   21: (c7) r2 s>>= 32                   ; R2=scalar(smin=-2147483648,smax=256)
>   22: (c5) if r2 s< 0x0 goto pc+7       ; R2=scalar(umax=256,var_off=(0x0; 0x1ff))
>   26: (77) r1 >>= 32                    ; R1_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff))
>   29: (0f) r6 += r1                     ; R1_w=Pscalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R6_w=map_value(off=1060,ks=4,vs=1572,umax=4294967295,var_off=(0)
> where r1 has conservative value range compared to r2 and r1 is used later.
>
> In llvm, commit [1] triggered the above code generation and caused
> verification failure.
>
> It may take a while for llvm to address this issue. In the main time,
> let us change the variable 'len' type to 'long' and adjust condition properly.
> Tested with llvm14 and latest llvm15, both worked fine.
>
>  [1] https://reviews.llvm.org/D126647
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  tools/testing/selftests/bpf/progs/test_varlen.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/test_varlen.c b/tools/testing/selftests/bpf/progs/test_varlen.c
> index 913acdffd90f..3987ff174f1f 100644
> --- a/tools/testing/selftests/bpf/progs/test_varlen.c
> +++ b/tools/testing/selftests/bpf/progs/test_varlen.c
> @@ -41,20 +41,20 @@ int handler64_unsigned(void *regs)
>  {
>         int pid = bpf_get_current_pid_tgid() >> 32;
>         void *payload = payload1;
> -       u64 len;
> +       long len;
>
>         /* ignore irrelevant invocations */
>         if (test_pid != pid || !capture)
>                 return 0;
>
>         len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in1[0]);
> -       if (len <= MAX_LEN) {
> +       if (len >= 0) {
>                 payload += len;
>                 payload1_len1 = len;
>         }
>
>         len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in2[0]);
> -       if (len <= MAX_LEN) {
> +       if (len >= 0) {
>                 payload += len;
>                 payload1_len2 = len;
>         }
> @@ -123,7 +123,7 @@ int handler32_signed(void *regs)
>  {
>         int pid = bpf_get_current_pid_tgid() >> 32;
>         void *payload = payload4;
> -       int len;
> +       long len;


The whole point of handler32_signed and handler64_unsigned was to test
that this code patterns works for signed 32-bit and unsigned 64-bit
variables. By the time clang is fixed we might forget to undo these
changes in test_varlen.c. Maybe it's better to leave code as is? We
already blacklisted this test for CI anyways, so it doesn't affect
kernel-patches CI runs.

>
>         /* ignore irrelevant invocations */
>         if (test_pid != pid || !capture)
> --
> 2.30.2
>