bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v2] [tools/bpf] workaround a verifier failure for test_progs
@ 2019-11-07 17:00 Yonghong Song
  2019-11-07 23:10 ` Song Liu
  2019-11-11 13:07 ` Daniel Borkmann
  0 siblings, 2 replies; 6+ messages in thread
From: Yonghong Song @ 2019-11-07 17:00 UTC (permalink / raw)
  To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team

With latest llvm compiler, running test_progs will have
the following verifier failure for test_sysctl_loop1.o:
  libbpf: load bpf program failed: Permission denied
  libbpf: -- BEGIN DUMP LOG ---
  libbpf:
  invalid indirect read from stack var_off (0x0; 0xff)+196 size 7
  ...
  libbpf: -- END LOG --
  libbpf: failed to load program 'cgroup/sysctl'
  libbpf: failed to load object 'test_sysctl_loop1.o'

The related bytecodes look below:
  0000000000000308 LBB0_8:
      97:       r4 = r10
      98:       r4 += -288
      99:       r4 += r7
     100:       w8 &= 255
     101:       r1 = r10
     102:       r1 += -488
     103:       r1 += r8
     104:       r2 = 7
     105:       r3 = 0
     106:       call 106
     107:       w1 = w0
     108:       w1 += -1
     109:       if w1 > 6 goto -24 <LBB0_5>
     110:       w0 += w8
     111:       r7 += 8
     112:       w8 = w0
     113:       if r7 != 224 goto -17 <LBB0_8>
and source code:
     for (i = 0; i < ARRAY_SIZE(tcp_mem); ++i) {
             ret = bpf_strtoul(value + off, MAX_ULONG_STR_LEN, 0,
                               tcp_mem + i);
             if (ret <= 0 || ret > MAX_ULONG_STR_LEN)
                     return 0;
             off += ret & MAX_ULONG_STR_LEN;
     }
Current verifier is not able to conclude register w0 before '+'
at insn 110 has a range of 1 to 7 and thinks it is from 0 - 255.
This leads to more conservative range for w8 at insn 112,
and later verifier complaint.

Let us workaound this issue until we found a compiler and/or
verifier solution. The workaround in this patch is
to make variable 'ret' volatile, which will force a reload
and then '&' operation to ensure better value range.
With this patch, I got the below byte code for the loop,
  0000000000000328 LBB0_9:
     101:       r4 = r10
     102:       r4 += -288
     103:       r4 += r7
     104:       w8 &= 255
     105:       r1 = r10
     106:       r1 += -488
     107:       r1 += r8
     108:       r2 = 7
     109:       r3 = 0
     110:       call 106
     111:       *(u32 *)(r10 - 64) = r0
     112:       r1 = *(u32 *)(r10 - 64)
     113:       if w1 s< 1 goto -28 <LBB0_5>
     114:       r1 = *(u32 *)(r10 - 64)
     115:       if w1 s> 7 goto -30 <LBB0_5>
     116:       r1 = *(u32 *)(r10 - 64)
     117:       w1 &= 7
     118:       w1 += w8
     119:       r7 += 8
     120:       w8 = w1
     121:       if r7 != 224 goto -21 <LBB0_9>
Insn 117 did the '&' operation and we got more precise
value range for 'w8' at insn 120.
The test is happy then.
  #3/17 test_sysctl_loop1.o:OK

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

Changelog:
  v1 -> v2:
    - Added byte codes after the code change in the commit message

diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c b/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c
index 608a06871572..d22e438198cf 100644
--- a/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c
+++ b/tools/testing/selftests/bpf/progs/test_sysctl_loop1.c
@@ -44,7 +44,10 @@ int sysctl_tcp_mem(struct bpf_sysctl *ctx)
 	unsigned long tcp_mem[TCP_MEM_LOOPS] = {};
 	char value[MAX_VALUE_STR_LEN];
 	unsigned char i, off = 0;
-	int ret;
+	/* a workaround to prevent compiler from generating
+	 * codes verifier cannot handle yet.
+	 */
+	volatile int ret;
 
 	if (ctx->write)
 		return 0;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next v2] [tools/bpf] workaround a verifier failure for test_progs
  2019-11-07 17:00 [PATCH bpf-next v2] [tools/bpf] workaround a verifier failure for test_progs Yonghong Song
@ 2019-11-07 23:10 ` Song Liu
  2019-11-11 13:07 ` Daniel Borkmann
  1 sibling, 0 replies; 6+ messages in thread
From: Song Liu @ 2019-11-07 23:10 UTC (permalink / raw)
  To: Yonghong Song; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team



> On Nov 7, 2019, at 9:00 AM, Yonghong Song <yhs@fb.com> wrote:
> 

[...]

> Let us workaound this issue until we found a compiler and/or
> verifier solution. The workaround in this patch is
> to make variable 'ret' volatile, which will force a reload
> and then '&' operation to ensure better value range.
> With this patch, I got the below byte code for the loop,
>  0000000000000328 LBB0_9:
>     101:       r4 = r10
>     102:       r4 += -288
>     103:       r4 += r7
>     104:       w8 &= 255
>     105:       r1 = r10
>     106:       r1 += -488
>     107:       r1 += r8
>     108:       r2 = 7
>     109:       r3 = 0
>     110:       call 106
>     111:       *(u32 *)(r10 - 64) = r0
>     112:       r1 = *(u32 *)(r10 - 64)
>     113:       if w1 s< 1 goto -28 <LBB0_5>
>     114:       r1 = *(u32 *)(r10 - 64)
>     115:       if w1 s> 7 goto -30 <LBB0_5>
>     116:       r1 = *(u32 *)(r10 - 64)
>     117:       w1 &= 7
>     118:       w1 += w8
>     119:       r7 += 8
>     120:       w8 = w1
>     121:       if r7 != 224 goto -21 <LBB0_9>
> Insn 117 did the '&' operation and we got more precise
> value range for 'w8' at insn 120.

Thanks for adding this information. 

> The test is happy then.
>  #3/17 test_sysctl_loop1.o:OK
> 
> Signed-off-by: Yonghong Song <yhs@fb.com>

Acked-by: Song Liu <songliubraving@fb.com>




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next v2] [tools/bpf] workaround a verifier failure for test_progs
  2019-11-07 17:00 [PATCH bpf-next v2] [tools/bpf] workaround a verifier failure for test_progs Yonghong Song
  2019-11-07 23:10 ` Song Liu
@ 2019-11-11 13:07 ` Daniel Borkmann
  1 sibling, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2019-11-11 13:07 UTC (permalink / raw)
  To: Yonghong Song, bpf; +Cc: Alexei Starovoitov, kernel-team

On 11/7/19 6:00 PM, Yonghong Song wrote:
> With latest llvm compiler, running test_progs will have
> the following verifier failure for test_sysctl_loop1.o:
>    libbpf: load bpf program failed: Permission denied
>    libbpf: -- BEGIN DUMP LOG ---
>    libbpf:
>    invalid indirect read from stack var_off (0x0; 0xff)+196 size 7
>    ...
>    libbpf: -- END LOG --
>    libbpf: failed to load program 'cgroup/sysctl'
>    libbpf: failed to load object 'test_sysctl_loop1.o'
> 
> The related bytecodes look below:
>    0000000000000308 LBB0_8:
>        97:       r4 = r10
>        98:       r4 += -288
>        99:       r4 += r7
>       100:       w8 &= 255
>       101:       r1 = r10
>       102:       r1 += -488
>       103:       r1 += r8
>       104:       r2 = 7
>       105:       r3 = 0
>       106:       call 106
>       107:       w1 = w0
>       108:       w1 += -1
>       109:       if w1 > 6 goto -24 <LBB0_5>
>       110:       w0 += w8
>       111:       r7 += 8
>       112:       w8 = w0
[...]
Applied, thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next v2] [tools/bpf] workaround a verifier failure for test_progs
  2020-04-28  6:13 ` Yonghong Song
@ 2020-04-28 22:33   ` Alexei Starovoitov
  0 siblings, 0 replies; 6+ messages in thread
From: Alexei Starovoitov @ 2020-04-28 22:33 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Ma Xinjian, Alexei Starovoitov, bpf, Daniel Borkmann,
	Kernel Team, Li, Philip

On Mon, Apr 27, 2020 at 11:14 PM Yonghong Song <yhs@fb.com> wrote:
> > -       int ret;
> > +      /* a workaround to prevent compiler from generating
> > +      * codes verifier cannot handle yet.
> > +      */
> > +      volatile int ret;
> >
> >          if (ctx->write)
> >                  return 0;
>
> Right. This is related to alu32 mode. The detailed description
> https://lore.kernel.org/bpf/20191107170045.2503480-1-yhs@fb.com/
>
> We are still working on this, either a verifier solution or a compiler
> workaround.

Thanks for the report.
I pushed the same fix for progs/test_sysctl_prog.c into bpf-next.
Now test_sysctl passes.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next v2] [tools/bpf] workaround a verifier failure for test_progs
  2020-04-28  2:56 Ma Xinjian
@ 2020-04-28  6:13 ` Yonghong Song
  2020-04-28 22:33   ` Alexei Starovoitov
  0 siblings, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2020-04-28  6:13 UTC (permalink / raw)
  To: Ma Xinjian, ast, bpf, daniel, kernel-team; +Cc: Li, Philip



On 4/27/20 7:56 PM, Ma Xinjian wrote:
> Hi Yonghong.
> 
> During our team's test, we find a similar issue. when run test_sysctl of 
> bpf, we met error:
> 
> ```
> 
> libbpf: -- END LOG --
> libbpf: failed to load program 'cgroup/sysctl'
> libbpf: failed to load object './test_sysctl_prog.o'
> (test_sysctl.c:1490: errno: Permission denied) >>> Loading program 
> (./test_sysctl_prog.o) error.
> 
> ```
> 
> Testing env: "Debian GNU/Linux 9 (stretch)"
> 
> kernel: 5.6 5.7-rc1 5.7-rc2 5.7-rc3
> 
> clang/llvm version: v11.0.0.
> 
> we have tested a log of commits, following commits are part of them
> 
> drwxrwxr-x 2 root root  1 Apr 28 07:01 
> f30416fdde922eaa655934e050026930fefbd260
> drwxrwxr-x 2 root root  2 Apr 26 11:38 
> 10bc12588dac532fad044b2851dde8e7b9121e88
> drwxrwxr-x 2 root root  1 Apr 26 07:01 
> 969e7edd88ceb4791eb1cac22828290f0ae30b3d
> drwxrwxr-x 2 root root  1 Apr 23 13:51 
> bbf386f02b05db017fda66875cc5edef70779244
> drwxrwxr-x 2 root root  1 Apr 22 10:01 
> 2de52422acf04662b45599f77c14ce1b2cec2b81
> drwxrwxr-x 2 root root  1 Apr 21 07:07 
> a9b137f9ffba8cb25dfd7dd1fb613e8aac121b37
> drwxrwxr-x 2 root root  1 Apr 17 07:01 
> 40d139c620f83509fe18acbff5ec358298e99def
> 
> drwxrwxr-x 2 root root  1 Apr 16 07:02 
> bee6c234ed28ae7349cb83afa322dfd8394590ee
> 
> 
> And I have tested, If I add following patch like you did, test_sysctl pass:
> 
> diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_prog.c 
> b/tools/testing/selftests/bpf/progs/test_sysctl_prog.c
> index 2d0b0b82a78a..8e3da8d2e7c9 100644
> --- a/tools/testing/selftests/bpf/progs/test_sysctl_prog.c
> +++ b/tools/testing/selftests/bpf/progs/test_sysctl_prog.c
> @@ -45,7 +45,10 @@ int sysctl_tcp_mem(struct bpf_sysctl *ctx)
>          unsigned long tcp_mem[3] = {0, 0, 0};
>          char value[MAX_VALUE_STR_LEN];
>          unsigned char i, off = 0;
> -       int ret;
> +      /* a workaround to prevent compiler from generating
> +      * codes verifier cannot handle yet.
> +      */
> +      volatile int ret;
> 
>          if (ctx->write)
>                  return 0;

Right. This is related to alu32 mode. The detailed description
https://lore.kernel.org/bpf/20191107170045.2503480-1-yhs@fb.com/

We are still working on this, either a verifier solution or a compiler 
workaround.

> 
> root@vm-snb-15 
> /usr/src/perf_selftests-x86_64-rhel-7.6-kselftests-6a8b55ed4056ea5559ebe4f6a4b247f627870d4c/tools/testing/selftests/bpf# 
> ./test_sysctl
> 
> ...
> 
>   Summary: 40 PASSED, 0 FAILED
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next v2] [tools/bpf] workaround a verifier failure for test_progs
@ 2020-04-28  2:56 Ma Xinjian
  2020-04-28  6:13 ` Yonghong Song
  0 siblings, 1 reply; 6+ messages in thread
From: Ma Xinjian @ 2020-04-28  2:56 UTC (permalink / raw)
  To: yhs, ast, bpf, daniel, kernel-team; +Cc: Li, Philip

Hi Yonghong.

During our team's test, we find a similar issue. when run test_sysctl of 
bpf, we met error:

```

libbpf: -- END LOG --
libbpf: failed to load program 'cgroup/sysctl'
libbpf: failed to load object './test_sysctl_prog.o'
(test_sysctl.c:1490: errno: Permission denied) >>> Loading program 
(./test_sysctl_prog.o) error.

```

Testing env: "Debian GNU/Linux 9 (stretch)"

kernel: 5.6 5.7-rc1 5.7-rc2 5.7-rc3

clang/llvm version: v11.0.0.

we have tested a log of commits, following commits are part of them

drwxrwxr-x 2 root root  1 Apr 28 07:01 
f30416fdde922eaa655934e050026930fefbd260
drwxrwxr-x 2 root root  2 Apr 26 11:38 
10bc12588dac532fad044b2851dde8e7b9121e88
drwxrwxr-x 2 root root  1 Apr 26 07:01 
969e7edd88ceb4791eb1cac22828290f0ae30b3d
drwxrwxr-x 2 root root  1 Apr 23 13:51 
bbf386f02b05db017fda66875cc5edef70779244
drwxrwxr-x 2 root root  1 Apr 22 10:01 
2de52422acf04662b45599f77c14ce1b2cec2b81
drwxrwxr-x 2 root root  1 Apr 21 07:07 
a9b137f9ffba8cb25dfd7dd1fb613e8aac121b37
drwxrwxr-x 2 root root  1 Apr 17 07:01 
40d139c620f83509fe18acbff5ec358298e99def

drwxrwxr-x 2 root root  1 Apr 16 07:02 
bee6c234ed28ae7349cb83afa322dfd8394590ee


And I have tested, If I add following patch like you did, test_sysctl pass:

diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_prog.c 
b/tools/testing/selftests/bpf/progs/test_sysctl_prog.c
index 2d0b0b82a78a..8e3da8d2e7c9 100644
--- a/tools/testing/selftests/bpf/progs/test_sysctl_prog.c
+++ b/tools/testing/selftests/bpf/progs/test_sysctl_prog.c
@@ -45,7 +45,10 @@ int sysctl_tcp_mem(struct bpf_sysctl *ctx)
         unsigned long tcp_mem[3] = {0, 0, 0};
         char value[MAX_VALUE_STR_LEN];
         unsigned char i, off = 0;
-       int ret;
+      /* a workaround to prevent compiler from generating
+      * codes verifier cannot handle yet.
+      */
+      volatile int ret;

         if (ctx->write)
                 return 0;

root@vm-snb-15 
/usr/src/perf_selftests-x86_64-rhel-7.6-kselftests-6a8b55ed4056ea5559ebe4f6a4b247f627870d4c/tools/testing/selftests/bpf# 
./test_sysctl

...

  Summary: 40 PASSED, 0 FAILED

-- 
Best Regards.
Ma Xinjian


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-04-28 22:34 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-07 17:00 [PATCH bpf-next v2] [tools/bpf] workaround a verifier failure for test_progs Yonghong Song
2019-11-07 23:10 ` Song Liu
2019-11-11 13:07 ` Daniel Borkmann
2020-04-28  2:56 Ma Xinjian
2020-04-28  6:13 ` Yonghong Song
2020-04-28 22:33   ` Alexei Starovoitov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).