linux-kernel - Re: [PATCH 2/3] perf/x86/pebs: add workaround for broken OVFL status on HSW

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-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 15 Dec 2016 09:42:13 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Stephane Eranian <eranian@...gle.com>
Cc:     Jiri Olsa <jolsa@...hat.com>, Andi Kleen <ak@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        "mingo@...e.hu" <mingo@...e.hu>,
        "Liang, Kan" <kan.liang@...el.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>
Subject: Re: [PATCH 2/3] perf/x86/pebs: add workaround for broken OVFL status
 on HSW

On Wed, Dec 14, 2016 at 11:26:49PM -0800, Stephane Eranian wrote:
> On Wed, Dec 14, 2016 at 9:55 AM, Peter Zijlstra <peterz@...radead.org> wrote:
> >
> > Just spotted this again, ping?
> >
> Ok, on what processor running what command, so I can try and reproduce?

For me its more of a correctness issue, i've not actually spotted a
problem as such.

But every time I read this code it makes me wonder.

Supposing that the hardware sets the CTRL overflow flags but hasn't
generated the PEBS record yet (or not enough records to reach the PEBS
buffer threshold) we still don't want to process these events as if they
were !PEBS.

That is, we _never_ want to look at pebs_enabled, hence unconditional
masking of those bits.

Hardware _should_ not set them in the first place, but clearly it does
sometimes.

> >> How about we make the clear of pebs_enabled unconditional?
> >>
> >> ---
> >>  arch/x86/events/intel/core.c | 20 ++++++++++----------
> >>  1 file changed, 10 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> >> index 68fa55b4d42e..dc9579665425 100644
> >> --- a/arch/x86/events/intel/core.c
> >> +++ b/arch/x86/events/intel/core.c
> >> @@ -1883,6 +1883,16 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
> >>       status &= ~(GLOBAL_STATUS_COND_CHG |
> >>                   GLOBAL_STATUS_ASIF |
> >>                   GLOBAL_STATUS_LBRS_FROZEN);
> >> +     /*
> >> +      * There are cases where, even though, the PEBS ovfl bit is set
> >> +      * in GLOBAL_OVF_STATUS, the PEBS events may also have their
> >> +      * overflow bits set for their counters. We must clear them
> >> +      * here because they have been processed as exact samples in
> >> +      * the drain_pebs() routine. They must not be processed again
> >> +      * in the for_each_bit_set() loop for regular samples below.
> >> +      */
> >> +     status &= ~cpuc->pebs_enabled;
> >> +
> >>       if (!status)
> >>               goto done;
> >>
> >> @@ -1892,16 +1902,6 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
> >>       if (__test_and_clear_bit(62, (unsigned long *)&status)) {
> >>               handled++;
> >>               x86_pmu.drain_pebs(regs);
> >> -             /*
> >> -              * There are cases where, even though, the PEBS ovfl bit is set
> >> -              * in GLOBAL_OVF_STATUS, the PEBS events may also have their
> >> -              * overflow bits set for their counters. We must clear them
> >> -              * here because they have been processed as exact samples in
> >> -              * the drain_pebs() routine. They must not be processed again
> >> -              * in the for_each_bit_set() loop for regular samples below.
> >> -              */
> >> -             status &= ~cpuc->pebs_enabled;
> >> -             status &= x86_pmu.intel_ctrl | GLOBAL_STATUS_TRACE_TOPAPMI;
> >>       }
> >>
> >>       /*