Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [High] Unconditional use of `bpf_event_mutex` in `kernel/events/core.c` causes a linker error in valid Kconfig combinations where `CONFIG_BPF_EVENTS` is disabled. Pre-existing issues: - [High] The fix for the data race in `perf_event_alloc()` is incomplete because `bpf_event_mutex` is not held during the detachment of non-tracing BPF programs, leaving the use-after-free race unfixed. - [High] Dereferencing prog->type without rcu_read_lock_trace() exposes sleepable BPF programs to Use-After-Free. - [Medium] The patch misses a plain write to event->prog in perf_event_attach_bpf_prog(), leaving a data race with READ_ONCE(event->prog) unfixed. - [Medium] `perf_event_free_bpf_handler()` still contains a plain read of `event->prog` which can race with a concurrent `WRITE_ONCE()` in `perf_event_set_bpf_handler()`. - [Medium] Missing release barrier in perf_event_set_bpf_handler() allows BPF programs to observe uninitialized or stale bpf_cookie values. -- --- Patch [1]: [PATCH v2] perf/bpf: Fix data races in BPF perf event handling --- commit f4518a73c55a1297eca26073838ec515d34cb05a Author: Deepanshu Kartikey <[email protected]> perf/bpf: Fix data races in BPF perf event handling This commit attempts to fix data races around the event->prog field by introducing READ_ONCE and WRITE_ONCE in various handlers, and acquiring bpf_event_mutex during fork() in perf_event_alloc(). Link: https://lore.kernel.org/all/[email protected]/T/ > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index 48d851fbd8ea5..5c6dabb6dccb1 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -2136,4 +2136,6 @@ static inline void perf_lopwr_cb(bool mode) > } > #endif > > +extern struct mutex bpf_event_mutex; > + [Severity: High] Will this extern declaration lead to a linker error in valid Kconfig combinations where CONFIG_BPF_EVENTS is disabled? The bpf_event_mutex is defined in kernel/trace/bpf_trace.c, which is only compiled when CONFIG_BPF_EVENTS=y. If a kernel is configured with CONFIG_BPF_SYSCALL=y and CONFIG_EVENT_TRACING=y, but kprobe and uprobe events are disabled, this might result in an unresolved external symbol. > #endif /* _LINUX_PERF_EVENT_H */ > diff --git a/kernel/events/core.c b/kernel/events/core.c > index ba5bd6a78fe7b..1416066ef460e 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c [ ... ] > @@ -10708,7 +10707,7 @@ static inline int perf_event_set_bpf_handler(struct > perf_event *event, > return -EPROTO; > } > > - event->prog = prog; > + WRITE_ONCE(event->prog, prog); > event->bpf_cookie = bpf_cookie; > return 0; > } [Severity: Medium] This is a pre-existing issue, but does publishing event->prog without a release barrier allow concurrent readers in __perf_event_overflow() to observe an uninitialized or stale bpf_cookie? Without smp_store_release() or similar ordering, the CPU can reorder the event->prog write and the plain store to event->bpf_cookie. Since the reader uses READ_ONCE(), which lacks acquire semantics, a BPF program might read the uninitialized cookie. > @@ -10720,7 +10719,7 @@ static inline void perf_event_free_bpf_handler(struct > perf_event *event) > if (!prog) > return; > > - event->prog = NULL; > + WRITE_ONCE(event->prog, NULL); > bpf_prog_put(prog); > } [Severity: Medium] This is a pre-existing issue, but does perf_event_free_bpf_handler() still contain a plain read of event->prog? At the beginning of this function: struct bpf_prog *prog = event->prog; Could this read race with a concurrent WRITE_ONCE() from perf_event_set_bpf_handler() during ioctl(PERF_EVENT_IOC_SET_BPF)? [Severity: High] This is a pre-existing issue, but does perf_event_free_bpf_handler() need to acquire bpf_event_mutex to safely detach non-tracing BPF programs? Because this function clears event->prog and drops the program reference locklessly, a concurrent fork() calling perf_event_alloc() could acquire the mutex, read the program, and increment a zero-refcount object. > #else > @@ -10753,6 +10752,7 @@ static int __perf_event_overflow(struct perf_event > *event, > { > int events = atomic_read(&event->event_limit); > int ret = 0; > + struct bpf_prog *prog; > > /* > * Non-sampling counters might still use the PMI to fold short > @@ -10766,8 +10766,9 @@ static int __perf_event_overflow(struct perf_event > *event, > if (event->attr.aux_pause) > perf_event_aux_pause(event->aux_event, true); > > - if (event->prog && event->prog->type == BPF_PROG_TYPE_PERF_EVENT && > - !bpf_overflow_handler(event, data, regs)) > + prog = READ_ONCE(event->prog); > + if (prog && prog->type == BPF_PROG_TYPE_PERF_EVENT && > + !bpf_overflow_handler(event, data, regs, prog)) [Severity: High] This is a pre-existing issue, but does dereferencing prog->type here without rcu_read_lock_trace() expose sleepable BPF programs to a use-after-free? RCU Tasks Trace grace periods do not wait for NMIs or preemption-disabled sections (Classic RCU context). If a sleepable program is detached and freed via call_rcu_tasks_trace(), couldn't it be freed while __perf_event_overflow() is still executing and evaluating prog->type? > goto out; > > /* > @@ -13433,12 +13434,15 @@ perf_event_alloc(struct perf_event_attr *attr, int > cpu, > overflow_handler = parent_event->overflow_handler; > context = parent_event->overflow_handler_context; > #if defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_EVENT_TRACING) > - if (parent_event->prog) { > - struct bpf_prog *prog = parent_event->prog; > > + struct bpf_prog *prog; > > + mutex_lock(&bpf_event_mutex); > + prog = parent_event->prog; > + if (prog) { > bpf_prog_inc(prog); > event->prog = prog; > } > + mutex_unlock(&bpf_event_mutex); > #endif [Severity: High] As mentioned above, since bpf_event_mutex is only compiled when CONFIG_BPF_EVENTS=y, will unconditionally locking it here cause a build failure on kernels where that configuration is disabled? > } > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 75495a5c35079..026b5c4921158 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c [ ... ] > @@ -2018,7 +2018,7 @@ void perf_event_detach_bpf_prog(struct perf_event > *event) > > put: > prog = event->prog; > - event->prog = NULL; > + WRITE_ONCE(event->prog, NULL); > > unlock: > mutex_unlock(&bpf_event_mutex); [Severity: Medium] This isn't a bug introduced by this patch, but doesn't perf_event_attach_bpf_prog() still contain a plain write to event->prog? Since this patch fixes data races against the READ_ONCE() in the fast path, should the plain write: event->prog = prog; in the attach path also be converted to a WRITE_ONCE()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
