invalid indirect read from stack off -16+4 size 16 (when tracing spinlocks)

[pavlix]

Hello! I jumped right into the hard job, tracing spinlocks. I have issues copying a pointer value from the argument of _raw_spin_lock to a BPF_PERF_OUTPUT.

#include <uapi/linux/ptrace.h>
#include <linux/irq.h>
#include <linux/spinlock.h>

struct event {
    int direction;
    u64 lock;
};

BPF_PERF_OUTPUT(events);

int kprobe___raw_spin_lock(struct pt_regs *ctx, u64 lock)
{
    struct event event = {
        .direction = 0,
        .lock = lock,
    };
    events.perf_submit(ctx, &event, sizeof event);
    bpf_trace_printk("lock %ld\n", lock);
    return 0;
}

This cannot be loaded to the kernel.

bpf: Failed to load program: Permission denied
; u64 lock = ctx->di;
0: (79) r6 = *(u64 *)(r1 +112)
; struct event event = {
1: (7b) *(u64 *)(r10 -8) = r6
2: (b7) r2 = 0
3: (63) *(u32 *)(r10 -16) = r2
last_idx 3 first_idx 0
regs=4 stack=0 before 2: (b7) r2 = 0
; bpf_perf_event_output(ctx, bpf_pseudo_fd(1, -1), CUR_CPU_IDENTIFIER, &event, sizeof event);
4: (18) r2 = 0xffff904e72e57600
6: (bf) r4 = r10
; 
7: (07) r4 += -16
; bpf_perf_event_output(ctx, bpf_pseudo_fd(1, -1), CUR_CPU_IDENTIFIER, &event, sizeof event);
8: (18) r3 = 0xffffffff
10: (b7) r5 = 16
11: (85) call bpf_perf_event_output#25
invalid indirect read from stack off -16+4 size 16
processed 10 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0

Surprisingly, a trivial change from static initialization from the arguments to an individual assignment worked for me. Is there a reason for that or is this a bug?

The following modification works.

int kprobe___raw_spin_lock(struct pt_regs *ctx, u64 lock)
{
    struct event event = {
        .direction = 0,
    };
    event.lock = lock,
    events.perf_submit(ctx, &event, sizeof event);
    bpf_trace_printk("lock %ld\n", lock);
    return 0;
}

P. S. I read that tracing spinlocks might be tricky and that kretprobes are rightfully forbidden for some raw spinlock functions but kprobes aren't. For _raw_spin_lock I understand it, we can trace before locked. For _raw_spin_unlock we don't seem to care but for _raw_spin_unlock_irqrestore there must be some reason. How does that cause a lockup?

[yonghong-song]

@pavlix this is not a bug. The difference is due to how compiler generates code for data structures with padding.

For the case

    struct event event = {
        .direction = 0,
        .lock = lock,
    };

padding is not initialized and the verifier rightfully complained.

For

    struct event event = {
        .direction = 0,
    };
    event.lock = lock,

The compiler actually generated two 64bit loads so padding are initialized so the verifier is happy.

The recommended portable way is actually like below:

   __builtin_memset(&event, 0, sizeof(event));
   event.lock = lock;

This will ensure padding is always zeroed.