perf/x86/uncore: Allow single pmu/box within events group

PATCH In review Jiri Olsa 3 participants 10 messages Nov 18, 2016 – Nov 22, 2016

Subsystems (2)

x86 perf events

lore

Share & tools

lore

Jiri Olsa

author

2016-11-18 00:15 UTC #0 lore raw

PATCH 1/1 perf/x86/uncore: Allow single pmu/box within events group

hi,
I was regularly hitting bug in uncore_validate_group when running fuzzer. Attached patch makes it go away.

The problem is that in uncore_validate_group we could access uncore boxes regs data with event's regs data which was initialized for another box. Attached patch makes the check fail when it detects wrong box.

Before I realized that the generic code won't allow for more pmu/box contexts within events group, I reworked uncore_validate_group logic to allow that, it's in here:
https://git.kernel.org/cgit/linux/kernel/git/jolsa/perf.git/log/?h=perf/uncore_fix

I'm not sure wether it's useful to allow different uncore boxes within events group, but it should not be problem to enable it.

Also I still don't understand how can we touch group sibling events within event_init callback.. I must be missing something, because fuzzer does not complain about this ;-)

thanks,
jirka

---
Current uncore_validate_group code expects all events within the group to have same pmu.

This leads to constraint code using wrong boxes which leads in my case to touching uninitialized spinlocks, but could be probably worse.. depends on type and box details.

I get lockdep warning below for following perf stat:

  # perf stat -vv -e '{uncore_cbox_0/config=0x0334/,uncore_qpi_0/event=1/}' -a sleep 1

INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.

CPU: 12 PID: 3727 Comm: perf_fuzzer Tainted: G        W       4.9.0-rc1+ #10
Hardware name: IBM System x3650 M4 : -[7915E2G]-/00Y7683, BIOS -[VVE124AUS-1.30]- 11/21/2012
 ffffc90002ca3ae0 ffffffff81399565 0000000000000000 0000000000000000
 ffffc90002ca3b50 ffffffff810d0a3a 0000000000000006 ffff880276285eb8
 ffffc90002ca3bc8 ffff8802711a8b98 ffff8802711a8000 ffff8802711a8b98
Call Trace:
 [<ffffffff81399565>] dump_stack+0x68/0x93
 [<ffffffff810d0a3a>] register_lock_class+0x50a/0x510
 [<ffffffff810d2d78>] __lock_acquire+0x88/0x12a0
 [<ffffffff811a2cfc>] ? ___perf_sw_event+0x16c/0x280
 [<ffffffff810f091d>] ? debug_lockdep_rcu_enabled+0x1d/0x20
 [<ffffffff810d43b9>] lock_acquire+0xe9/0x1d0
 [<ffffffffa0186679>] ? uncore_get_constraint+0x59/0x100 [intel_uncore]
 [<ffffffff817075e3>] _raw_spin_lock_irqsave+0x43/0x60
 [<ffffffffa0186679>] ? uncore_get_constraint+0x59/0x100 [intel_uncore]
 [<ffffffffa0186679>] uncore_get_constraint+0x59/0x100 [intel_uncore]
 [<ffffffffa01853fd>] uncore_assign_events+0x6d/0x250 [intel_uncore]
 [<ffffffffa0185df7>] uncore_pmu_event_init+0x187/0x220 [intel_uncore]
 [<ffffffff81197443>] perf_try_init_event+0x43/0x90
 [<ffffffff8119b65e>] perf_event_alloc+0x55e/0xc50
 [<ffffffff8119b568>] ? perf_event_alloc+0x468/0xc50
 [<ffffffff8119f7db>] SYSC_perf_event_open+0x40b/0xf90
 [<ffffffff811a2f69>] SyS_perf_event_open+0x9/0x10
 [<ffffffff81002eb6>] do_syscall_64+0x66/0x1d0
 [<ffffffff81707f24>] entry_SYSCALL64_slow_path+0x25/0x25

The reason is that leader event will get extra reg configuration which touches regs of second event's box.

This patch makes the uncore_validate_group check fail when it detects wrong box.

Signed-off-by Jiri Olsa <[email protected]>

---

arch/x86/events/intel/uncore.c +4 -0

index efca2685d876..a81b5a39b79e 100644
@@ -597,6 +597,10 @@ static int uncore_validate_group(struct intel_uncore_pmu *pmu,
 	struct intel_uncore_box *fake_box;
 	int ret = -EINVAL, n;
 
+	/* All events in group must be from the same uncore box. */
+	if (leader->pmu->type != pmu->pmu.type)
+		return -EINVAL;
+
 	fake_box = uncore_alloc_box(pmu->type, NUMA_NO_NODE);
 	if (!fake_box)
 		return -ENOMEM;

Peter Zijlstra

maintainer

2016-11-18 11:28 UTC #1 lore raw

Quoted 9 lines · Jiri Olsa

Current uncore_validate_group code expects all events within the group to have same pmu.
This leads to constraint code using wrong boxes which leads in my case to touching uninitialized spinlocks, but could be probably worse.. depends on type and box details.
I get lockdep warning below for following perf stat:
  # perf stat -vv -e '{uncore_cbox_0/config=0x0334/,uncore_qpi_0/event=1/}' -a sleep 1

Hurm, we shouldn't be allowing that in the first place I think.

Let me stare at the generic group code, the intent was to only allow software events to mix with hw events, nothing else.

Jiri Olsa

author

2016-11-18 12:33 UTC #2 lore raw

Quoted 8 lines · Peter Zijlstra

Quoted 7 lines · Jiri Olsa
Current uncore_validate_group code expects all events within the group to have same pmu.
This leads to constraint code using wrong boxes which leads in my case to touching uninitialized spinlocks, but could be probably worse.. depends on type and box details.

Quoted 8 lines

Quoted 2 lines
I get lockdep warning below for following perf stat:
  # perf stat -vv -e '{uncore_cbox_0/config=0x0334/,uncore_qpi_0/event=1/}' -a sleep 1
Hurm, we shouldn't be allowing that in the first place I think.
Let me stare at the generic group code, the intent was to only allow software events to mix with hw events, nothing else.

yep, that's what's happening now.. but after the event_init callback

jirka

Peter Zijlstra

maintainer

2016-11-18 12:54 UTC #3 lore raw

Quoted 12 lines · Jiri Olsa

Quoted 11 lines · Peter Zijlstra
Quoted 9 lines · Jiri Olsa
Current uncore_validate_group code expects all events within the group to have same pmu.
This leads to constraint code using wrong boxes which leads in my case to touching uninitialized spinlocks, but could be probably worse.. depends on type and box details.
I get lockdep warning below for following perf stat:
  # perf stat -vv -e '{uncore_cbox_0/config=0x0334/,uncore_qpi_0/event=1/}' -a sleep 1

Quoted 7 lines

Quoted 5 lines
Hurm, we shouldn't be allowing that in the first place I think.
Let me stare at the generic group code, the intent was to only allow software events to mix with hw events, nothing else.
yep, that's what's happening now.. but after the event_init callback

Ah yes indeed. Its the is_uncore_event() test in uncore_collect_event() that's too lenient, that allows us to mix events from various uncore boxes.

Would something like so fix things too? Because that is the point of is_uncore_event() in collect(), to only collect events for _that_ pmu.

---
 arch/x86/events/intel/uncore.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

arch/x86/events/intel/uncore.c +4 -4

index efca2685d876..7b1b34576886 100644
@@ -319,9 +319,9 @@ static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,
  */
 static int uncore_pmu_event_init(struct perf_event *event);
 
-static bool is_uncore_event(struct perf_event *event)
+static bool is_box_event(struct intel_uncore_box *box, struct perf_event *event)
 {
-	return event->pmu->event_init == uncore_pmu_event_init;
+	return box->pmu == event->pmu;
 }
 
 static int

@@ -340,7 +340,7 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader,
 
 	n = box->n_events;
 
-	if (is_uncore_event(leader)) {
+	if (is_box_event(box, leader)) {
 		box->event_list[n] = leader;
 		n++;
 	}

@@ -349,7 +349,7 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader,
 		return n;
 
 	list_for_each_entry(event, &leader->sibling_list, group_entry) {
-		if (!is_uncore_event(event) ||
+		if (!is_box_event(box, event) ||
 		    event->state <= PERF_EVENT_STATE_OFF)
 			continue;

Jiri Olsa

author

2016-11-18 13:25 UTC #4 lore raw

Quoted 19 lines · Peter Zijlstra

Quoted 18 lines · Jiri Olsa
Quoted 16 lines · Peter Zijlstra
Quoted 9 lines · Jiri Olsa
Current uncore_validate_group code expects all events within the group to have same pmu.
This leads to constraint code using wrong boxes which leads in my case to touching uninitialized spinlocks, but could be probably worse.. depends on type and box details.
I get lockdep warning below for following perf stat:
  # perf stat -vv -e '{uncore_cbox_0/config=0x0334/,uncore_qpi_0/event=1/}' -a sleep 1
Hurm, we shouldn't be allowing that in the first place I think.
Let me stare at the generic group code, the intent was to only allow software events to mix with hw events, nothing else.

Quoted 8 lines

Quoted 1 line
yep, that's what's happening now.. but after the event_init callback
Ah yes indeed. Its the is_uncore_event() test in uncore_collect_event() that's too lenient, that allows us to mix events from various uncore boxes.
Would something like so fix things too? Because that is the point of is_uncore_event() in collect(), to only collect events for _that_ pmu.

that looks ok.. I'll run the test

jirka

Jiri Olsa

author Tested-by

2016-11-18 13:49 UTC #5 lore raw

In reply to Peter Zijlstra

On Fri, Nov 18, 2016 at 01:53:54PM +0100, Peter Zijlstra wrote:

SNIP

Quoted 9 lines

---
 arch/x86/events/intel/uncore.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

arch/x86/events/intel/uncore.c +0 -0

index efca2685d876..7b1b34576886 100644

arch/x86/events/intel/uncore.c static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type, · line 324

Quoted 8 lines

arch/x86/events/intel/uncore.c +2 -2

@@ -319,9 +319,9 @@ static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,
  */
 static int uncore_pmu_event_init(struct perf_event *event);
 
-static bool is_uncore_event(struct perf_event *event)
+static bool is_box_event(struct intel_uncore_box *box, struct perf_event *event)
 {
-	return event->pmu->event_init == uncore_pmu_event_init;
+	return box->pmu == event->pmu;

this one needs to be:

+ return box->pmu == uncore_event_to_pmu(event);

and it works.. ;-)

Tested-by Jiri Olsa <[email protected]>

thanks,
jirka

Quoted 21 lines

arch/x86/events/intel/uncore.c +2 -2

 }
 
 static int

@@ -340,7 +340,7 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader,
 
 	n = box->n_events;
 
-	if (is_uncore_event(leader)) {
+	if (is_box_event(box, leader)) {
 		box->event_list[n] = leader;
 		n++;
 	}

@@ -349,7 +349,7 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader,
 		return n;
 
 	list_for_each_entry(event, &leader->sibling_list, group_entry) {
-		if (!is_uncore_event(event) ||
+		if (!is_box_event(box, event) ||
 		    event->state <= PERF_EVENT_STATE_OFF)
 			continue;

Peter Zijlstra

maintainer

2016-11-18 14:06 UTC #6 lore raw

Quoted 24 lines · Jiri Olsa

On Fri, Nov 18, 2016 at 01:53:54PM +0100, Peter Zijlstra wrote:
SNIP
Quoted 9 lines
---
 arch/x86/events/intel/uncore.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/x86/events/intel/uncore.c +0 -0
index efca2685d876..7b1b34576886 100644
arch/x86/events/intel/uncore.c static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type, · line 324
Quoted 8 lines
arch/x86/events/intel/uncore.c +2 -2
@@ -319,9 +319,9 @@ static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,
  */
 static int uncore_pmu_event_init(struct perf_event *event);
 
-static bool is_uncore_event(struct perf_event *event)
+static bool is_box_event(struct intel_uncore_box *box, struct perf_event *event)
 {
-	return event->pmu->event_init == uncore_pmu_event_init;
+	return box->pmu == event->pmu;
this one needs to be:

Quoted 3 lines

+ return box->pmu == uncore_event_to_pmu(event);
and it works.. ;-)

Will that not explode if we fudge a software event in there?

Wouldn't:

	return box->pmu.pmu == event->pmu;

be the safer option?

Jiri Olsa

author

2016-11-18 14:30 UTC #7 lore raw

Quoted 29 lines · Peter Zijlstra

Quoted 24 lines · Jiri Olsa
On Fri, Nov 18, 2016 at 01:53:54PM +0100, Peter Zijlstra wrote:
SNIP
Quoted 9 lines
---
 arch/x86/events/intel/uncore.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/x86/events/intel/uncore.c +0 -0
index efca2685d876..7b1b34576886 100644
arch/x86/events/intel/uncore.c static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type, · line 324
Quoted 8 lines
arch/x86/events/intel/uncore.c +2 -2
@@ -319,9 +319,9 @@ static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,
  */
 static int uncore_pmu_event_init(struct perf_event *event);
 
-static bool is_uncore_event(struct perf_event *event)
+static bool is_box_event(struct intel_uncore_box *box, struct perf_event *event)
 {
-	return event->pmu->event_init == uncore_pmu_event_init;
+	return box->pmu == event->pmu;
this one needs to be:
Quoted 3 lines
+ return box->pmu == uncore_event_to_pmu(event);
and it works.. ;-)

Quoted 7 lines

Will that not explode if we fudge a software event in there?
Wouldn't:
	return box->pmu.pmu == event->pmu;
be the safer option?

hum right.. but for some reason I can't crash it nor even fuzzer complains

jirka

Peter Zijlstra

maintainer

2016-11-18 14:45 UTC #8 lore raw

Quoted 13 lines · Jiri Olsa

Quoted 13 lines
Quoted 10 lines
arch/x86/events/intel/uncore.c static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type, · line 324
Quoted 4 lines
arch/x86/events/intel/uncore.c +2 -1
+static bool is_box_event(struct intel_uncore_box *box, struct perf_event *event)
 {
-	return event->pmu->event_init == uncore_pmu_event_init;
+	return box->pmu == event->pmu;
this one needs to be:
+ return box->pmu == uncore_event_to_pmu(event);
and it works.. ;-)
Will that not explode if we fudge a software event in there?

Quoted 7 lines

Quoted 5 lines
Wouldn't:
	return box->pmu.pmu == event->pmu;
be the safer option?
hum right.. but for some reason I can't crash it nor even fuzzer complains

Its because pmu is the first member, so the pointer is the exact same, just the type changes. But that is a 'happy' accident of implementation.

tip-bot for Peter Zijlstra

merge bot

2016-11-22 12:31 UTC #9 lore raw

[tip:perf/urgent] perf/x86/intel/uncore: Allow only a single PMU/box within an events group

In reply to Peter Zijlstra

perf/x86/intel/uncore: Allow only a single PMU/box within an events group

Commit-ID:  033ac60c7f21f9996a0fab2fd04f334afbf77b33
Gitweb:     http://git.kernel.org/tip/033ac60c7f21f9996a0fab2fd04f334afbf77b33
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 18 Nov 2016 13:53:54 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 22 Nov 2016 12:36:59 +0100

perf/x86/intel/uncore: Allow only a single PMU/box within an events group

Group validation expects all events to be of the same PMU; however is_uncore_pmu() is too wide, it matches _all_ uncore events, even across PMUs.

This triggers failure when we group different events from different uncore PMUs, like:

perf stat -vv -e '{uncore_cbox_0/config=0x0334/,uncore_qpi_0/event=1/}' -a sleep 1

Fix is_uncore_pmu() by only matching events to the box at hand.

Note that generic code; ran after this step; will disallow this mixture of PMU events.

Reported-by Jiri Olsa <[email protected]>

Tested-by Jiri Olsa <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Kan Liang <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Vince Weaver <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/events/intel/uncore.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

arch/x86/events/intel/uncore.c +4 -4

index efca268..dbaaf7dc 100644
@@ -319,9 +319,9 @@ static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,
  */
 static int uncore_pmu_event_init(struct perf_event *event);
 
-static bool is_uncore_event(struct perf_event *event)
+static bool is_box_event(struct intel_uncore_box *box, struct perf_event *event)
 {
-	return event->pmu->event_init == uncore_pmu_event_init;
+	return &box->pmu->pmu == event->pmu;
 }
 
 static int

@@ -340,7 +340,7 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader,
 
 	n = box->n_events;
 
-	if (is_uncore_event(leader)) {
+	if (is_box_event(box, leader)) {
 		box->event_list[n] = leader;
 		n++;
 	}

@@ -349,7 +349,7 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader,
 		return n;
 
 	list_for_each_entry(event, &leader->sibling_list, group_entry) {
-		if (!is_uncore_event(event) ||
+		if (!is_box_event(box, event) ||
 		    event->state <= PERF_EVENT_STATE_OFF)
 			continue;