perf: Fuzzer fixes

hi,
sending out 3 fuzzer fixes, which kept my server
up and fuzzing over the xmas ;-)

Available also in here:
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
perf/fuzzer

jirka

---
Jiri Olsa (4):
perf/x86/intel: Account interrupts for PEBS errors perf/x86: Fix period for non sampling events
perf: Add perf_event_overflow_throttle function perf/x86/intel: Throttle PEBS events only from pmi

 arch/x86/events/core.c       |  7 +++++++
 arch/x86/events/intel/core.c |  2 +-
 arch/x86/events/intel/ds.c   | 21 +++++++++++++--------
 arch/x86/events/perf_event.h |  2 +-
 include/linux/perf_event.h   |  5 +++++
 kernel/events/core.c         | 55 +++++++++++++++++++++++++++++++++++--------------------
 6 files changed, 62 insertions(+), 30 deletions(-)

It's possible to setup PEBS events to get only errors and not a single data, like on SNB-X (model 45) and IVB-EP (model 62) via 2 perf commands running simultaneously:

taskset -c 1 ./perf record -c 4 -e branches:pp -j any -C 10

This leads to soft lock up, because the error path of the intel_pmu_drain_pebs_nhm does not account event->hw.interrupt for error PEBS interrupts, so in case you're getting ONLY errors you don;t have a way to stop event when it's over the max_samples_per_tick limit.

NMI watchdog: BUG: soft lockup - CPU#22 stuck for 22s! [perf_fuzzer:5816]
...

  RIP: 0010:[<ffffffff81159232>]  [<ffffffff81159232>] smp_call_function_single+0xe2/0x140
  ...
  Call Trace:
   ? trace_hardirqs_on_caller+0xf5/0x1b0
   ? perf_cgroup_attach+0x70/0x70
   perf_install_in_context+0x199/0x1b0
   ? ctx_resched+0x90/0x90
   SYSC_perf_event_open+0x641/0xf90
   SyS_perf_event_open+0x9/0x10
   do_syscall_64+0x6c/0x1f0
   entry_SYSCALL64_slow_path+0x25/0x25

Adding perf_event_account_interrupt which does the interrupt and frequency checks and calling it from drain_pebs's error path.

Keeping pending_kill and pending_wakeup logic only in __perf_event_overflow path, abecause they make sense only if there's any data to deliver.

---
 arch/x86/events/intel/ds.c |  6 +++++-
 include/linux/perf_event.h |  1 +
 kernel/events/core.c       | 47 ++++++++++++++++++++++++++++++----------------
 3 files changed, 37 insertions(+), 17 deletions(-)

index be202390bbd3..9dfeeeca0ea8 100644
@@ -1389,9 +1389,13 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs)
 			continue;
 
 		/* log dropped samples number */
-		if (error[bit])
+		if (error[bit]) {
 			perf_log_lost_samples(event, error[bit]);
 
+			if (perf_event_account_interrupt(event))
+				x86_pmu_stop(event, 0);
+		}
+
 		if (counts[bit]) {
 			__intel_pmu_pebs_event(event, iregs, base,
 					       top, bit, counts[bit]);

index 4741ecdb9817..78ed8105e64d 100644
@@ -1259,6 +1259,7 @@ extern void perf_event_disable(struct perf_event *event);
 extern void perf_event_disable_local(struct perf_event *event);
 extern void perf_event_disable_inatomic(struct perf_event *event);
 extern void perf_event_task_tick(void);
+extern int perf_event_account_interrupt(struct perf_event *event);
 #else /* !CONFIG_PERF_EVENTS: */
 static inline void *
 perf_aux_output_begin(struct perf_output_handle *handle,

2.7.4

When in counting mode we setup the counter with the longest possible period and read the value with read syscall.

We also still setup the PMI to be triggered when such counter overflow to reconfigure it.

We also get PEBS interrupt if such counter has precise_ip set (which makes no sense, but it's possible).

Having such counter with:
- counting mode
- precise_ip set

I watched my server to get stuck serving PEBS interrupt again and again because of following (AFAICS):

- PEBS interrupt is triggered before PMI
- when PEBS handling path reconfigured counter it had remaining value of -256
- the x86_perf_event_set_period does not consider this as an extreme value, so it's configured back as the new counter value
- this makes the PEBS interrupt to be triggered right away again
- and because it's non sampling event, this irq storm is never throttled

Forcing the non sampling events to reconfigure from scratch is probably not the best solution, but it seems to work.

---
 arch/x86/events/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

index f1c22584a46f..657486be9780 100644
@@ -1116,6 +1116,13 @@ int x86_perf_event_set_period(struct perf_event *event)
 		return 0;
 
 	/*
+	 * For non sampling event, we are not interested
+	 * in leftover, force the count from beginning.
+	 */
+	if (left && !is_sampling_event(event))
+		left = 0;
+
+	/*
 	 * If we are way outside a reasonable range then just skip forward:
 	 */
 	if (unlikely(left <= -period)) {

2.7.4

I think we should reject non sampling pebs events, as you say they make no sense what so ever.

ook, attached

jirka

---
As Peter suggested [1] rejecting non sampling PEBS events, because they dont make any sense and could cause issues in NMI handler [2].

[1] http://lkml.kernel.org/r/20170103094059.GC3093@worktop
[2] http://lkml.kernel.org/r/[email protected]

---
 arch/x86/events/core.c | 4 ++++
 1 file changed, 4 insertions(+)

index 64582cfa6976..48b851f0ef89 100644
@@ -505,6 +505,10 @@ int x86_pmu_hw_config(struct perf_event *event)
 
 		if (event->attr.precise_ip > precise)
 			return -EOPNOTSUPP;
+
+		/* There's no sense in having PEBS for non sampling events. */
+		if (!is_sampling_event(event))
+			return -EINVAL;
 	}
 	/*
 	 * check that PEBS LBR correction does not conflict with

2.9.3

you can use the PEBS events to gather aggregate stats though and they seem roughly right. Are they truly meaningless?

I had misremembered that they might not have the determinism problems of regular events (turns out that's wrong). They oddly seem to be worse in some limited tests I did.

So I guess nothing will be lost if they're disabled.

Vince

Slightly confused, the PEBS interrupt _is_ the PMI. And how can we get an interrupt before the counter overflows?

You're talking about the actual counter value, right, not @left?

Right, a counter value of -256 would result in @left being 256 which is positive and not too large, so we 'retain' the value.

So I'm curious how this is even possible. The normal described way of things is:

- we program the counter with a negative value
- each 'event' does a counter increment
- if we 'overflow' (turn positive) we start to arm the PEBS assist
- once the assist is armed, the next 'event' triggers a PEBS record.
- if the amount of PEBS records exceeds the DS threshold, we set bit 62 in GLOBAL_STATUS and raise the PMI.

At which point the actual counter value should be at the very least 1 (for having counted the event that triggers the PEBS assist into creating the record).

Did your kernel include commit:

daa864b8f8e3 ("perf/x86/pebs: Fix handling of PEBS buffer overflows")

?

right

heh, I guess I thought this could happen earlier ;-) otherwise I dont get how could I saw -256 left in the counter value..

what I saw was the bit 62 set and pebs_drain->__intel_pmu_pebs_event re-configuring the event back with -256 again and again..

I'll run fuzzer again without the fix with my debug stuff in and try to recreate ;-)

yep

jirka

Adding perf_event_overflow_throttle function to allow callers to decide on throttling events. It's used in following patch.

---
 include/linux/perf_event.h |  4 ++++
 kernel/events/core.c       | 14 +++++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

index 78ed8105e64d..f5a9468bad90 100644
@@ -966,6 +966,10 @@ extern int perf_event_overflow(struct perf_event *event,
 				 struct perf_sample_data *data,
 				 struct pt_regs *regs);
 
+extern int perf_event_overflow_throttle(struct perf_event *event,
+					int throttle, struct perf_sample_data *data,
+					struct pt_regs *regs);
+
 extern void perf_event_output_forward(struct perf_event *event,
 				     struct perf_sample_data *data,
 				     struct pt_regs *regs);

index 38f4baef5df5..466ed56340bc 100644
@@ -7079,9 +7079,9 @@ int perf_event_account_interrupt(struct perf_event *event)
  * Generic event overflow handling, sampling.
  */
 
-static int __perf_event_overflow(struct perf_event *event,
-				   int throttle, struct perf_sample_data *data,
-				   struct pt_regs *regs)
+int perf_event_overflow_throttle(struct perf_event *event,
+				 int throttle, struct perf_sample_data *data,
+				 struct pt_regs *regs)
 {
 	int events = atomic_read(&event->event_limit);
 	int ret = 0;

@@ -7122,7 +7122,7 @@ int perf_event_overflow(struct perf_event *event,
 			  struct perf_sample_data *data,
 			  struct pt_regs *regs)
 {
-	return __perf_event_overflow(event, 1, data, regs);
+	return perf_event_overflow_throttle(event, 1, data, regs);
 }
 
 /*

@@ -7184,8 +7184,8 @@ static void perf_swevent_overflow(struct perf_event *event, u64 overflow,
 		return;
 
 	for (; overflow; overflow--) {
-		if (__perf_event_overflow(event, throttle,
-					    data, regs)) {
+		if (perf_event_overflow_throttle(event, throttle,
+						 data, regs)) {
 			/*
 			 * We inhibit the overflow from happening when
 			 * hwc->interrupts == MAX_INTERRUPTS.

@@ -8298,7 +8298,7 @@ static enum hrtimer_restart perf_swevent_hrtimer(struct hrtimer *hrtimer)
 
 	if (regs && !perf_exclude_event(event, regs)) {
 		if (!(event->attr.exclude_idle && is_idle_task(current)))
-			if (__perf_event_overflow(event, 1, &data, regs))
+			if (perf_event_overflow_throttle(event, 1, &data, regs))
 				ret = HRTIMER_NORESTART;
 	}
 

2.7.4

This patch fixes following WARNING:

  WARNING: CPU: 15 PID: 15768 at arch/x86/events/core.c:1256 x86_pmu_start+0x1b3/0x1c0
  ...
  Call Trace:
   <IRQ>
   dump_stack+0x86/0xc3
   __warn+0xcb/0xf0
   warn_slowpath_null+0x1d/0x20
   x86_pmu_start+0x1b3/0x1c0
   perf_event_task_tick+0x342/0x3f0
   scheduler_tick+0x75/0xd0
   update_process_times+0x47/0x60
   tick_sched_handle.isra.19+0x25/0x60
   tick_sched_timer+0x3d/0x70
   __hrtimer_run_queues+0xfb/0x510
   hrtimer_interrupt+0x9d/0x1a0
   local_apic_timer_interrupt+0x38/0x60
   smp_trace_apic_timer_interrupt+0x56/0x25a
   trace_apic_timer_interrupt+0x9d/0xb0
   ...

which happens AFAICS under following conditions:
(we have PEBS events configured)

- x86_pmu_enable reconfigures counters and calls:
- x86_pmu_stop on PEBS event
- x86_pmu_stop drains the PEBS buffer, crosses the throttle limit and sets:
event->hw.interrupts = MAX_INTERRUPTS
- following x86_pmu_start call starts the event
- perf_event_task_tick is triggered
- perf_adjust_freq_unthr_context sees event with MAX_INTERRUPTS set and calls x86_pmu_start on already started event, which triggers the warning

My first attempt to fix this was to unthrottle the event before starting it in x86_pmu_enable. But I think that omitting the throttling completely when we are not in the PMI is better.

---
 arch/x86/events/intel/core.c |  2 +-
 arch/x86/events/intel/ds.c   | 17 +++++++++--------
 arch/x86/events/perf_event.h |  2 +-
 3 files changed, 11 insertions(+), 10 deletions(-)

index 86138267b68a..16332867d435 100644
@@ -2137,7 +2137,7 @@ 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);
+		x86_pmu.drain_pebs(regs, 1);
 		status &= x86_pmu.intel_ctrl | GLOBAL_STATUS_TRACE_TOPAPMI;
 	}
 

index 9dfeeeca0ea8..75ba3cb06012 100644
@@ -603,7 +603,7 @@ static inline void intel_pmu_drain_pebs_buffer(void)
 {
 	struct pt_regs regs;
 
-	x86_pmu.drain_pebs(&regs);
+	x86_pmu.drain_pebs(&regs, 0);
 }
 
 void intel_pmu_pebs_sched_task(struct perf_event_context *ctx, bool sched_in)

@@ -1233,7 +1233,7 @@ get_next_pebs_record_by_bit(void *base, void *top, int bit)
 static void __intel_pmu_pebs_event(struct perf_event *event,
 				   struct pt_regs *iregs,
 				   void *base, void *top,
-				   int bit, int count)
+				   int bit, int count, int pmi)
 {
 	struct perf_sample_data data;
 	struct pt_regs regs;

@@ -1257,14 +1257,14 @@ static void __intel_pmu_pebs_event(struct perf_event *event,
 	 * All but the last records are processed.
 	 * The last one is left to be able to call the overflow handler.
 	 */
-	if (perf_event_overflow(event, &data, &regs)) {
+	if (perf_event_overflow_throttle(event, pmi, &data, &regs)) {
 		x86_pmu_stop(event, 0);
 		return;
 	}
 
 }
 
-static void intel_pmu_drain_pebs_core(struct pt_regs *iregs)
+static void intel_pmu_drain_pebs_core(struct pt_regs *iregs, int pmi)
 {
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 	struct debug_store *ds = cpuc->ds;

@@ -1295,10 +1295,10 @@ static void intel_pmu_drain_pebs_core(struct pt_regs *iregs)
 	if (n <= 0)
 		return;
 
-	__intel_pmu_pebs_event(event, iregs, at, top, 0, n);
+	__intel_pmu_pebs_event(event, iregs, at, top, 0, n, pmi);
 }
 
-static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs)
+static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs, int pmi)
 {
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 	struct debug_store *ds = cpuc->ds;

@@ -1392,13 +1392,14 @@ static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs)
 		if (error[bit]) {
 			perf_log_lost_samples(event, error[bit]);
 
-			if (perf_event_account_interrupt(event))
+			if (pmi && perf_event_account_interrupt(event))
 				x86_pmu_stop(event, 0);
 		}
 
 		if (counts[bit]) {
 			__intel_pmu_pebs_event(event, iregs, base,
-					       top, bit, counts[bit]);
+					       top, bit, counts[bit],
+					       pmi);
 		}
 	}
 }

index bcbb1d2ae10b..cdcfdd06b2d2 100644
@@ -590,7 +590,7 @@ struct x86_pmu {
 			pebs_prec_dist	:1;
 	int		pebs_record_size;
 	int		pebs_buffer_size;
-	void		(*drain_pebs)(struct pt_regs *regs);
+	void		(*drain_pebs)(struct pt_regs *regs, int pmi);
 	struct event_constraint *pebs_constraints;
 	void		(*pebs_aliases)(struct perf_event *event);
 	int 		max_pebs_events;

2.7.4

Hurm,.. good catch that. Makes a bit of a mess of things though. Then again, I cannot quickly think of something better either :/

So I don't particularly like these patches... they make a wee bit of a mess.

Under the assumption that draining a single event is on the same order of cost as a regular PMI, then accounting a drain of multiple events as an equal amount of interrupts makes sense.

We should not disregard this work. Now it looks like both (BTS & PEBS) drain methods only count a single interrupt, that's something we maybe ought to fix too.

So these things that drain are different from the regular case in that ::stop() will do this extra work not 'expected' by the regular core, so we must do something special. But 'hiding' the work is not correct.

Arguably the x86_pmu_start() call in x86_pmu_enable() is wrong, if the stop caused a throttle, we should respect that. The problem is that we 'loose' the x86_pmu_stop() call done by drain. We check PERF_HES_STOPPED() before doing x86_pmu_stop(), but we cannot do thereafter because HES_STOPPED will always be set.

Hmm, so we have:

  x86_pmu_enable()
    if (HES_STOPPED)
      hwc->state |= HES_ARCH;

    x86_pmu_stop()
      if __tac(active_mask) (true)
	x86_pmu.disable() := intel_pmu_disable_event()
	  intel_pmu_pebs_disable()
	    intel_pmu_drain_pebs_buffer()
	      x86_pmu_stop()
		__tac(active_mask) (false)
      hwc->state |= HES_STOPPED;

    if (!HES_ARCH)
      x86_pmu_start();

So if we have that recursive stop also set ARCH, things might just work.

---

index 1635c0c8df23..a95707a4140f 100644
@@ -1343,6 +1343,8 @@ void x86_pmu_stop(struct perf_event *event, int flags)
 		cpuc->events[hwc->idx] = NULL;
 		WARN_ON_ONCE(hwc->state & PERF_HES_STOPPED);
 		hwc->state |= PERF_HES_STOPPED;
+	} else {
+		hwc->state |= PERF_HES_ARCH;
 	}
 
 	if ((flags & PERF_EF_UPDATE) && !(hwc->state & PERF_HES_UPTODATE)) {

On Tue, Jan 24, 2017 at 05:41:22PM +0100, Peter Zijlstra wrote:

SNIP

I understood it like we're trying more to protect the interrupt context abuse rather than account the samples here.. but yes, we compare (hwc->interrupts >= max_samples_per_tick) for throttling so I guess we should not cross max_samples_per_tick in any case

right, we account just single hwc->interrupts++, and BTS drain does not have the throttle logic

I tried to put the perf_event_overflow call into both PEBS and BTS drains (below, untested). I think it's going to be slower especially for BTS code, which bypassed single event allocations and allocated one buffer for all samples at once.

jirka

---

index be202390bbd3..703bd82c69b1 100644
@@ -520,10 +520,7 @@ int intel_pmu_drain_bts_buffer(void)
 	};
 	struct perf_event *event = cpuc->events[INTEL_PMC_IDX_FIXED_BTS];
 	struct bts_record *at, *base, *top;
-	struct perf_output_handle handle;
-	struct perf_event_header header;
 	struct perf_sample_data data;
-	unsigned long skip = 0;
 	struct pt_regs regs;
 
 	if (!event)

@@ -562,40 +559,18 @@ int intel_pmu_drain_bts_buffer(void)
 		 */
 		if (event->attr.exclude_kernel &&
 		    (kernel_ip(at->from) || kernel_ip(at->to)))
-			skip++;
-	}
-
-	/*
-	 * Prepare a generic sample, i.e. fill in the invariant fields.
-	 * We will overwrite the from and to address before we output
-	 * the sample.
-	 */
-	rcu_read_lock();
-	perf_prepare_sample(&header, &data, event, &regs);
-
-	if (perf_output_begin(&handle, event, header.size *
-			      (top - base - skip)))
-		goto unlock;
-
-	for (at = base; at < top; at++) {
-		/* Filter out any records that contain kernel addresses. */
-		if (event->attr.exclude_kernel &&
-		    (kernel_ip(at->from) || kernel_ip(at->to)))
 			continue;
 
 		data.ip		= at->from;
 		data.addr	= at->to;
 
-		perf_output_sample(&handle, &header, &data, event);
-	}
+		if (perf_event_overflow(event, &data, &regs)) {
+			x86_pmu_stop(event, 0);
+			break;
+		}
 
-	perf_output_end(&handle);
+	}
 
-	/* There's new data available. */
-	event->hw.interrupts++;
-	event->pending_kill = POLL_IN;
-unlock:
-	rcu_read_unlock();
 	return 1;
 }
 

@@ -1243,25 +1218,18 @@ static void __intel_pmu_pebs_event(struct perf_event *event,
 	    !(event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD))
 		return;
 
-	while (count > 1) {
+	while (count > 0) {
 		setup_pebs_sample_data(event, iregs, at, &data, &regs);
-		perf_event_output(event, &data, &regs);
+
+		if (perf_event_overflow(event, &data, &regs)) {
+			x86_pmu_stop(event, 0);
+			break;
+		}
+
 		at += x86_pmu.pebs_record_size;
 		at = get_next_pebs_record_by_bit(at, top, bit);
 		count--;
 	}
-
-	setup_pebs_sample_data(event, iregs, at, &data, &regs);
-
-	/*
-	 * All but the last records are processed.
-	 * The last one is left to be able to call the overflow handler.
-	 */
-	if (perf_event_overflow(event, &data, &regs)) {
-		x86_pmu_stop(event, 0);
-		return;
-	}
-
 }
 
 static void intel_pmu_drain_pebs_core(struct pt_regs *iregs)

On Tue, Jan 24, 2017 at 05:41:22PM +0100, Peter Zijlstra wrote:

SNIP