On 11/14/2017 02:42 PM, Arnaldo Carvalho de Melo wrote:

Em Tue, Nov 14, 2017 at 02:09:34PM +0100, Daniel Borkmann escreveu:

quoted

On 11/14/2017 01:58 PM, Arnaldo Carvalho de Melo wrote:

quoted

Em Tue, Nov 14, 2017 at 01:09:39AM +0100, Daniel Borkmann escreveu:

quoted

On 11/13/2017 04:08 PM, Arnaldo Carvalho de Melo wrote:

quoted

libbpf: -- BEGIN DUMP LOG ---
libbpf: 
0: (79) r3 = *(u64 *)(r1 +104)
1: (b7) r2 = 0
2: (bf) r6 = r1
3: (bf) r1 = r10
4: (07) r1 += -128
5: (b7) r2 = 128
6: (85) call bpf_probe_read_str#45
7: (bf) r1 = r0
8: (07) r1 += -1
9: (67) r1 <<= 32
10: (77) r1 >>= 32
11: (25) if r1 > 0x7f goto pc+11

Right, so the compiler is optimizing the two tests into a single one above,
which means lower bound cannot properly be derived again by the verifier due
to this and thus you'll get the error. Similar issue was seen recently [1].

Does the below hack work for you?

int prog([...])
{
        char filename[128];
        int ret = bpf_probe_read_str(filename, sizeof(filename), filename_ptr);
        if (ret > 0)
                bpf_perf_event_output(ctx, &__bpf_stdout__, BPF_F_CURRENT_CPU, filename,
                                      ret & (sizeof(filename) - 1));
        return 1;
}

r0 should keep on tracking bounds here at least:

prog:
       0:	bf 16 00 00 00 00 00 00 	r6 = r1
       1:	bf a1 00 00 00 00 00 00 	r1 = r10
       2:	07 01 00 00 80 ff ff ff 	r1 += -128
       3:	b7 02 00 00 80 00 00 00 	r2 = 128
       4:	85 00 00 00 2d 00 00 00 	call 45
       5:	67 00 00 00 20 00 00 00 	r0 <<= 32
       6:	c7 00 00 00 20 00 00 00 	r0 s>>= 32
       7:	b7 01 00 00 01 00 00 00 	r1 = 1
       8:	6d 01 0a 00 00 00 00 00 	if r1 s> r0 goto 10
       9:	57 00 00 00 7f 00 00 00 	r0 &= 127
      10:	bf a4 00 00 00 00 00 00 	r4 = r10
      11:	07 04 00 00 80 ff ff ff 	r4 += -128
      12:	bf 61 00 00 00 00 00 00 	r1 = r6
      13:	18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 	r2 = 0ll
      15:	18 03 00 00 ff ff ff ff 00 00 00 00 00 00 00 00 	r3 = 4294967295ll
      17:	bf 05 00 00 00 00 00 00 	r5 = r0
      18:	85 00 00 00 19 00 00 00 	call 25

  [1] http://patchwork.ozlabs.org/project/netdev/list/?series=13211

Not yet:

6: (85) call bpf_probe_read_str#45
7: (bf) r1 = r0
8: (67) r1 <<= 32
9: (77) r1 >>= 32
10: (15) if r1 == 0x0 goto pc+10
 R0=inv(id=0) R1=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6=ctx(id=0,off=0,imm=0) R10=fp0
11: (57) r0 &= 127
12: (bf) r4 = r10
13: (07) r4 += -128
14: (bf) r1 = r6
15: (18) r2 = 0xffff92bfc2aba840u
17: (18) r3 = 0xffffffff
19: (bf) r5 = r0
20: (85) call bpf_perf_event_output#25
invalid stack type R4 off=-128 access_size=0

I'll try updating clang/llvm...

Full details:

[root@jouet bpf]# cat open.c 
#include "bpf.h"

SEC("prog=do_sys_open filename")
int prog(void *ctx, int err, const char __user *filename_ptr)
{
	char filename[128];
	const unsigned len = bpf_probe_read_str(filename, sizeof(filename), filename_ptr);

Btw, I was using 'int' here above instead of 'unsigned' as strncpy_from_unsafe()
could potentially return errors like -EFAULT.

I changed to int, didn't help
 

quoted

Currently having a version compiled from the git tree:

# llc --version
LLVM (http://llvm.org/):
  LLVM version 6.0.0git-2d810c2
  Optimized build.
  Default target: x86_64-unknown-linux-gnu
  Host CPU: skylake

[root@jouet bpf]# llc --version
LLVM (http://llvm.org/):
  LLVM version 4.0.0svn

Old stuff! ;-) Will change, but improving these messages should be on
the radar, I think :-)

Yep, agree, I think we need a generic, better solution for this type of
issue instead of converting individual helpers to handle 0 min bound and
then only bailing out in such case; need to brainstorm a bit on that.

I think for the above in your case ...

 [...]
  6: (85) call bpf_probe_read_str#45
  7: (bf) r1 = r0
  8: (67) r1 <<= 32
  9: (77) r1 >>= 32
 10: (15) if r1 == 0x0 goto pc+10
  R0=inv(id=0) R1=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6=ctx(id=0,off=0,imm=0) R10=fp0
 11: (57) r0 &= 127
 [...]

... the shifts on r1 might be due to using 32 bit type, so if you find
a way to avoid these and have the test on r0 directly, we might get there.
Perhaps keep using a 64 bit type to avoid them. It would be useful to
propagate the deduced bound information back to r0 when we know that
neither r0 nor r1 has changed in the meantime.

- Arnaldo
 

quoted

  Registered Targets:
    bpf    - BPF (host endian)
    bpfeb  - BPF (big endian)
    bpfel  - BPF (little endian)
    x86    - 32-bit X86: Pentium-Pro and above
    x86-64 - 64-bit X86: EM64T and AMD64

quoted

	if (len > 0)
       		perf_event_output(ctx, &__bpf_stdout__, BPF_F_CURRENT_CPU, filename,
				  len & (sizeof(filename) - 1));
	return 1;
}