Re: [PATCH v2] perf/bpf: Fix data races in BPF perf event handling

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