linux-kernel - Re: [PATCH v2 1/3] perf: Reorder overflow handler ahead of event

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CANpmjNOCFz43zhJm1mS5Ai1XTQiZ_-BW2K3z8vJEV4CyfoZtCA@mail.gmail.com>
Date:   Thu, 7 Dec 2023 18:53:58 +0100
From:   Marco Elver <elver@...gle.com>
To:     Kyle Huey <me@...ehuey.com>
Cc:     Kyle Huey <khuey@...ehuey.com>,
        open list <linux-kernel@...r.kernel.org>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Yonghong Song <yonghong.song@...ux.dev>,
        "Robert O'Callahan" <robert@...llahan.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...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
Subject: Re: [PATCH v2 1/3] perf: Reorder overflow handler ahead of event_limit/sigtrap

On Thu, 7 Dec 2023 at 18:47, Kyle Huey <me@...ehuey.com> wrote:
>
>
>
> On Thu, Dec 7, 2023, 9:05 AM Marco Elver <elver@...gle.com> wrote:
>>
>> On Thu, 7 Dec 2023 at 17:35, Kyle Huey <me@...ehuey.com> wrote:
>> >
>> > The perf subsystem already allows an overflow handler to clear pending_kill
>> > to suppress a fasync signal (although nobody currently does this). Allow an
>> > overflow handler to suppress the other visible side effects of an overflow,
>> > namely event_limit accounting and SIGTRAP generation.
>> >
>> > Signed-off-by: Kyle Huey <khuey@...ehuey.com>
>> > ---
>> >  kernel/events/core.c | 10 +++++++---
>> >  1 file changed, 7 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/kernel/events/core.c b/kernel/events/core.c
>> > index b704d83a28b2..19fddfc27a4a 100644
>> > --- a/kernel/events/core.c
>> > +++ b/kernel/events/core.c
>> > @@ -9541,6 +9541,12 @@ static int __perf_event_overflow(struct perf_event *event,
>> >          */
>> >
>> >         event->pending_kill = POLL_IN;
>> > +
>> > +       READ_ONCE(event->overflow_handler)(event, data, regs);
>> > +
>> > +       if (!event->pending_kill)
>> > +               return ret;
>>
>> It's not at all intuitive that resetting pending_kill to 0 will
>> suppress everything else, too. There is no relationship between the
>> fasync signals and SIGTRAP. pending_kill is for the former and
>> pending_sigtrap is for the latter. One should not affect the other.
>>
>> A nicer solution would be to properly undo the various pending_*
>> states (in the case of pending_sigtrap being set it should be enough
>> to reset pending_sigtrap to 0, and also decrement
>> event->ctx->nr_pending).
>
>
> I don't believe it's possible to correctly undo the event_limit decrement after the fact (if it's e.g. racing with the ioctl that adds to the event limit).
>
>> Although I can see why this solution is simpler. Perhaps with enough
>> comments it might be clearer.
>>
>> Preferences?
>
>
> The cleanest way would probably be to add a return value to the overflow handler function that controls this. It requires changing a bunch of arch specific code on arches I don't have access to though.

Hmm.

Maybe wait for perf maintainers to say what is preferrable. (I could
live with just making sure this has no other weird side effects and
more comments.)