Bcc: Verifier failure on reading ctx

[palmtenor]

One of my trace.py call failed with an USDT tracepoint because of verifier issues.
And the verifier failed on:

bpf: Permission denied
0: (bf) r6 = r1
1: (85) call bpf_get_current_pid_tgid#14
2: (bf) r7 = r0
3: (bf) r8 = r7
4: (77) r8 >>= 32
5: (15) if r8 == 0xdb365 goto pc+46
 R0=inv R6=ctx R7=inv R8=inv32 R10=fp
6: (b7) r1 = 0
7: (7b) *(u64 *)(r10 -8) = r1
8: (7b) *(u64 *)(r10 -16) = r1
9: (7b) *(u64 *)(r10 -24) = r1
10: (7b) *(u64 *)(r10 -32) = r1
11: (7b) *(u64 *)(r10 -40) = r1
12: (7b) *(u64 *)(r10 -48) = r1
13: (7b) *(u64 *)(r10 -56) = r1
14: (7b) *(u64 *)(r10 -64) = r1
15: (7b) *(u64 *)(r10 -72) = r1
16: (7b) *(u64 *)(r10 -80) = r1
17: (7b) *(u64 *)(r10 -88) = r1
18: (7b) *(u64 *)(r10 -96) = r1
19: (85) call bpf_ktime_get_ns#5
20: (7b) *(u64 *)(r10 -112) = r0
21: (63) *(u32 *)(r10 -104) = r8
22: (63) *(u32 *)(r10 -100) = r7
23: (bf) r1 = r10
24: (07) r1 += -96
25: (b7) r2 = 16
26: (85) call bpf_get_current_comm#16
27: (79) r1 = *(u64 *)(r6 +128)
28: (15) if r1 == 0x111be2b goto pc+5
 R0=inv R1=inv R6=ctx R7=inv R8=inv32 R10=fp
29: (15) if r1 == 0x111bd1a goto pc+1
 R0=inv R1=inv R6=ctx R7=inv R8=inv32 R10=fp
30: (55) if r1 != 0x111bbfd goto pc+11
 R0=inv R1=inv,min_value=17939453,max_value=17939453 R6=ctx R7=inv R8=inv32 R10=fp
31: (bf) r1 = r6
32: (07) r1 += 96
33: (05) goto pc+2
36: (79) r3 = *(u64 *)(r1 +0)
R1 invalid mem access 'inv'

Looking with the full program (below), it seems we are just trying to access *(ctx+96), but doing it by

31: (bf) r1 = r6
32: (07) r1 += 96

confuses the verifier. I think this could happen to any BPF program that uses ctx value. Do we have a workaround or fix toward the issue?

The full BPF program:

#include <linux/ptrace.h>
#include <linux/sched.h>        /* For TASK_COMM_LEN */


struct probe_strobelight_data_destruct_1_data_t
{
        u64 timestamp_ns;
        u32 tgid;
        u32 pid;
        char comm[TASK_COMM_LEN];
        char v0[80];



};

BPF_PERF_OUTPUT(probe_strobelight_data_destruct_1_events);


int probe_strobelight_data_destruct_1(struct pt_regs *ctx)
{
        u64 __pid_tgid = bpf_get_current_pid_tgid();
        u32 __tgid = __pid_tgid >> 32;
        u32 __pid = __pid_tgid; // implicit cast to u32 for bottom half

        if (__tgid == 897893) { return 0; }



        if (!(1)) return 0;

        struct probe_strobelight_data_destruct_1_data_t __data = {0};
        __data.timestamp_ns = bpf_ktime_get_ns();
        __data.tgid = __tgid;
        __data.pid = __pid;
        bpf_get_current_comm(&__data.comm, sizeof(__data.comm));
        int64_t arg1 = 0;
        bpf_usdt_readarg(1, ctx, &arg1);

        if (arg1 != 0) {
                bpf_probe_read(&__data.v0, sizeof(__data.v0), (void *)arg1);
        }


        probe_strobelight_data_destruct_1_events.perf_submit(ctx, &__data, sizeof(__data));
        return 0;
}

#include <uapi/linux/ptrace.h>
static inline int _bpf_readarg_probe_strobelight_data_destruct_1_1(struct pt_regs *ctx, void *dest, size_t len) {
  if (len != sizeof(int64_t)) return -1;
  switch(ctx->ip) {
  case 0x111bbfdULL: *((int64_t *)dest) = (int64_t)ctx->dx; return 0;
  case 0x111bd1aULL: *((int64_t *)dest) = (int64_t)ctx->dx; return 0;
  case 0x111be2bULL: *((int64_t *)dest) = (int64_t)ctx->cx; return 0;
  }
  return -1;
}
static inline int _bpf_readarg_probe_strobelight_data_destruct_1_2(struct pt_regs *ctx, void *dest, size_t len) {
  if (len != sizeof(int64_t)) return -1;
  switch(ctx->ip) {
  case 0x111bbfdULL: *((int64_t *)dest) = (int64_t)ctx->cx; return 0;
  case 0x111bd1aULL: *((int64_t *)dest) = (int64_t)ctx->cx; return 0;
  case 0x111be2bULL: *((int64_t *)dest) = (int64_t)ctx->dx; return 0;
  }
  return -1;
}
static inline int _bpf_readarg_probe_strobelight_data_destruct_1_3(struct pt_regs *ctx, void *dest, size_t len) {
  if (len != sizeof(int64_t)) return -1;
  switch(ctx->ip) {
  case 0x111bbfdULL: { u64 __addr = ctx->bx + (56);int64_t __res = 0x0; bpf_probe_read(&__res, sizeof(__res), (void *)__addr); *((int64_t *)dest) = __res; } return 0;
  case 0x111bd1aULL: { u64 __addr = ctx->bx + (56);int64_t __res = 0x0; bpf_probe_read(&__res, sizeof(__res), (void *)__addr); *((int64_t *)dest) = __res; } return 0;
  case 0x111be2bULL: { u64 __addr = ctx->bx + (48);int64_t __res = 0x0; bpf_probe_read(&__res, sizeof(__res), (void *)__addr); *((int64_t *)dest) = __res; } return 0;
  }
  return -1;
}

[goldshtn]

Seems like the same issue as here: #751. I think it's time to think of what we can do on the LLVM side to avoid this bad code gen.
@4ast @drzaeus77

[yonghong-song]

Some root cause analysis:

static inline int _bpf_readarg_probe_strobelight_data_destruct_1_1(struct pt_regs *ctx, void *dest, size_t len) {
  if (len != sizeof(int64_t)) return -1;
  switch(ctx->ip) {
  case 0x111bbfdULL: *((int64_t *)dest) = (int64_t)ctx->dx; return 0;
  case 0x111bd1aULL: *((int64_t *)dest) = (int64_t)ctx->dx; return 0;
  case 0x111be2bULL: *((int64_t *)dest) = (int64_t)ctx->cx; return 0;
  }
  return -1;
}

after inlining, it is something like below

    a = (switch(ctx->ip) { case #1: ctx->dx; case #2: ctx->dx; case #3: ctx->cx })

the LLVM does some transformation likes below:

    p = (switch(ctx->ip) { case #1: &(ctx->dx); case #2: &(ctx->dx); case #3: &(ctx->cx); })
    a = *p;

This is a reasonable compiler optimization. it reduces potential code size.

I found a workaround for this:

static __always_inline int _bpf_readarg_probe_strobelight_data_destruct_1_1(struct pt_regs *ctx, void *dest, size_t len) {
  if (len != sizeof(int64_t)) return -1;
  switch(ctx->ip) {
  case 0x111bbfdULL: *((volatile int64_t *)dest) = (int64_t)ctx->dx; return 0;
  case 0x111bd1aULL: *((volatile int64_t *)dest) = (int64_t)ctx->dx; return 0;
  case 0x111be2bULL: *((volatile int64_t *)dest) = (int64_t)ctx->cx; return 0;
  }
  return -1;
}

Basically, a store to memory by dest will force read ctx->dx and ctx->cx,
hence prevent compiler optimization. I also changed inline to "__always_inline"
to guarantee inlining.

The diff for a bcc change:

[yhs@localhost bcc]$ git diff src/cc/usdt.cc
diff --git a/src/cc/usdt.cc b/src/cc/usdt.cc
index 7d1dd57..ef929d7 100644
--- a/src/cc/usdt.cc
+++ b/src/cc/usdt.cc
@@ -152,10 +152,10 @@ bool Probe::usdt_getarg(std::ostream &stream) {

   for (size_t arg_n = 0; arg_n < arg_count; ++arg_n) {
     std::string ctype = largest_arg_type(arg_n);
-    std::string cptr = tfm::format("*((%s *)dest)", ctype);
+    std::string cptr = tfm::format("*((volatile %s *)dest)", ctype);

     tfm::format(stream,
-                "static inline int _bpf_readarg_%s_%d("
+                "static __always_inline int _bpf_readarg_%s_%d("
                 "struct pt_regs *ctx, void *dest, size_t len) {\n"
                 "  if (len != sizeof(%s)) return -1;\n",
                 attached_to_.value(), arg_n + 1, ctype);
[yhs@localhost bcc]$ 

The proper way to address this issue is probably in verifier. In verifier, we have
frame pointer and stack pointer (frame pointer + offset), we also track packet
pointer with an offset. If also keep track of context + offset properly, we should
be fine. I am starting to working on verifier fix now.