[PATCHSET/RFC 0/2] perf: Fix issues reported by perf fuzzer

[PATCHSET/RFC 0/2] perf: Fix issues reported by perf fuzzer

[Jiri Olsa]

hi,
Vince reported several issues with perf code hit by perf fuzzer,
in here:
  http://marc.info/?l=linux-kernel&m=141806390822670&w=2

I run the fuzzer, but wasn't able to hit issues reported
by Vince. But.. I was lucky enough to hit other 2 issues
described in following patches ;-)

As I dont follow the uncore code much, it is sort of RFC
patchset. However I'm running the fuzzer now for several
hours and can't hit any other issue.

thanks,
jirka


Cc: Andi Kleen <ak@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Vince Weaver <vince@deater.net>
Cc: Yan, Zheng <zheng.z.yan@intel.com>
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
Jiri Olsa (2):
      perf/x86/intel/uncore: Make sure only uncore events are collected
      perf: Fix events installation during moving group

 arch/x86/kernel/cpu/perf_event_intel_uncore.c | 22 +++++++++++++++++++---
 kernel/events/core.c                          |  4 ++--
 2 files changed, 21 insertions(+), 5 deletions(-)

[PATCH 1/2] perf/x86/intel/uncore: Make sure only uncore events are collected

[Jiri Olsa]

The uncore_collect_events functions assumes that event group
might contain only uncore events which is wrong, because it
might contain any type of events.

This bug leads to uncore framework touching 'not' uncore events,
which could end up all sorts of bugs.

One was triggered by Vince's perf fuzzer, when the uncore code
touched breakpoint event private event space as if it was uncore
event and caused BUG:

---
[ 4696.010643] BUG: unable to handle kernel paging request at ffffffff82822068
[ 4696.018433] IP: [<ffffffff81020338>] uncore_assign_events+0x188/0x250
[ 4696.025633] PGD 1812067 PUD 1813063 PMD 0
[ 4696.030228] Oops: 0000 [#1] SMP
[ 4696.033844] Modules linked in: igb ptp ioatdma pps_core i2c_algo_bit i2c_core x86_pkg_temp_thermal crc32c_intel dca microcode lpc_ich mfd_core megaraid_sas
[ 4696.049503] CPU: 1 PID: 9199 Comm: perf_fuzzer Not tainted 3.18.0fuzzer #117
[ 4696.057368] Hardware name: IBM System x3650 M4 : -[7915E2G]-/00Y7683, BIOS -[VVE124AUS-1.30]- 11/21/2012
[ 4696.067947] task: ffff88026be9da00 ti: ffff88026b9b0000 task.ti: ffff88026b9b0000
[ 4696.076295] RIP: 0010:[<ffffffff81020338>]  [<ffffffff81020338>] uncore_assign_events+0x188/0x250
[ 4696.086207] RSP: 0018:ffff88026b9b3d48  EFLAGS: 00010246
[ 4696.092133] RAX: 0000000000000000 RBX: ffff88007a3eda00 RCX: ffffffff81821fa0
[ 4696.100095] RDX: ffff8802719a1000 RSI: 0000000008000446 RDI: 0000000000000000
[ 4696.108057] RBP: ffff88026b9b3db8 R08: ffffffff81821fe0 R09: ffff880277803600
[ 4696.116018] R10: ffffffff81821fa0 R11: ffffffff81020429 R12: ffff88007a3eda10
[ 4696.123980] R13: 0000000000000004 R14: 0000000000000000 R15: 0000000000000004
[ 4696.131942] FS:  00007ff960a2d740(0000) GS:ffff880277c20000(0000) knlGS:0000000000000000
[ 4696.140971] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 4696.147380] CR2: ffffffff82822068 CR3: 0000000273ef0000 CR4: 00000000000407e0
[ 4696.155341] DR0: 000000003af8b4b4 DR1: 0000000000000000 DR2: 0000000000000000
[ 4696.163302] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600
[ 4696.171264] Stack:
[ 4696.173505]  ffff88026ca5e000 0000000200000000 ffff88007a3eda10 0000000000000001
[ 4696.181797]  ffff88026b9b3d98 ffff88026ca5e000 ffff88007a3eda00 0000000000000000
[ 4696.190089]  0000000000000000 ffff88026ca5e000 ffff880473280100 ffff88007a3eda00
[ 4696.198381] Call Trace:
[ 4696.201109]  [<ffffffff81020c4c>] uncore_pmu_event_init+0x1cc/0x260
[ 4696.208104]  [<ffffffff81109053>] perf_init_event+0x93/0x130
[ 4696.214419]  [<ffffffff81109488>] perf_event_alloc+0x398/0x440
[ 4696.220928]  [<ffffffff811098f1>] SYSC_perf_event_open+0x3c1/0xbb0
[ 4696.227825]  [<ffffffff8110a539>] SyS_perf_event_open+0x9/0x10
[ 4696.234335]  [<ffffffff8154d712>] system_call_fastpath+0x12/0x17
---

The code in uncore_assign_events fucntion was looking for event->hw.idx
data while the event was initialized as a breakpoint with different members
in event->hw union.

This patch forces uncore_collect_events to collect only uncore events.

Reported-by: Vince Weaver <vince@deater.net>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Vince Weaver <vince@deater.net>
Cc: Yan, Zheng <zheng.z.yan@intel.com>
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 arch/x86/kernel/cpu/perf_event_intel_uncore.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.c b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
index 9762dbd9f3f7..e98f68cfea02 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
@@ -276,6 +276,17 @@ static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,
 	return box;
 }
 
+/*
+ * Using uncore_pmu_event_init pmu event_init callback
+ * as a detection point for uncore events.
+ */
+static int uncore_pmu_event_init(struct perf_event *event);
+
+static bool is_uncore_event(struct perf_event *event)
+{
+	return event->pmu->event_init == uncore_pmu_event_init;
+}
+
 static int
 uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader, bool dogrp)
 {
@@ -290,13 +301,18 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader, b
 		return -EINVAL;
 
 	n = box->n_events;
-	box->event_list[n] = leader;
-	n++;
+
+	if (is_uncore_event(leader)) {
+		box->event_list[n] = leader;
+		n++;
+	}
+
 	if (!dogrp)
 		return n;
 
 	list_for_each_entry(event, &leader->sibling_list, group_entry) {
-		if (event->state <= PERF_EVENT_STATE_OFF)
+		if (!is_uncore_event(event) ||
+		    event->state <= PERF_EVENT_STATE_OFF)
 			continue;
 
 		if (n >= max_count)
-- 
1.9.3

Re: [PATCH 1/2] perf/x86/intel/uncore: Make sure only uncore events are collected

[Andi Kleen]

On Wed, Dec 10, 2014 at 09:23:50PM +0100, Jiri Olsa wrote:
> The uncore_collect_events functions assumes that event group
> might contain only uncore events which is wrong, because it
> might contain any type of events.
> 
> This bug leads to uncore framework touching 'not' uncore events,
> which could end up all sorts of bugs.

Thanks for tracking that down. Looks good to me.

-Andi

[tip:perf/urgent] perf/x86/intel/uncore: Make sure only uncore events are collected

[tip-bot for Jiri Olsa]

Commit-ID:  af91568e762d04931dcbdd6bef4655433d8b9418
Gitweb:     http://git.kernel.org/tip/af91568e762d04931dcbdd6bef4655433d8b9418
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Wed, 10 Dec 2014 21:23:50 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 11 Dec 2014 11:24:14 +0100

perf/x86/intel/uncore: Make sure only uncore events are collected

The uncore_collect_events functions assumes that event group
might contain only uncore events which is wrong, because it
might contain any type of events.

This bug leads to uncore framework touching 'not' uncore events,
which could end up all sorts of bugs.

One was triggered by Vince's perf fuzzer, when the uncore code
touched breakpoint event private event space as if it was uncore
event and caused BUG:

   BUG: unable to handle kernel paging request at ffffffff82822068
   IP: [<ffffffff81020338>] uncore_assign_events+0x188/0x250
   ...

The code in uncore_assign_events() function was looking for
event->hw.idx data while the event was initialized as a
breakpoint with different members in event->hw union.

This patch forces uncore_collect_events() to collect only uncore
events.

Reported-by: Vince Weaver <vince@deater.net>
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Yan, Zheng <zheng.z.yan@intel.com>
Cc: <stable@vger.kernel.org>
Link: http://lkml.kernel.org/r/1418243031-20367-2-git-send-email-jolsa@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/perf_event_intel_uncore.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.c b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
index 9762dbd..e98f68c 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
@@ -276,6 +276,17 @@ static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,
 	return box;
 }
 
+/*
+ * Using uncore_pmu_event_init pmu event_init callback
+ * as a detection point for uncore events.
+ */
+static int uncore_pmu_event_init(struct perf_event *event);
+
+static bool is_uncore_event(struct perf_event *event)
+{
+	return event->pmu->event_init == uncore_pmu_event_init;
+}
+
 static int
 uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader, bool dogrp)
 {
@@ -290,13 +301,18 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader, b
 		return -EINVAL;
 
 	n = box->n_events;
-	box->event_list[n] = leader;
-	n++;
+
+	if (is_uncore_event(leader)) {
+		box->event_list[n] = leader;
+		n++;
+	}
+
 	if (!dogrp)
 		return n;
 
 	list_for_each_entry(event, &leader->sibling_list, group_entry) {
-		if (event->state <= PERF_EVENT_STATE_OFF)
+		if (!is_uncore_event(event) ||
+		    event->state <= PERF_EVENT_STATE_OFF)
 			continue;
 
 		if (n >= max_count)

[PATCH 2/2] perf: Fix events installation during moving group

[Jiri Olsa]

We allow PMU driver to change the cpu on which the event
should be installed to. This happened in patch:
  e2d37cd213dc perf: Allow the PMU driver to choose the CPU on which to install events

This patch also forces all the group members to follow
the currently opened events cpu if the group happened
to be moved.

This and the change of event->cpu in perf_install_in_context
function introduced in:
  0cda4c023132 perf: Introduce perf_pmu_migrate_context()

forces group members to change their event->cpu,
if the currently-opened-event's PMU changed the cpu
and there is a group move.

Above behaviour causes problem for breakpoint events,
which uses event->cpu to touch cpu specific data for
breakpoints accounting. By changing event->cpu, some
breakpoints slots were wrongly accounted for given
cpu.

Vinces's perf fuzzer hit this issue and caused following
WARN on my setup:

[ 7113.758779] WARNING: CPU: 0 PID: 20214 at arch/x86/kernel/hw_breakpoint.c:119 arch_install_hw_breakpoint+0x142/0x150()
[ 7113.759262] Can't find any breakpoint slot
[ 7113.759433] Modules linked in:
[ 7113.759433] CPU: 0 PID: 20214 Comm: perf_fuzzer Not tainted 3.18.0 #31
[ 7113.759433] Hardware name: Intel Corporation Montevina platform/To be filled by O.E.M., BIOS AMVACRB1.86C.0066.B00.0805070703 05/07/2008
[ 7113.759433]  0000000000000009 ffff880050783ab8 ffffffff8157b70b 0000000000000004
[ 7113.759433]  ffff880050783b08 ffff880050783af8 ffffffff8104b771 0000000000000021
[ 7113.759433]  0000000000000004 ffff88007a60a958 000000000000a960 ffff88002ef42000
[ 7113.759433] Call Trace:
[ 7113.759433]  [<ffffffff8157b70b>] dump_stack+0x4f/0x7c
[ 7113.759433]  [<ffffffff8104b771>] warn_slowpath_common+0x81/0xa0
[ 7113.759433]  [<ffffffff8104b7d6>] warn_slowpath_fmt+0x46/0x50
[ 7113.759433]  [<ffffffff8100b262>] arch_install_hw_breakpoint+0x142/0x150
[ 7113.759433]  [<ffffffff81117e48>] hw_breakpoint_add+0x48/0x50
[ 7113.759433]  [<ffffffff81111611>] event_sched_in.isra.81+0xa1/0x270
[ 7113.759433]  [<ffffffff81111892>] group_sched_in+0xb2/0x1d0
[ 7113.759433]  [<ffffffff81111b90>] ctx_sched_in+0x1e0/0x390
[ 7113.759433]  [<ffffffff81111da4>] perf_event_sched_in+0x64/0x90
[ 7113.759433]  [<ffffffff811126a1>] __perf_install_in_context+0x121/0x1c0
[ 7113.759433]  [<ffffffff8108b975>] ? mark_held_locks+0x75/0xa0
[ 7113.759433]  [<ffffffff8110e930>] ? task_clock_event_add+0x40/0x40
[ 7113.759433]  [<ffffffff8110e983>] remote_function+0x53/0x70
[ 7113.759433]  [<ffffffff810c0d43>] generic_exec_single+0x123/0x180
[ 7113.759433]  [<ffffffff8110e930>] ? task_clock_event_add+0x40/0x40
[ 7113.759433]  [<ffffffff810c0dfa>] smp_call_function_single+0x5a/0xc0
[ 7113.759433]  [<ffffffff8108ba9d>] ? trace_hardirqs_on_caller+0xfd/0x1c0
[ 7113.759433]  [<ffffffff8110bc44>] task_function_call+0x44/0x50
[ 7113.759433]  [<ffffffff81112580>] ? perf_cpu_hrtimer_handler+0x210/0x210
[ 7113.759433]  [<ffffffff8110e77b>] perf_install_in_context+0x8b/0x110
[ 7113.759433]  [<ffffffff811161eb>] SyS_perf_event_open+0x59b/0xcc0
[ 7113.759433]  [<ffffffff811361d5>] ? vm_mmap_pgoff+0x75/0xa0
[ 7113.759433]  [<ffffffff81584162>] tracesys_phase2+0xd4/0xd9
[ 7113.759433] ---[ end trace 1a0d82aa412e33cf ]---

This patch changes the group moving code to keep events
original cpus.

Reported-by: Vince Weaver <vince@deater.net>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Vince Weaver <vince@deater.net>
Cc: Yan, Zheng <zheng.z.yan@intel.com>
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 kernel/events/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 3e19d3ebc29c..af0a5ba4e21d 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7477,11 +7477,11 @@ SYSCALL_DEFINE5(perf_event_open,
 
 	if (move_group) {
 		synchronize_rcu();
-		perf_install_in_context(ctx, group_leader, event->cpu);
+		perf_install_in_context(ctx, group_leader, group_leader->cpu);
 		get_ctx(ctx);
 		list_for_each_entry(sibling, &group_leader->sibling_list,
 				    group_entry) {
-			perf_install_in_context(ctx, sibling, event->cpu);
+			perf_install_in_context(ctx, sibling, sibling->cpu);
 			get_ctx(ctx);
 		}
 	}
-- 
1.9.3

[tip:perf/urgent] perf: Fix events installation during moving group

[tip-bot for Jiri Olsa]

Commit-ID:  9fc81d87420d0d3fd62d5e5529972c0ad9eab9cc
Gitweb:     http://git.kernel.org/tip/9fc81d87420d0d3fd62d5e5529972c0ad9eab9cc
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Wed, 10 Dec 2014 21:23:51 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 11 Dec 2014 11:24:15 +0100

perf: Fix events installation during moving group

We allow PMU driver to change the cpu on which the event
should be installed to. This happened in patch:

  e2d37cd213dc ("perf: Allow the PMU driver to choose the CPU on which to install events")

This patch also forces all the group members to follow
the currently opened events cpu if the group happened
to be moved.

This and the change of event->cpu in perf_install_in_context()
function introduced in:

  0cda4c023132 ("perf: Introduce perf_pmu_migrate_context()")

forces group members to change their event->cpu,
if the currently-opened-event's PMU changed the cpu
and there is a group move.

Above behaviour causes problem for breakpoint events,
which uses event->cpu to touch cpu specific data for
breakpoints accounting. By changing event->cpu, some
breakpoints slots were wrongly accounted for given
cpu.

Vinces's perf fuzzer hit this issue and caused following
WARN on my setup:

   WARNING: CPU: 0 PID: 20214 at arch/x86/kernel/hw_breakpoint.c:119 arch_install_hw_breakpoint+0x142/0x150()
   Can't find any breakpoint slot
   [...]

This patch changes the group moving code to keep the event's
original cpu.

Reported-by: Vince Weaver <vince@deater.net>
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Vince Weaver <vince@deater.net>
Cc: Yan, Zheng <zheng.z.yan@intel.com>
Cc: <stable@vger.kernel.org>
Link: http://lkml.kernel.org/r/1418243031-20367-3-git-send-email-jolsa@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 1cd5eef..2ab0238 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7435,11 +7435,11 @@ SYSCALL_DEFINE5(perf_event_open,
 
 	if (move_group) {
 		synchronize_rcu();
-		perf_install_in_context(ctx, group_leader, event->cpu);
+		perf_install_in_context(ctx, group_leader, group_leader->cpu);
 		get_ctx(ctx);
 		list_for_each_entry(sibling, &group_leader->sibling_list,
 				    group_entry) {
-			perf_install_in_context(ctx, sibling, event->cpu);
+			perf_install_in_context(ctx, sibling, sibling->cpu);
 			get_ctx(ctx);
 		}
 	}

Re: [PATCHSET/RFC 0/2] perf: Fix issues reported by perf fuzzer

[Ingo Molnar]

* Jiri Olsa <jolsa@kernel.org> wrote:

> hi,
> Vince reported several issues with perf code hit by perf fuzzer,
> in here:
>   http://marc.info/?l=linux-kernel&m=141806390822670&w=2
> 
> I run the fuzzer, but wasn't able to hit issues reported
> by Vince. But.. I was lucky enough to hit other 2 issues
> described in following patches ;-)
> 
> As I dont follow the uncore code much, it is sort of RFC
> patchset. However I'm running the fuzzer now for several
> hours and can't hit any other issue.

The changes are looking good to me so I've applied them to 
tip:perf/urgent to give them more testing.

If they break anything in the uncore code's expectations the 
uncore people should chime in. In any case, stability comes 
first.

Thanks,

	Ingo