linux-kernel - Re: [RESEND PATCH v5 1/4] perf/bpf: Call bpf handler directly, not through overflow machinery

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP045AqQ0MbF2PAm9f5t=PnkJ4eOnwsNR624gEEjyLWEpTFz1g@mail.gmail.com>
Date: Thu, 11 Apr 2024 08:11:28 -0400
From: Kyle Huey <me@...ehuey.com>
To: Ingo Molnar <mingo@...nel.org>
Cc: linux-kernel@...r.kernel.org, Andrii Nakryiko <andrii.nakryiko@...il.com>, 
	Jiri Olsa <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>, Marco Elver <elver@...gle.com>, 
	Yonghong Song <yonghong.song@...ux.dev>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>, 
	"Robert O'Callahan" <robert@...llahan.org>, Song Liu <song@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Ian Rogers <irogers@...gle.com>, 
	Adrian Hunter <adrian.hunter@...el.com>, linux-perf-users@...r.kernel.org, 
	bpf@...r.kernel.org
Subject: Re: [RESEND PATCH v5 1/4] perf/bpf: Call bpf handler directly, not
 through overflow machinery

On Wed, Apr 10, 2024 at 12:32 AM Ingo Molnar <mingo@...nel.org> wrote:
>
>
> * Kyle Huey <me@...ehuey.com> wrote:
>
> > To ultimately allow bpf programs attached to perf events to completely
> > suppress all of the effects of a perf event overflow (rather than just the
> > sample output, as they do today), call bpf_overflow_handler() from
> > __perf_event_overflow() directly rather than modifying struct perf_event's
> > overflow_handler. Return the bpf program's return value from
> > bpf_overflow_handler() so that __perf_event_overflow() knows how to
> > proceed. Remove the now unnecessary orig_overflow_handler from struct
> > perf_event.
> >
> > This patch is solely a refactoring and results in no behavior change.
> >
> > Signed-off-by: Kyle Huey <khuey@...ehuey.com>
> > Suggested-by: Namhyung Kim <namhyung@...nel.org>
> > Acked-by: Song Liu <song@...nel.org>
> > Acked-by: Jiri Olsa <jolsa@...nel.org>
> > ---
> >  include/linux/perf_event.h |  6 +-----
> >  kernel/events/core.c       | 28 +++++++++++++++-------------
> >  2 files changed, 16 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > index d2a15c0c6f8a..c7f54fd74d89 100644
> > --- a/include/linux/perf_event.h
> > +++ b/include/linux/perf_event.h
> > @@ -810,7 +810,6 @@ struct perf_event {
> >       perf_overflow_handler_t         overflow_handler;
> >       void                            *overflow_handler_context;
> >  #ifdef CONFIG_BPF_SYSCALL
> > -     perf_overflow_handler_t         orig_overflow_handler;
> >       struct bpf_prog                 *prog;
> >       u64                             bpf_cookie;
> >  #endif
>
> Could we reduce the #ifdeffery please?

Not easily.

> On distros CONFIG_BPF_SYSCALL is almost always enabled, so it's not like
> this truly saves anything on real systems.
>
> I'd suggest making the perf_event::prog and perf_event::bpf_cookie fields
> unconditional.

That's not sufficient. See below.

> > +#ifdef CONFIG_BPF_SYSCALL
> > +static int bpf_overflow_handler(struct perf_event *event,
> > +                             struct perf_sample_data *data,
> > +                             struct pt_regs *regs);
> > +#endif
>
> If the function definitions are misordered then first do a patch that moves
> the function earlier in the file, instead of slapping a random prototype
> into a random place.

Ok.

> > -     READ_ONCE(event->overflow_handler)(event, data, regs);
> > +#ifdef CONFIG_BPF_SYSCALL
> > +     if (!(event->prog && !bpf_overflow_handler(event, data, regs)))
> > +#endif
> > +             READ_ONCE(event->overflow_handler)(event, data, regs);
>
> This #ifdef would go away too - on !CONFIG_BPF_SYSCALL event->prog should
> always be NULL.

bpf_overflow_handler() is also #ifdef CONFIG_BPF_SYSCALL. It uses
bpf_prog_active, so that would need to be moved out of the ifdef,
which would require moving the DEFINE_PER_CPU out of bpf/syscall.c ...
or I'd have to add a !CONFIG_BPF_SYSCALL definition of
bpf_overflow_handler() that only returns 1 and never actually gets
called because the condition short-circuits on event->prog. Neither
seems like it makes my patch or the code simpler, especially since
this weird ifdef-that-applies-only-to-the-condition goes away in Part
3 where I actually change the behavior.

It feels like the root of your objection is that CONFIG_BPF_SYSCALL
exists at all. I could remove it in a separate patch if there's
consensus about that.




> Please keep the #ifdeffery reduction and function-moving patches separate
> from these other changes.
>
> Thanks,
>
>         Ingo

- Kyle