All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jiri Olsa <olsajiri@gmail.com>
To: sashiko-reviews@lists.linux.dev
Cc: Deepanshu Kartikey <kartikey406@gmail.com>,
	bpf@vger.kernel.org, linux-trace-kernel@vger.kernel.org,
	linux-perf-users@vger.kernel.org
Subject: Re: [PATCH] perf/bpf: Fix data race in __perf_event_overflow
Date: Wed, 12 Aug 2026 12:35:08 +0200	[thread overview]
Message-ID: <anxMXBwCX0HCYgxr@krava> (raw)
In-Reply-To: <20260812000859.654FC1F00A3A@smtp.kernel.org>

On Wed, Aug 12, 2026 at 12:08:58AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Lockless TOCTOU race in perf_event_alloc() causes NULL pointer dereference or Use-After-Free during BPF program inheritance.
> - [Medium] Incomplete fix for data races and missing memory barriers in BPF attach/detach paths.
> - [Medium] Time-of-Check to Time-of-Use (TOCTOU) race drops valid perf events during concurrent BPF detach.
> --
> 
> --- Patch [1]: [PATCH] perf/bpf: Fix data race in __perf_event_overflow ---
> commit f63a2be5ceb79a2a8bb0263ba6723f7bf4ae763d
> Author: Deepanshu Kartikey <kartikey406@gmail.com>
> 
> perf/bpf: Fix data race in __perf_event_overflow
> 
> This patch addresses a data race where __perf_event_overflow() concurrently
> reads event->prog without lock protection while perf_event_detach_bpf_prog()
> writes it to NULL. It fixes the race by introducing READ_ONCE() and
> WRITE_ONCE() in the respective read and write paths.
> 
> Reported-by: syzbot+651d2774bd1d8395595f@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=651d2774bd1d8395595f
> Fixes: f11f10bfa1ca ("perf/bpf: Call BPF handler directly, not through overflow machinery")
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> 
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> 
> [ ... ]
> 
> > @@ -10766,7 +10767,8 @@ 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 &&
> > +	prog = READ_ONCE(event->prog);
> > +	if (prog && prog->type == BPF_PROG_TYPE_PERF_EVENT &&
> >  	    !bpf_overflow_handler(event, data, regs))
> >  		goto out;
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but does the independent second
> read in bpf_overflow_handler() create a time-of-check to time-of-use race?
> 
> If a concurrent detach sets event->prog to NULL between the READ_ONCE() here
> in __perf_event_overflow() and the second READ_ONCE() inside
> bpf_overflow_handler():
> 
> bpf_overflow_handler() {
>     prog = READ_ONCE(event->prog);
>     if (prog) {
>         ...
>     }
>     return 0;
> }
> 
> Wouldn't bpf_overflow_handler() evaluate prog as NULL and return 0? The
> caller __perf_event_overflow() evaluates !0 as true and executes goto out,
> which unconditionally drops the perf sample.
> 
> Should the already-validated prog pointer be passed directly to
> bpf_overflow_handler()?

seems legit

> 
> 
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > --- 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:
> 
> [Severity: Medium]
> This is a pre-existing issue, but does this incomplete fix leave other data
> races in the attach and detach paths?
> 
> Other functions like perf_event_set_bpf_handler(),
> perf_event_free_bpf_handler(), and perf_event_attach_bpf_prog() still
> perform plain writes to event->prog without WRITE_ONCE().

was wondering the same, the perf_event_detach_bpf_prog is called for
tracing events (tracepoint/kprobe/uprobe) the perf event release goes
through perf_event_free_bpf_handler, check perf_event_free_bpf_prog

jirka

> 
> Additionally, does this need release and acquire semantics? If the writes to
> event->prog and event->bpf_cookie in the attach paths are reordered by the
> CPU, could the NMI handler read a valid event->prog but a stale or
> uninitialized event->bpf_cookie?
> 
> [Severity: High]
> This is also a pre-existing issue, but does the lockless access in
> perf_event_alloc() create a use-after-free or NULL pointer dereference
> during BPF program inheritance?
> 
> During fork, perf_event_alloc() inherits the BPF program:
> 
> perf_event_alloc() {
>     ...
>     if (parent_event->prog) {
>         struct bpf_prog *prog = parent_event->prog;
>         bpf_prog_inc(prog);
>         ...
>     }
> }
> 
> Since this accesses parent_event->prog locklessly, can a concurrent
> bpf_perf_link_release() clear the pointer? If the compiler reloads the
> pointer after the check, bpf_prog_inc() would dereference NULL.
> 
> Even if it is optimized to a single read, perf_event_alloc() is not inside
> an RCU read-side critical section. Could a concurrent detach finish its RCU
> grace period and free the BPF program, causing bpf_prog_inc() to write to
> freed memory?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260811235331.10044-1-kartikey406@gmail.com?part=1
> 

  reply	other threads:[~2026-08-12 10:35 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-11 23:53 [PATCH] perf/bpf: Fix data race in __perf_event_overflow Deepanshu Kartikey
2026-08-12  0:08 ` sashiko-bot
2026-08-12 10:35   ` Jiri Olsa [this message]
2026-08-12  1:04 ` bot+bpf-ci
2026-08-12 10:12 ` Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=anxMXBwCX0HCYgxr@krava \
    --to=olsajiri@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=kartikey406@gmail.com \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=linux-trace-kernel@vger.kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.