Date:   Tue, 16 Jun 2020 14:27:36 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Kernel Team <kernel-team@...com>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH bpf 2/2] selftests/bpf: add variable-length data
 concatenation pattern test

On Tue, Jun 16, 2020 at 1:21 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 6/16/20 7:04 AM, Andrii Nakryiko wrote:
> > Add selftest that validates variable-length data reading and concatentation
> > with one big shared data array. This is a common pattern in production use for
> > monitoring and tracing applications, that potentially can read a lot of data,
> > but usually reads much less. Such pattern allows to determine precisely what
> > amount of data needs to be sent over perfbuf/ringbuf and maximize efficiency.
> >
> > This is the first BPF selftest that at all looks at and tests
> > bpf_probe_read_str()-like helper's return value, closing a major gap in BPF
> > testing. It surfaced the problem with bpf_probe_read_kernel_str() returning
> > 0 on success, instead of amount of bytes successfully read.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@...com>
>
> Fix looks good, but I'm seeing an issue in the selftest on my side. With latest
> Clang/LLVM I'm getting:
>
> # ./test_progs -t varlen
> #86 varlen:OK
> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
>
> All good, however, the test_progs-no_alu32 fails for me with:

Yeah, same here. It's due to Clang emitting unnecessary bit shifts
because bpf_probe_read_kernel_str() is defined as returning 32-bit
int. I have a patch ready locally, just waiting for bpf-next to open,
which switches those helpers to return long, which auto-matically
fixes this test.

If it's not a problem, I'd just wait for that patch to go into
bpf-next. If not, I can sprinkle bits of assembly magic around to
force the kernel to do those bitshifts earlier. But I figured having
test_progs-no_alu32 failing one selftest temporarily wasn't too bad.

>
> # ./test_progs-no_alu32 -t varlen
> Switching to flavor 'no_alu32' subdirectory...
> libbpf: load bpf program failed: Invalid argument
> libbpf: -- BEGIN DUMP LOG ---
> libbpf:
> arg#0 type is not a struct
> Unrecognized arg#0 type PTR
> ; int pid = bpf_get_current_pid_tgid() >> 32;
> 0: (85) call bpf_get_current_pid_tgid#14
> ; int pid = bpf_get_current_pid_tgid() >> 32;
> 1: (77) r0 >>= 32
> ; if (test_pid != pid || !capture)
> 2: (18) r1 = 0xffffb14a4010c200
> 4: (61) r1 = *(u32 *)(r1 +0)
>   R0_w=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R1_w=map_value(id=0,off=512,ks=4,vs=1056,imm=0) R10=fp0
> ; if (test_pid != pid || !capture)
> 5: (5d) if r1 != r0 goto pc+43
>   R0_w=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R1_w=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R10=fp0
> 6: (18) r1 = 0xffffb14a4010c204
> 8: (71) r1 = *(u8 *)(r1 +0)
>   R0_w=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R1_w=map_value(id=0,off=516,ks=4,vs=1056,imm=0) R10=fp0
> 9: (15) if r1 == 0x0 goto pc+39
>   R0=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R1=inv(id=0,umax_value=255,var_off=(0x0; 0xff)) R10=fp0
> ; len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in1[0]);
> 10: (18) r6 = 0xffffb14a4010c220
> 12: (18) r1 = 0xffffb14a4010c220
> 14: (b7) r2 = 256
> 15: (18) r3 = 0xffffb14a4010c000
> 17: (85) call bpf_probe_read_kernel_str#115
>   R0=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R1_w=map_value(id=0,off=544,ks=4,vs=1056,imm=0) R2_w=inv256 R3_w=map_value(id=0,off=0,ks=4,vs=1056,imm=0) R6_w=map_value(id=0,off=544,ks=4,vs=1056,imm=0) R10=fp0
> last_idx 17 first_idx 9
> regs=4 stack=0 before 15: (18) r3 = 0xffffb14a4010c000
> regs=4 stack=0 before 14: (b7) r2 = 256
> 18: (67) r0 <<= 32
> 19: (bf) r1 = r0
> 20: (77) r1 >>= 32
> ; if (len <= MAX_LEN) {
> 21: (25) if r1 > 0x100 goto pc+7
>   R0=inv(id=0,smax_value=1099511627776,umax_value=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min_value=0,s32_max_value=0,u32_max_value=0) R1=inv(id=0,umax_value=256,var_off=(0x0; 0x1ff)) R6=map_value(id=0,off=544,ks=4,vs=1056,imm=0) R10=fp0
> ;
> 22: (c7) r0 s>>= 32
> ; payload1_len1 = len;
> 23: (18) r1 = 0xffffb14a4010c208
> 25: (7b) *(u64 *)(r1 +0) = r0
>   R0_w=inv(id=0,smin_value=-2147483648,smax_value=256) R1_w=map_value(id=0,off=520,ks=4,vs=1056,imm=0) R6=map_value(id=0,off=544,ks=4,vs=1056,imm=0) R10=fp0
> ; payload += len;
> 26: (18) r6 = 0xffffb14a4010c220
> 28: (0f) r6 += r0
> last_idx 28 first_idx 21
> regs=1 stack=0 before 26: (18) r6 = 0xffffb14a4010c220
> regs=1 stack=0 before 25: (7b) *(u64 *)(r1 +0) = r0
> regs=1 stack=0 before 23: (18) r1 = 0xffffb14a4010c208
> regs=1 stack=0 before 22: (c7) r0 s>>= 32
> regs=1 stack=0 before 21: (25) if r1 > 0x100 goto pc+7
>   R0_rw=invP(id=0,smax_value=1099511627776,umax_value=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min_value=0,s32_max_value=0,u32_max_value=0) R1_rw=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6_w=map_value(id=0,off=544,ks=4,vs=1056,imm=0) R10=fp0
> parent didn't have regs=1 stack=0 marks
> last_idx 20 first_idx 9
> regs=1 stack=0 before 20: (77) r1 >>= 32
> regs=1 stack=0 before 19: (bf) r1 = r0
> regs=1 stack=0 before 18: (67) r0 <<= 32
> regs=1 stack=0 before 17: (85) call bpf_probe_read_kernel_str#115
> value -2147483648 makes map_value pointer be out of bounds
> processed 22 insns (limit 1000000) max_states_per_insn 0 total_states 2 peak_states 2 mark_read 1
>
> libbpf: -- END LOG --
> libbpf: failed to load program 'raw_tp/sys_enter'
> libbpf: failed to load object 'test_varlen'
> libbpf: failed to load BPF skeleton 'test_varlen': -4007
> test_varlen:FAIL:skel_open failed to open skeleton
> #86 varlen:FAIL
> Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED

Powered by blists - more mailing lists