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

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

[Jiri Olsa]

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 <jolsa@redhat.com>
---
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index efca2685d876..a81b5a39b79e 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -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;

Re: [PATCH] perf/x86/uncore: Allow single pmu/box within events group

[Peter Zijlstra]

On Fri, Nov 18, 2016 at 01:15:28AM +0100, Jiri Olsa wrote:
> 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.

Re: [PATCH] perf/x86/uncore: Allow single pmu/box within events group

[Jiri Olsa]

On Fri, Nov 18, 2016 at 12:28:50PM +0100, Peter Zijlstra wrote:
> On Fri, Nov 18, 2016 at 01:15:28AM +0100, Jiri Olsa wrote:
> > 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.

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

jirka

Re: [PATCH] perf/x86/uncore: Allow single pmu/box within events group

[Peter Zijlstra]

On Fri, Nov 18, 2016 at 01:33:25PM +0100, Jiri Olsa wrote:
> On Fri, Nov 18, 2016 at 12:28:50PM +0100, Peter Zijlstra wrote:
> > On Fri, Nov 18, 2016 at 01:15:28AM +0100, Jiri Olsa wrote:
> > > 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.
> 
> 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(-)

diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index efca2685d876..7b1b34576886 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -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;
 

Re: [PATCH] perf/x86/uncore: Allow single pmu/box within events group

[Jiri Olsa]

On Fri, Nov 18, 2016 at 01:53:54PM +0100, Peter Zijlstra wrote:
> On Fri, Nov 18, 2016 at 01:33:25PM +0100, Jiri Olsa wrote:
> > On Fri, Nov 18, 2016 at 12:28:50PM +0100, Peter Zijlstra wrote:
> > > On Fri, Nov 18, 2016 at 01:15:28AM +0100, Jiri Olsa wrote:
> > > > 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.
> > 
> > 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

Re: [PATCH] perf/x86/uncore: Allow single pmu/box within events group

[Jiri Olsa]

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

SNIP

> ---
>  arch/x86/events/intel/uncore.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index efca2685d876..7b1b34576886 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -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 <jolsa@redhat.com>

thanks,
jirka

>  }
>  
>  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;
>  

Re: [PATCH] perf/x86/uncore: Allow single pmu/box within events group

[Peter Zijlstra]

On Fri, Nov 18, 2016 at 02:48:59PM +0100, Jiri Olsa wrote:
> On Fri, Nov 18, 2016 at 01:53:54PM +0100, Peter Zijlstra wrote:
> 
> SNIP
> 
> > ---
> >  arch/x86/events/intel/uncore.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> > index efca2685d876..7b1b34576886 100644
> > --- a/arch/x86/events/intel/uncore.c
> > +++ b/arch/x86/events/intel/uncore.c
> > @@ -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.. ;-)

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

Wouldn't:

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

be the safer option?

Re: [PATCH] perf/x86/uncore: Allow single pmu/box within events group

[Jiri Olsa]

On Fri, Nov 18, 2016 at 03:06:05PM +0100, Peter Zijlstra wrote:
> On Fri, Nov 18, 2016 at 02:48:59PM +0100, Jiri Olsa wrote:
> > On Fri, Nov 18, 2016 at 01:53:54PM +0100, Peter Zijlstra wrote:
> > 
> > SNIP
> > 
> > > ---
> > >  arch/x86/events/intel/uncore.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> > > index efca2685d876..7b1b34576886 100644
> > > --- a/arch/x86/events/intel/uncore.c
> > > +++ b/arch/x86/events/intel/uncore.c
> > > @@ -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.. ;-)
> 
> 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

Re: [PATCH] perf/x86/uncore: Allow single pmu/box within events group

[Peter Zijlstra]

On Fri, Nov 18, 2016 at 03:30:48PM +0100, Jiri Olsa wrote:
> > > > +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?
> > 
> > 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:perf/urgent] perf/x86/intel/uncore: Allow only a single PMU/box within an events group

[tip-bot for Peter Zijlstra]

Commit-ID:  033ac60c7f21f9996a0fab2fd04f334afbf77b33
Gitweb:     http://git.kernel.org/tip/033ac60c7f21f9996a0fab2fd04f334afbf77b33
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Fri, 18 Nov 2016 13:53:54 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
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 <jolsa@redhat.com>
Tested-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vince@deater.net>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Link: http://lkml.kernel.org/r/20161118125354.GQ3117@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/events/intel/uncore.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index efca268..dbaaf7dc 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -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;