Re: [PATCH] perf/bpf: Fix lockless access to parent_event->prog in perf_event_alloc()

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 [off-list ref]

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.

[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?

[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?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821014210.18681-1-kartikey406@gmail.com?part=1