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