Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] The patch introduces a build failure/linker error on configurations where `CONFIG_BPF_EVENTS` is disabled. - [High] Severe global lock contention and performance regression during `fork()` for all inherited perf events. Pre-existing issues: - [High] Memory and reference count leak of BPF programs and address filters in the `perf_event_alloc` error path. - [High] The patch fails to fix the use-after-free race condition for non-tracing events, as they bypass `bpf_event_mutex`. - [High] Tracepoint BPF programs are globally detached and disabled when any inherited child perf event is closed. -- --- Patch [1]: [PATCH] perf/bpf: Fix lockless access to parent_event->prog in perf_event_alloc() --- commit f1b901f0de2c25ddd86c536608909f0fd81cff45 Author: Deepanshu Kartikey <[email protected]> perf/bpf: Fix lockless access to parent_event->prog in perf_event_alloc() This patch attempts to fix a use-after-free and NULL pointer dereference in perf_event_alloc() during fork(). It introduces bpf_event_mutex to synchronize the inheritance of the BPF program with concurrent detach operations. > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 2eee83cdb43de..e62c34f1f89c7 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -13463,12 +13463,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); [Severity: High] Does this introduce a linker error when 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 is enabled. However, this code is guarded by checking CONFIG_BPF_SYSCALL and CONFIG_EVENT_TRACING. If a configuration has CONFIG_BPF_SYSCALL=y, CONFIG_EVENT_TRACING=y, and CONFIG_PERF_EVENTS=y, but disables CONFIG_KPROBE_EVENTS and CONFIG_UPROBE_EVENTS, CONFIG_BPF_EVENTS evaluates to n. Will this result in an undefined reference to bpf_event_mutex? [Severity: High] Does unconditionally acquiring this global lock penalize the performance of process creation when basic perf inherited tracing is active? This acquires bpf_event_mutex across all CPUs during fork() for every inherited event, even when parent_event->prog is NULL. Should there be a lockless fast-path check, for example: if (READ_ONCE(parent_event->prog)) before acquiring the mutex? [Severity: High] This is a pre-existing issue, but does this fail to prevent the race condition for non-tracing events? While bpf_event_mutex is held here in perf_event_alloc(), concurrent detach operations for non-tracing events bypass this mutex entirely: kernel/events/core.c:perf_event_free_bpf_handler() { event->prog = NULL; bpf_prog_put(prog); } Since perf_event_free_bpf_handler() operates locklessly, doesn't the original use-after-free vulnerability remain unfixed for event types like BPF_PROG_TYPE_PERF_EVENT? > + prog = parent_event->prog; > + if (prog) { > bpf_prog_inc(prog); [Severity: High] This is a pre-existing issue, but does this leak the BPF program reference and address filters if perf_event_alloc() fails later on? If a failure occurs after this point, the error path uses the __free(__free_event) cleanup mechanism: kernel/events/core.c:__free_event() { ... } Unlike _free_event(), __free_event() omits the calls to perf_event_free_bpf_prog(event) and perf_free_addr_filters(event). Will this leave the reference incremented by bpf_prog_inc() unbalanced? > event->prog = prog; [Severity: High] This is a pre-existing issue, but does sharing the parent's program pointer here cause tracepoint BPF programs to be globally detached when a child exits? When an inherited child event is closed, it eventually calls: kernel/trace/bpf_trace.c:perf_event_detach_bpf_prog() { ... ret = bpf_prog_array_copy(old_array, event->prog, NULL, 0, &new_array); ... } This permanently removes event->prog from the tracepoint's shared global array. If an unprivileged child process exits, will it maliciously or accidentally remove the parent's BPF program from the tracepoint entirely? > } > + mutex_unlock(&bpf_event_mutex); > #endif > } [ ... ] > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 29260951aa871..73065ddad7f6d 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -1944,7 +1944,7 @@ const struct bpf_verifier_ops perf_event_verifier_ops = > { > const struct bpf_prog_ops perf_event_prog_ops = { > }; > > -static DEFINE_MUTEX(bpf_event_mutex); > +DEFINE_MUTEX(bpf_event_mutex); > > #define BPF_TRACE_MAX_PROGS 64 -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
