| Message ID | 20191107170045.2503480-1-yhs@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next,v2,tools/bpf] workaround a verifier failure for test_progs | expand |
> 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>
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!
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;
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