Messages in this thread | | | Date | Fri, 3 Jul 2015 20:56:57 +0200 | Subject | Re: perf: fuzzer triggered warning in intel_pmu_drain_pebs_nhm() | From | Stephane Eranian <> |
| |
Peter,
On Fri, Jul 3, 2015 at 3:13 PM, Peter Zijlstra <peterz@infradead.org> wrote: > On Thu, Jul 02, 2015 at 11:18:10AM -0400, Vince Weaver wrote: >> >> So sad to say the lack of fuzzer reports was because I was out of town for >> a bit, not due to the kernel suddenly getting amazingly better. >> >> In any case I am running against current git and getting a lot of >> warnings, but most of them seem to be old ones. This following one looks >> new though. >> >> This is current linus-git on a Haswell machine with peterz's patch to fix >> the aux buffer spinlock recursion (I can still crash the kernel if that >> patch is not applied). >> >> It corresponds to: >> >> WARN_ON_ONCE(!event->attr.precise_ip); >> >> [ 584.352324] WARNING: CPU: 2 PID: 18924 at arch/x86/kernel/cpu/perf_event_intel_ds.c:1198 intel_pmu_drain_pebs_nhm+0x283/0x2e0() > > I've not yet tried to reproduce, but the below could explain things. > > On disabling an event we first clear our cpuc->pebs_enabled bits, only > to then check them to see if there are any set, and if so, drain the > buffer. > > If we just cleared the last bit, we'll fail to drain the buffer. > Where do you see that we use cpuc->pebs_enabled after clearing it in pebs_disable() to check for overflow or active in drain_pebs()? I only see it used in get_next_pebs_record_by_bit()?
> If we then program another event on that counter and another PEBS event, > we can hit the above WARN with the 'stale' entries left over from the > previous event. > > --- > arch/x86/kernel/cpu/perf_event_intel_ds.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c b/arch/x86/kernel/cpu/perf_event_intel_ds.c > index 71fc40238843..041a30ba5654 100644 > --- a/arch/x86/kernel/cpu/perf_event_intel_ds.c > +++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c > @@ -548,7 +548,7 @@ int intel_pmu_drain_bts_buffer(void) > > static inline void intel_pmu_drain_pebs_buffer(void) > { > - struct pt_regs regs; > + struct pt_regs regs; /* SAMPLE_REGS_INTR must not be set for FREERUNNING */ > > x86_pmu.drain_pebs(®s); > } > @@ -755,13 +755,6 @@ void intel_pmu_pebs_disable(struct perf_event *event) > struct hw_perf_event *hwc = &event->hw; > struct debug_store *ds = cpuc->ds; > > - cpuc->pebs_enabled &= ~(1ULL << hwc->idx); > - > - if (event->hw.flags & PERF_X86_EVENT_PEBS_LDLAT) > - cpuc->pebs_enabled &= ~(1ULL << (hwc->idx + 32)); > - else if (event->hw.flags & PERF_X86_EVENT_PEBS_ST) > - cpuc->pebs_enabled &= ~(1ULL << 63); > - > if (ds->pebs_interrupt_threshold > > ds->pebs_buffer_base + x86_pmu.pebs_record_size) { > intel_pmu_drain_pebs_buffer(); > @@ -769,6 +762,13 @@ void intel_pmu_pebs_disable(struct perf_event *event) > perf_sched_cb_dec(event->ctx->pmu); > } > > + cpuc->pebs_enabled &= ~(1ULL << hwc->idx); > + > + if (event->hw.flags & PERF_X86_EVENT_PEBS_LDLAT) > + cpuc->pebs_enabled &= ~(1ULL << (hwc->idx + 32)); > + else if (event->hw.flags & PERF_X86_EVENT_PEBS_ST) > + cpuc->pebs_enabled &= ~(1ULL << 63); > + > if (cpuc->enabled) > wrmsrl(MSR_IA32_PEBS_ENABLE, cpuc->pebs_enabled); >
| |