[PATCH 0/3] [4.0] arm/arm64: Do not group hardware events from different PMUs

[PATCH 0/3] [4.0] arm/arm64: Do not group hardware events from different PMUs

[Suzuki K. Poulose]

From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>

This is a collection of fixes which denies grouping hardware events
from different PMUs. They also fix crashes triggered by perf_fuzzer
on Linux-4.0-rc2.

Pawel,

Similar fix is required in ARM CCN PMU driver. I didn't find it
straight forward to fix it there. Could you please take a look
into it ?


Suzuki K. Poulose (3):
  arm/pmu: Reject groups spanning multiple hardware PMUs
  arm64/pmu: Reject groups spanning multiple HW PMUs
  arm-cci: Reject groups spanning multiple HW PMUs

 arch/arm/kernel/perf_event.c   |   21 +++++++++++++++------
 arch/arm64/kernel/perf_event.c |   21 +++++++++++++++------
 drivers/bus/arm-cci.c          |   19 ++++++++++++++-----
 3 files changed, 44 insertions(+), 17 deletions(-)

-- 
1.7.9.5

[PATCH 0/3] [4.0] arm/arm64: Do not group hardware events from different PMUs

[Suzuki K. Poulose]

From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>

This is a collection of fixes which denies grouping hardware events
from different PMUs. They also fix crashes triggered by perf_fuzzer
on Linux-4.0-rc2.

Pawel,

Similar fix is required in ARM CCN PMU driver. I didn't find it
straight forward to fix it there. Could you please take a look
into it ?


Suzuki K. Poulose (3):
  arm/pmu: Reject groups spanning multiple hardware PMUs
  arm64/pmu: Reject groups spanning multiple HW PMUs
  arm-cci: Reject groups spanning multiple HW PMUs

 arch/arm/kernel/perf_event.c   |   21 +++++++++++++++------
 arch/arm64/kernel/perf_event.c |   21 +++++++++++++++------
 drivers/bus/arm-cci.c          |   19 ++++++++++++++-----
 3 files changed, 44 insertions(+), 17 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Suzuki K. Poulose]

From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>

Don't allow grouping hardware events from different PMUs
 (eg. CCI + CPU).

Fixes a crash triggered by perf_fuzzer on Linux-4.0-rc2,
with CCI PMU turned on. The validate_event(), after certain checks,
assumes that the given hardware pmu event belongs to armpmu,
which may not be true always, with other hardware PMUs
around (CCI, CCN).

 ---
CPU: 0 PID: 1527 Comm: perf_fuzzer Not tainted 4.0.0-rc2 #57
Hardware name: ARM-Versatile Express
task: bd8484c0 ti: be676000 task.ti: be676000
PC is at 0xbf1bbc90
LR is at validate_event+0x34/0x5c
pc : [<bf1bbc90>]    lr : [<80016060>]    psr: 00000013
...
[<80016060>] (validate_event) from [<80016198>] (validate_group+0x28/0x90)
[<80016198>] (validate_group) from [<80016398>] (armpmu_event_init+0x150/0x218)
[<80016398>] (armpmu_event_init) from [<800882e4>] (perf_try_init_event+0x30/0x48)
[<800882e4>] (perf_try_init_event) from [<8008f544>] (perf_init_event+0x5c/0xf4)
[<8008f544>] (perf_init_event) from [<8008f8a8>] (perf_event_alloc+0x2cc/0x35c)
[<8008f8a8>] (perf_event_alloc) from [<8009015c>] (SyS_perf_event_open+0x498/0xa70)
[<8009015c>] (SyS_perf_event_open) from [<8000e420>] (ret_fast_syscall+0x0/0x34)
Code: bf1be000 bf1bb380 802a2664 00000000 (00000002)
---[ end trace 01aff0ff00926a0a ]---

Also cleans up the code to use the arm_pmu only when we know that
we are dealing with an arm pmu event.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm/kernel/perf_event.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 557e128..4a86a01 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -259,20 +259,29 @@ out:
 }
 
 static int
-validate_event(struct pmu_hw_events *hw_events,
-	       struct perf_event *event)
+validate_event(struct pmu *pmu, struct pmu_hw_events *hw_events,
+			       struct perf_event *event)
 {
-	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+	struct arm_pmu *armpmu;
 
 	if (is_software_event(event))
 		return 1;
 
+	/*
+	 * Reject groups spanning multiple HW PMUs (e.g. CPU + CCI). The
+	 * core perf code won't check that the pmu->ctx == leader->ctx
+	 * until after pmu->event_init(event).
+	 */
+	if (event->pmu != pmu)
+		return 0;
+
 	if (event->state < PERF_EVENT_STATE_OFF)
 		return 1;
 
 	if (event->state == PERF_EVENT_STATE_OFF && !event->attr.enable_on_exec)
 		return 1;
 
+	armpmu = to_arm_pmu(event->pmu);
 	return armpmu->get_event_idx(hw_events, event) >= 0;
 }
 
@@ -288,15 +297,15 @@ validate_group(struct perf_event *event)
 	 */
 	memset(&fake_pmu.used_mask, 0, sizeof(fake_pmu.used_mask));
 
-	if (!validate_event(&fake_pmu, leader))
+	if (!validate_event(event->pmu, &fake_pmu, leader))
 		return -EINVAL;
 
 	list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
-		if (!validate_event(&fake_pmu, sibling))
+		if (!validate_event(event->pmu, &fake_pmu, sibling))
 			return -EINVAL;
 	}
 
-	if (!validate_event(&fake_pmu, event))
+	if (!validate_event(event->pmu, &fake_pmu, event))
 		return -EINVAL;
 
 	return 0;
-- 
1.7.9.5

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Suzuki K. Poulose]

From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>

Don't allow grouping hardware events from different PMUs
 (eg. CCI + CPU).

Fixes a crash triggered by perf_fuzzer on Linux-4.0-rc2,
with CCI PMU turned on. The validate_event(), after certain checks,
assumes that the given hardware pmu event belongs to armpmu,
which may not be true always, with other hardware PMUs
around (CCI, CCN).

 ---
CPU: 0 PID: 1527 Comm: perf_fuzzer Not tainted 4.0.0-rc2 #57
Hardware name: ARM-Versatile Express
task: bd8484c0 ti: be676000 task.ti: be676000
PC is at 0xbf1bbc90
LR is at validate_event+0x34/0x5c
pc : [<bf1bbc90>]    lr : [<80016060>]    psr: 00000013
...
[<80016060>] (validate_event) from [<80016198>] (validate_group+0x28/0x90)
[<80016198>] (validate_group) from [<80016398>] (armpmu_event_init+0x150/0x218)
[<80016398>] (armpmu_event_init) from [<800882e4>] (perf_try_init_event+0x30/0x48)
[<800882e4>] (perf_try_init_event) from [<8008f544>] (perf_init_event+0x5c/0xf4)
[<8008f544>] (perf_init_event) from [<8008f8a8>] (perf_event_alloc+0x2cc/0x35c)
[<8008f8a8>] (perf_event_alloc) from [<8009015c>] (SyS_perf_event_open+0x498/0xa70)
[<8009015c>] (SyS_perf_event_open) from [<8000e420>] (ret_fast_syscall+0x0/0x34)
Code: bf1be000 bf1bb380 802a2664 00000000 (00000002)
---[ end trace 01aff0ff00926a0a ]---

Also cleans up the code to use the arm_pmu only when we know that
we are dealing with an arm pmu event.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm/kernel/perf_event.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 557e128..4a86a01 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -259,20 +259,29 @@ out:
 }
 
 static int
-validate_event(struct pmu_hw_events *hw_events,
-	       struct perf_event *event)
+validate_event(struct pmu *pmu, struct pmu_hw_events *hw_events,
+			       struct perf_event *event)
 {
-	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+	struct arm_pmu *armpmu;
 
 	if (is_software_event(event))
 		return 1;
 
+	/*
+	 * Reject groups spanning multiple HW PMUs (e.g. CPU + CCI). The
+	 * core perf code won't check that the pmu->ctx == leader->ctx
+	 * until after pmu->event_init(event).
+	 */
+	if (event->pmu != pmu)
+		return 0;
+
 	if (event->state < PERF_EVENT_STATE_OFF)
 		return 1;
 
 	if (event->state == PERF_EVENT_STATE_OFF && !event->attr.enable_on_exec)
 		return 1;
 
+	armpmu = to_arm_pmu(event->pmu);
 	return armpmu->get_event_idx(hw_events, event) >= 0;
 }
 
@@ -288,15 +297,15 @@ validate_group(struct perf_event *event)
 	 */
 	memset(&fake_pmu.used_mask, 0, sizeof(fake_pmu.used_mask));
 
-	if (!validate_event(&fake_pmu, leader))
+	if (!validate_event(event->pmu, &fake_pmu, leader))
 		return -EINVAL;
 
 	list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
-		if (!validate_event(&fake_pmu, sibling))
+		if (!validate_event(event->pmu, &fake_pmu, sibling))
 			return -EINVAL;
 	}
 
-	if (!validate_event(&fake_pmu, event))
+	if (!validate_event(event->pmu, &fake_pmu, event))
 		return -EINVAL;
 
 	return 0;
-- 
1.7.9.5

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Peter Zijlstra]

On Mon, Mar 09, 2015 at 12:46:30PM +0000, Suzuki K. Poulose wrote:
> From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
> 
> Don't allow grouping hardware events from different PMUs
>  (eg. CCI + CPU).

Uhm, how does this work? If we have multiple hardware PMUs we'll stop
scheduling events after the first failed event schedule. This can leave
one of the PMUs severely under utilized.

Re: [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Peter Zijlstra]

On Mon, Mar 09, 2015 at 12:46:30PM +0000, Suzuki K. Poulose wrote:
> From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
> 
> Don't allow grouping hardware events from different PMUs
>  (eg. CCI + CPU).

Uhm, how does this work? If we have multiple hardware PMUs we'll stop
scheduling events after the first failed event schedule. This can leave
one of the PMUs severely under utilized.

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Suzuki K. Poulose]

On 10/03/15 11:27, Peter Zijlstra wrote:
> On Mon, Mar 09, 2015 at 12:46:30PM +0000, Suzuki K. Poulose wrote:
>> From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
>>
>> Don't allow grouping hardware events from different PMUs
>>   (eg. CCI + CPU).
>
> Uhm, how does this work? If we have multiple hardware PMUs we'll stop
> scheduling events after the first failed event schedule. This can leave
> one of the PMUs severely under utilized.
This is done from pmu->event_init(), where we haven't scheduled an
event yet. Do you think we need to solve it using a different approach
? What is the best way to handle this situation ? Is it OK
to allow different PMUs in the group ?

Suzuki
>

Re: [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Suzuki K. Poulose]

On 10/03/15 11:27, Peter Zijlstra wrote:
> On Mon, Mar 09, 2015 at 12:46:30PM +0000, Suzuki K. Poulose wrote:
>> From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
>>
>> Don't allow grouping hardware events from different PMUs
>>   (eg. CCI + CPU).
>
> Uhm, how does this work? If we have multiple hardware PMUs we'll stop
> scheduling events after the first failed event schedule. This can leave
> one of the PMUs severely under utilized.
This is done from pmu->event_init(), where we haven't scheduled an
event yet. Do you think we need to solve it using a different approach
? What is the best way to handle this situation ? Is it OK
to allow different PMUs in the group ?

Suzuki
>

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Mark Rutland]

On Tue, Mar 10, 2015 at 11:27:23AM +0000, Peter Zijlstra wrote:
> On Mon, Mar 09, 2015 at 12:46:30PM +0000, Suzuki K. Poulose wrote:
> > From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
> > 
> > Don't allow grouping hardware events from different PMUs
> >  (eg. CCI + CPU).
> 
> Uhm, how does this work? If we have multiple hardware PMUs we'll stop
> scheduling events after the first failed event schedule. This can leave
> one of the PMUs severely under utilized.

The problem is here group validation at pmu::event_init() time, not
scheduling.

We don't allow grouping across disparate HW PMUs because we can't
provide group semantics anyway. Scheduling is not a problem in this case
(unlike the big.LITTLE case I have a patch for [1]).

We have a CPU PMU and an "uncore" CCI PMU. You can't create task-bound
events for the CCI, but you can create CPU-bound events for the CCI on
the nominal CPU the CCI is monitored from.

The context check you added in c3c87e770458aa00 "perf: Tighten (and fix)
the grouping condition" implicitly rejects groups that have CPU and CCI
events (each event::ctx will be the relevant pmu::pmu_cpu_context and
will differ), and this is sane -- you can't provide group semantics
across disparate HW PMUs.

Unfortunately that happens after we've done the
event->pmu->event_init(event) dance on each event, and in our event_init
function we try to verify the group is sane. In our verification we
ignore SW events, but assume that all !SW events are for the CPU PMU.
If you add a CPU event to a CCI group, that's not the case, and we use
container_of on an unsuitable object, derefence garbage, invoke the
eschaton and so on.

It would be nicer if we could prevent this in the core so we're not
reliant on every PMU driver doing the same verification. My initial
thought was that seemed like unnecessary duplication of the ctx checking
above, but if we're going to end up shoving it into several drivers
anyway perhaps it's the lesser evil.

Mark.

[1] https://lkml.org/lkml/2015/2/5/520

Re: [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Mark Rutland]

On Tue, Mar 10, 2015 at 11:27:23AM +0000, Peter Zijlstra wrote:
> On Mon, Mar 09, 2015 at 12:46:30PM +0000, Suzuki K. Poulose wrote:
> > From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
> > 
> > Don't allow grouping hardware events from different PMUs
> >  (eg. CCI + CPU).
> 
> Uhm, how does this work? If we have multiple hardware PMUs we'll stop
> scheduling events after the first failed event schedule. This can leave
> one of the PMUs severely under utilized.

The problem is here group validation at pmu::event_init() time, not
scheduling.

We don't allow grouping across disparate HW PMUs because we can't
provide group semantics anyway. Scheduling is not a problem in this case
(unlike the big.LITTLE case I have a patch for [1]).

We have a CPU PMU and an "uncore" CCI PMU. You can't create task-bound
events for the CCI, but you can create CPU-bound events for the CCI on
the nominal CPU the CCI is monitored from.

The context check you added in c3c87e770458aa00 "perf: Tighten (and fix)
the grouping condition" implicitly rejects groups that have CPU and CCI
events (each event::ctx will be the relevant pmu::pmu_cpu_context and
will differ), and this is sane -- you can't provide group semantics
across disparate HW PMUs.

Unfortunately that happens after we've done the
event->pmu->event_init(event) dance on each event, and in our event_init
function we try to verify the group is sane. In our verification we
ignore SW events, but assume that all !SW events are for the CPU PMU.
If you add a CPU event to a CCI group, that's not the case, and we use
container_of on an unsuitable object, derefence garbage, invoke the
eschaton and so on.

It would be nicer if we could prevent this in the core so we're not
reliant on every PMU driver doing the same verification. My initial
thought was that seemed like unnecessary duplication of the ctx checking
above, but if we're going to end up shoving it into several drivers
anyway perhaps it's the lesser evil.

Mark.

[1] https://lkml.org/lkml/2015/2/5/520

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Peter Zijlstra]

On Tue, Mar 10, 2015 at 12:05:21PM +0000, Mark Rutland wrote:
> On Tue, Mar 10, 2015 at 11:27:23AM +0000, Peter Zijlstra wrote:
> > On Mon, Mar 09, 2015 at 12:46:30PM +0000, Suzuki K. Poulose wrote:
> > > From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
> > > 
> > > Don't allow grouping hardware events from different PMUs
> > >  (eg. CCI + CPU).
> > 
> > Uhm, how does this work? If we have multiple hardware PMUs we'll stop
> > scheduling events after the first failed event schedule. This can leave
> > one of the PMUs severely under utilized.
> 
> The problem is here group validation at pmu::event_init() time, not
> scheduling.

Maybe make that a little more explicit.

> We don't allow grouping across disparate HW PMUs because we can't
> provide group semantics anyway. Scheduling is not a problem in this case
> (unlike the big.LITTLE case I have a patch for [1]).

Right, I remember that; I was wondering if this was related.

> We have a CPU PMU and an "uncore" CCI PMU. You can't create task-bound
> events for the CCI, but you can create CPU-bound events for the CCI on
> the nominal CPU the CCI is monitored from.

Indeed, ok.

> The context check you added in c3c87e770458aa00 "perf: Tighten (and fix)
> the grouping condition" implicitly rejects groups that have CPU and CCI
> events (each event::ctx will be the relevant pmu::pmu_cpu_context and
> will differ), and this is sane -- you can't provide group semantics
> across disparate HW PMUs.

Agreed.

> Unfortunately that happens after we've done the
> event->pmu->event_init(event) dance on each event, and in our event_init
> function we try to verify the group is sane. In our verification we
> ignore SW events, but assume that all !SW events are for the CPU PMU.
> If you add a CPU event to a CCI group, that's not the case, and we use
> container_of on an unsuitable object, derefence garbage, invoke the
> eschaton and so on.

Indeed, on x86 we explicitly ignore everything not an x86_pmu event.

> It would be nicer if we could prevent this in the core so we're not
> reliant on every PMU driver doing the same verification. My initial
> thought was that seemed like unnecessary duplication of the ctx checking
> above, but if we're going to end up shoving it into several drivers
> anyway perhaps it's the lesser evil.

Again, agreed, that would be better and less error prone. But I'm not
entirely sure how to go about doing it :/ I'll have to go think about
that; and conferences are not the best place for that.

Suggestions on that are welcome of course ;)

Re: [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Peter Zijlstra]

On Tue, Mar 10, 2015 at 12:05:21PM +0000, Mark Rutland wrote:
> On Tue, Mar 10, 2015 at 11:27:23AM +0000, Peter Zijlstra wrote:
> > On Mon, Mar 09, 2015 at 12:46:30PM +0000, Suzuki K. Poulose wrote:
> > > From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
> > > 
> > > Don't allow grouping hardware events from different PMUs
> > >  (eg. CCI + CPU).
> > 
> > Uhm, how does this work? If we have multiple hardware PMUs we'll stop
> > scheduling events after the first failed event schedule. This can leave
> > one of the PMUs severely under utilized.
> 
> The problem is here group validation at pmu::event_init() time, not
> scheduling.

Maybe make that a little more explicit.

> We don't allow grouping across disparate HW PMUs because we can't
> provide group semantics anyway. Scheduling is not a problem in this case
> (unlike the big.LITTLE case I have a patch for [1]).

Right, I remember that; I was wondering if this was related.

> We have a CPU PMU and an "uncore" CCI PMU. You can't create task-bound
> events for the CCI, but you can create CPU-bound events for the CCI on
> the nominal CPU the CCI is monitored from.

Indeed, ok.

> The context check you added in c3c87e770458aa00 "perf: Tighten (and fix)
> the grouping condition" implicitly rejects groups that have CPU and CCI
> events (each event::ctx will be the relevant pmu::pmu_cpu_context and
> will differ), and this is sane -- you can't provide group semantics
> across disparate HW PMUs.

Agreed.

> Unfortunately that happens after we've done the
> event->pmu->event_init(event) dance on each event, and in our event_init
> function we try to verify the group is sane. In our verification we
> ignore SW events, but assume that all !SW events are for the CPU PMU.
> If you add a CPU event to a CCI group, that's not the case, and we use
> container_of on an unsuitable object, derefence garbage, invoke the
> eschaton and so on.

Indeed, on x86 we explicitly ignore everything not an x86_pmu event.

> It would be nicer if we could prevent this in the core so we're not
> reliant on every PMU driver doing the same verification. My initial
> thought was that seemed like unnecessary duplication of the ctx checking
> above, but if we're going to end up shoving it into several drivers
> anyway perhaps it's the lesser evil.

Again, agreed, that would be better and less error prone. But I'm not
entirely sure how to go about doing it :/ I'll have to go think about
that; and conferences are not the best place for that.

Suggestions on that are welcome of course ;)

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Peter Zijlstra]

On Tue, Mar 10, 2015 at 01:53:51PM +0100, Peter Zijlstra wrote:
> > It would be nicer if we could prevent this in the core so we're not
> > reliant on every PMU driver doing the same verification. My initial
> > thought was that seemed like unnecessary duplication of the ctx checking
> > above, but if we're going to end up shoving it into several drivers
> > anyway perhaps it's the lesser evil.
> 
> Again, agreed, that would be better and less error prone. But I'm not
> entirely sure how to go about doing it :/ I'll have to go think about
> that; and conferences are not the best place for that.
> 
> Suggestions on that are welcome of course ;)

So the problem is that event_init() is what will return the pmu, so we
cannot make decisions on it until after that returns.

Maybe we can pull out the validate step into its own funciton;
pmu->validate() or whatnot, to be called slightly later.

Re: [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Peter Zijlstra]

On Tue, Mar 10, 2015 at 01:53:51PM +0100, Peter Zijlstra wrote:
> > It would be nicer if we could prevent this in the core so we're not
> > reliant on every PMU driver doing the same verification. My initial
> > thought was that seemed like unnecessary duplication of the ctx checking
> > above, but if we're going to end up shoving it into several drivers
> > anyway perhaps it's the lesser evil.
> 
> Again, agreed, that would be better and less error prone. But I'm not
> entirely sure how to go about doing it :/ I'll have to go think about
> that; and conferences are not the best place for that.
> 
> Suggestions on that are welcome of course ;)

So the problem is that event_init() is what will return the pmu, so we
cannot make decisions on it until after that returns.

Maybe we can pull out the validate step into its own funciton;
pmu->validate() or whatnot, to be called slightly later.

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Mark Rutland]

> So the problem is that event_init() is what will return the pmu, so we
> cannot make decisions on it until after that returns.

I took a look into hacking something into perf_try_init_event, but it
ends up duplicating all of the existing tests and looks really out of
place.

> Maybe we can pull out the validate step into its own funciton;
> pmu->validate() or whatnot, to be called slightly later.

Perhaps the other way around: move the call to pmu::event_init() later
(after the other checks in perf_event_open), and in its original spot
add a new pmu::can_handle() that only tells us whether an event in
isolation is for this PMU, without validation of the specifics of the
event.

That would keep the split between the two callbacks more clearly
defined, though it would require updating most PMU drivers, so perhaps
that's too invasive.

I'll dig into this a bit.

Mark.

Re: [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Mark Rutland]

> So the problem is that event_init() is what will return the pmu, so we
> cannot make decisions on it until after that returns.

I took a look into hacking something into perf_try_init_event, but it
ends up duplicating all of the existing tests and looks really out of
place.

> Maybe we can pull out the validate step into its own funciton;
> pmu->validate() or whatnot, to be called slightly later.

Perhaps the other way around: move the call to pmu::event_init() later
(after the other checks in perf_event_open), and in its original spot
add a new pmu::can_handle() that only tells us whether an event in
isolation is for this PMU, without validation of the specifics of the
event.

That would keep the split between the two callbacks more clearly
defined, though it would require updating most PMU drivers, so perhaps
that's too invasive.

I'll dig into this a bit.

Mark.

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Suzuki K. Poulose]

On 10/03/15 13:00, Peter Zijlstra wrote:
> On Tue, Mar 10, 2015 at 01:53:51PM +0100, Peter Zijlstra wrote:
>>> It would be nicer if we could prevent this in the core so we're not
>>> reliant on every PMU driver doing the same verification. My initial
>>> thought was that seemed like unnecessary duplication of the ctx checking
>>> above, but if we're going to end up shoving it into several drivers
>>> anyway perhaps it's the lesser evil.
>>
>> Again, agreed, that would be better and less error prone. But I'm not
>> entirely sure how to go about doing it :/ I'll have to go think about
>> that; and conferences are not the best place for that.
>>
>> Suggestions on that are welcome of course ;)
>
> So the problem is that event_init() is what will return the pmu, so we
> cannot make decisions on it until after that returns.
>
> Maybe we can pull out the validate step into its own funciton;
> pmu->validate() or whatnot, to be called slightly later.
>
I think we could still solve this problem by deferring the 'context'
validation to the core. The PMUs could validate the group, within its
context. i.e, if it can accommodate its events as a group, during 
event_init.  The problem we face now, is encountering an event from a 
different PMU, which we could leave it to the core as we do already.

i.e the fix could look like (and similarly for other cases):

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 557e128..b3af19b 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -259,20 +259,28 @@ out:
  }

  static int
-validate_event(struct pmu_hw_events *hw_events,
-	       struct perf_event *event)
+validate_event(struct pmu *pmu, struct pmu_hw_events *hw_events,
+			       struct perf_event *event)
  {
-	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+	struct arm_pmu *armpmu;

  	if (is_software_event(event))
  		return 1;

+	/*
+	 * We are only worried if we can accommodate the events
+	 * from this pmu in this group.
+	 */
+	if (event->pmu != pmu)
+		return 1;
+
  	if (event->state < PERF_EVENT_STATE_OFF)
  		return 1;

  	if (event->state == PERF_EVENT_STATE_OFF && !event->attr.enable_on_exec)
  		return 1;

+	armpmu = to_arm_pmu(event->pmu);
  	return armpmu->get_event_idx(hw_events, event) >= 0;
  }

@@ -288,15 +296,15 @@ validate_group(struct perf_event *event)
  	 */
  	memset(&fake_pmu.used_mask, 0, sizeof(fake_pmu.used_mask));

-	if (!validate_event(&fake_pmu, leader))
+	if (!validate_event(event->pmu, &fake_pmu, leader))
  		return -EINVAL;

  	list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
-		if (!validate_event(&fake_pmu, sibling))
+		if (!validate_event(event->pmu, &fake_pmu, sibling))
  			return -EINVAL;
  	}

-	if (!validate_event(&fake_pmu, event))
+	if (!validate_event(event->pmu, &fake_pmu, event))
  		return -EINVAL;

  	return 0;

Thoughts ?

Suzuki

Re: [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Suzuki K. Poulose]

On 10/03/15 13:00, Peter Zijlstra wrote:
> On Tue, Mar 10, 2015 at 01:53:51PM +0100, Peter Zijlstra wrote:
>>> It would be nicer if we could prevent this in the core so we're not
>>> reliant on every PMU driver doing the same verification. My initial
>>> thought was that seemed like unnecessary duplication of the ctx checking
>>> above, but if we're going to end up shoving it into several drivers
>>> anyway perhaps it's the lesser evil.
>>
>> Again, agreed, that would be better and less error prone. But I'm not
>> entirely sure how to go about doing it :/ I'll have to go think about
>> that; and conferences are not the best place for that.
>>
>> Suggestions on that are welcome of course ;)
>
> So the problem is that event_init() is what will return the pmu, so we
> cannot make decisions on it until after that returns.
>
> Maybe we can pull out the validate step into its own funciton;
> pmu->validate() or whatnot, to be called slightly later.
>
I think we could still solve this problem by deferring the 'context'
validation to the core. The PMUs could validate the group, within its
context. i.e, if it can accommodate its events as a group, during 
event_init.  The problem we face now, is encountering an event from a 
different PMU, which we could leave it to the core as we do already.

i.e the fix could look like (and similarly for other cases):

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 557e128..b3af19b 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -259,20 +259,28 @@ out:
  }

  static int
-validate_event(struct pmu_hw_events *hw_events,
-	       struct perf_event *event)
+validate_event(struct pmu *pmu, struct pmu_hw_events *hw_events,
+			       struct perf_event *event)
  {
-	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+	struct arm_pmu *armpmu;

  	if (is_software_event(event))
  		return 1;

+	/*
+	 * We are only worried if we can accommodate the events
+	 * from this pmu in this group.
+	 */
+	if (event->pmu != pmu)
+		return 1;
+
  	if (event->state < PERF_EVENT_STATE_OFF)
  		return 1;

  	if (event->state == PERF_EVENT_STATE_OFF && !event->attr.enable_on_exec)
  		return 1;

+	armpmu = to_arm_pmu(event->pmu);
  	return armpmu->get_event_idx(hw_events, event) >= 0;
  }

@@ -288,15 +296,15 @@ validate_group(struct perf_event *event)
  	 */
  	memset(&fake_pmu.used_mask, 0, sizeof(fake_pmu.used_mask));

-	if (!validate_event(&fake_pmu, leader))
+	if (!validate_event(event->pmu, &fake_pmu, leader))
  		return -EINVAL;

  	list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
-		if (!validate_event(&fake_pmu, sibling))
+		if (!validate_event(event->pmu, &fake_pmu, sibling))
  			return -EINVAL;
  	}

-	if (!validate_event(&fake_pmu, event))
+	if (!validate_event(event->pmu, &fake_pmu, event))
  		return -EINVAL;

  	return 0;

Thoughts ?

Suzuki

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Mark Rutland]

> I think we could still solve this problem by deferring the 'context'
> validation to the core. The PMUs could validate the group, within its
> context. i.e, if it can accommodate its events as a group, during 
> event_init.  The problem we face now, is encountering an event from a 
> different PMU, which we could leave it to the core as we do already.

Good point: we're not reliant on other drivers because the core will
still check the context. We only hope that those other drivers don't
make similar mistakes and corrupt things.

[...]

>   static int
> -validate_event(struct pmu_hw_events *hw_events,
> -	       struct perf_event *event)
> +validate_event(struct pmu *pmu, struct pmu_hw_events *hw_events,
> +			       struct perf_event *event)
>   {
> -	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
> +	struct arm_pmu *armpmu;
> 
>   	if (is_software_event(event))
>   		return 1;
> 
> +	/*
> +	 * We are only worried if we can accommodate the events
> +	 * from this pmu in this group.
> +	 */
> +	if (event->pmu != pmu)
> +		return 1;

It's better to explicitly reject this case. We know it's non-sensical
and there's no point wasting any time on it. That will also make
big.LITTLE support a bit nicer, whenever I get that under control -- big
and LITTLE events could live in the same task context (so the core won't
reject grouping them) but mustn't be in the same group (so we have to
reject grouping in the backend).

I'd still prefer the group validation being triggered explicitly by the
core code, so that it's logically separate from initialising the event
in isolation, but that's looking like a much bigger job, and I don't
trust myself to correctly update every PMU driver for v4.0.

For the moment let's clean up the commit message for the original patch.
I'll add splitting group validation to my TODO list; there seems to be a
slot free around 2035...

Thanks,
Mark.

Re: [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Mark Rutland]

> I think we could still solve this problem by deferring the 'context'
> validation to the core. The PMUs could validate the group, within its
> context. i.e, if it can accommodate its events as a group, during 
> event_init.  The problem we face now, is encountering an event from a 
> different PMU, which we could leave it to the core as we do already.

Good point: we're not reliant on other drivers because the core will
still check the context. We only hope that those other drivers don't
make similar mistakes and corrupt things.

[...]

>   static int
> -validate_event(struct pmu_hw_events *hw_events,
> -	       struct perf_event *event)
> +validate_event(struct pmu *pmu, struct pmu_hw_events *hw_events,
> +			       struct perf_event *event)
>   {
> -	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
> +	struct arm_pmu *armpmu;
> 
>   	if (is_software_event(event))
>   		return 1;
> 
> +	/*
> +	 * We are only worried if we can accommodate the events
> +	 * from this pmu in this group.
> +	 */
> +	if (event->pmu != pmu)
> +		return 1;

It's better to explicitly reject this case. We know it's non-sensical
and there's no point wasting any time on it. That will also make
big.LITTLE support a bit nicer, whenever I get that under control -- big
and LITTLE events could live in the same task context (so the core won't
reject grouping them) but mustn't be in the same group (so we have to
reject grouping in the backend).

I'd still prefer the group validation being triggered explicitly by the
core code, so that it's logically separate from initialising the event
in isolation, but that's looking like a much bigger job, and I don't
trust myself to correctly update every PMU driver for v4.0.

For the moment let's clean up the commit message for the original patch.
I'll add splitting group validation to my TODO list; there seems to be a
slot free around 2035...

Thanks,
Mark.

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Mark Rutland]

On Tue, Mar 10, 2015 at 12:53:51PM +0000, Peter Zijlstra wrote:
> On Tue, Mar 10, 2015 at 12:05:21PM +0000, Mark Rutland wrote:
> > On Tue, Mar 10, 2015 at 11:27:23AM +0000, Peter Zijlstra wrote:
> > > On Mon, Mar 09, 2015 at 12:46:30PM +0000, Suzuki K. Poulose wrote:
> > > > From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
> > > > 
> > > > Don't allow grouping hardware events from different PMUs
> > > >  (eg. CCI + CPU).
> > > 
> > > Uhm, how does this work? If we have multiple hardware PMUs we'll stop
> > > scheduling events after the first failed event schedule. This can leave
> > > one of the PMUs severely under utilized.
> > 
> > The problem is here group validation at pmu::event_init() time, not
> > scheduling.
> 
> Maybe make that a little more explicit.

On the assumption that the patch is otherwise OK, how about the commit
message below?

---->8----
arm/pmu: Reject groups spanning multiple hardware PMUs

The perf core implicitly rejects events spanning multiple HW PMUs, as in
these cases the event->ctx will differ. However this validation is
performed after pmu::event_init() is called in perf_init_event(), and
thus pmu::event_init() may be called with a group leader from a
different HW PMU.

The ARM PMU driver does not take this fact into account, and when
validating groups assumes that it can call to_arm_pmu(event->pmu) for
any HW event. When the event in question is from another HW PMU this is
wrong, and results in dereferencing garbage.

This patch updates the ARM PMU driver to first test for and reject
events from other PMUs, moving the to_arm_pmu and related logic after
this test. Fixes a crash triggered by perf_fuzzer on Linux-4.0-rc2, with
a CCI PMU present:

CPU: 0 PID: 1527 Comm: perf_fuzzer Not tainted 4.0.0-rc2 #57
Hardware name: ARM-Versatile Express
task: bd8484c0 ti: be676000 task.ti: be676000
PC is at 0xbf1bbc90
LR is at validate_event+0x34/0x5c
pc : [<bf1bbc90>]    lr : [<80016060>]    psr: 00000013
...
[<80016060>] (validate_event) from [<80016198>] (validate_group+0x28/0x90)
[<80016198>] (validate_group) from [<80016398>] (armpmu_event_init+0x150/0x218)
[<80016398>] (armpmu_event_init) from [<800882e4>] (perf_try_init_event+0x30/0x48)
[<800882e4>] (perf_try_init_event) from [<8008f544>] (perf_init_event+0x5c/0xf4)
[<8008f544>] (perf_init_event) from [<8008f8a8>] (perf_event_alloc+0x2cc/0x35c)
[<8008f8a8>] (perf_event_alloc) from [<8009015c>] (SyS_perf_event_open+0x498/0xa70)
[<8009015c>] (SyS_perf_event_open) from [<8000e420>] (ret_fast_syscall+0x0/0x34)
Code: bf1be000 bf1bb380 802a2664 00000000 (00000002)
---[ end trace 01aff0ff00926a0a ]---
---->8----

Mark.

Re: [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Mark Rutland]

On Tue, Mar 10, 2015 at 12:53:51PM +0000, Peter Zijlstra wrote:
> On Tue, Mar 10, 2015 at 12:05:21PM +0000, Mark Rutland wrote:
> > On Tue, Mar 10, 2015 at 11:27:23AM +0000, Peter Zijlstra wrote:
> > > On Mon, Mar 09, 2015 at 12:46:30PM +0000, Suzuki K. Poulose wrote:
> > > > From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
> > > > 
> > > > Don't allow grouping hardware events from different PMUs
> > > >  (eg. CCI + CPU).
> > > 
> > > Uhm, how does this work? If we have multiple hardware PMUs we'll stop
> > > scheduling events after the first failed event schedule. This can leave
> > > one of the PMUs severely under utilized.
> > 
> > The problem is here group validation at pmu::event_init() time, not
> > scheduling.
> 
> Maybe make that a little more explicit.

On the assumption that the patch is otherwise OK, how about the commit
message below?

---->8----
arm/pmu: Reject groups spanning multiple hardware PMUs

The perf core implicitly rejects events spanning multiple HW PMUs, as in
these cases the event->ctx will differ. However this validation is
performed after pmu::event_init() is called in perf_init_event(), and
thus pmu::event_init() may be called with a group leader from a
different HW PMU.

The ARM PMU driver does not take this fact into account, and when
validating groups assumes that it can call to_arm_pmu(event->pmu) for
any HW event. When the event in question is from another HW PMU this is
wrong, and results in dereferencing garbage.

This patch updates the ARM PMU driver to first test for and reject
events from other PMUs, moving the to_arm_pmu and related logic after
this test. Fixes a crash triggered by perf_fuzzer on Linux-4.0-rc2, with
a CCI PMU present:

CPU: 0 PID: 1527 Comm: perf_fuzzer Not tainted 4.0.0-rc2 #57
Hardware name: ARM-Versatile Express
task: bd8484c0 ti: be676000 task.ti: be676000
PC is at 0xbf1bbc90
LR is at validate_event+0x34/0x5c
pc : [<bf1bbc90>]    lr : [<80016060>]    psr: 00000013
...
[<80016060>] (validate_event) from [<80016198>] (validate_group+0x28/0x90)
[<80016198>] (validate_group) from [<80016398>] (armpmu_event_init+0x150/0x218)
[<80016398>] (armpmu_event_init) from [<800882e4>] (perf_try_init_event+0x30/0x48)
[<800882e4>] (perf_try_init_event) from [<8008f544>] (perf_init_event+0x5c/0xf4)
[<8008f544>] (perf_init_event) from [<8008f8a8>] (perf_event_alloc+0x2cc/0x35c)
[<8008f8a8>] (perf_event_alloc) from [<8009015c>] (SyS_perf_event_open+0x498/0xa70)
[<8009015c>] (SyS_perf_event_open) from [<8000e420>] (ret_fast_syscall+0x0/0x34)
Code: bf1be000 bf1bb380 802a2664 00000000 (00000002)
---[ end trace 01aff0ff00926a0a ]---
---->8----

Mark.

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Peter Zijlstra]

On Tue, Mar 10, 2015 at 03:36:08PM +0000, Mark Rutland wrote:
> On the assumption that the patch is otherwise OK, how about the commit
> message below?

Yeah, that'll do fine. Thanks!

Re: [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Peter Zijlstra]

On Tue, Mar 10, 2015 at 03:36:08PM +0000, Mark Rutland wrote:
> On the assumption that the patch is otherwise OK, how about the commit
> message below?

Yeah, that'll do fine. Thanks!

[PATCH 2/3] arm64/pmu: Reject groups spanning multiple HW PMUs

[Suzuki K. Poulose]

From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>

Don't allow grouping hardware events from different PMUs
 (eg. CCI + CPU).

Fixes a crash triggered by perf_fuzzer on Linux-4.0-rc2,
with CCI PMU turned on. The validate_event(), after certain checks,
assumes that the given hardware pmu event belongs to armpmu,
which may not be true always, with other hardware PMUs
around (CCI, CCN).

Bad mode in Synchronous Abort handler detected, code 0x86000006 -- IABT (current EL)
CPU: 0 PID: 1371 Comm: perf_fuzzer Not tainted 3.19.0+ #249
Hardware name: V2F-1XV7 Cortex-A53x2 SMM (DT)
task: ffffffc07c73a280 ti: ffffffc07b0a0000 task.ti: ffffffc07b0a0000
PC is at 0x0
LR is at validate_event+0x90/0xa8
pc : [<0000000000000000>] lr : [<ffffffc000090228>] pstate: 00000145
sp : ffffffc07b0a3ba0

[<          (null)>]           (null)
[<ffffffc0000907d8>] armpmu_event_init+0x174/0x3cc
[<ffffffc00015d870>] perf_try_init_event+0x34/0x70
[<ffffffc000164094>] perf_init_event+0xe0/0x10c
[<ffffffc000164348>] perf_event_alloc+0x288/0x358
[<ffffffc000164c5c>] SyS_perf_event_open+0x464/0x98c
Code: bad PC value

Also cleans up the code to use the arm_pmu only when we know
that we are dealing with an arm pmu event.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/perf_event.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 25a5308..68a7415 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -322,22 +322,31 @@ out:
 }
 
 static int
-validate_event(struct pmu_hw_events *hw_events,
-	       struct perf_event *event)
+validate_event(struct pmu *pmu, struct pmu_hw_events *hw_events,
+				struct perf_event *event)
 {
-	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+	struct arm_pmu *armpmu;
 	struct hw_perf_event fake_event = event->hw;
 	struct pmu *leader_pmu = event->group_leader->pmu;
 
 	if (is_software_event(event))
 		return 1;
 
+	/*
+	 * Reject groups spanning multiple HW PMUs (e.g. CPU + CCI). The
+	 * core perf code won't check that the pmu->ctx == leader->ctx
+	 * until after pmu->event_init(event).
+	 */
+	if (event->pmu != pmu)
+		return 0;
+
 	if (event->pmu != leader_pmu || event->state < PERF_EVENT_STATE_OFF)
 		return 1;
 
 	if (event->state == PERF_EVENT_STATE_OFF && !event->attr.enable_on_exec)
 		return 1;
 
+	armpmu = to_arm_pmu(event->pmu);
 	return armpmu->get_event_idx(hw_events, &fake_event) >= 0;
 }
 
@@ -355,15 +364,15 @@ validate_group(struct perf_event *event)
 	memset(fake_used_mask, 0, sizeof(fake_used_mask));
 	fake_pmu.used_mask = fake_used_mask;
 
-	if (!validate_event(&fake_pmu, leader))
+	if (!validate_event(event->pmu, &fake_pmu, leader))
 		return -EINVAL;
 
 	list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
-		if (!validate_event(&fake_pmu, sibling))
+		if (!validate_event(event->pmu, &fake_pmu, sibling))
 			return -EINVAL;
 	}
 
-	if (!validate_event(&fake_pmu, event))
+	if (!validate_event(event->pmu, &fake_pmu, event))
 		return -EINVAL;
 
 	return 0;
-- 
1.7.9.5

[PATCH 2/3] arm64/pmu: Reject groups spanning multiple HW PMUs

[Suzuki K. Poulose]

From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>

Don't allow grouping hardware events from different PMUs
 (eg. CCI + CPU).

Fixes a crash triggered by perf_fuzzer on Linux-4.0-rc2,
with CCI PMU turned on. The validate_event(), after certain checks,
assumes that the given hardware pmu event belongs to armpmu,
which may not be true always, with other hardware PMUs
around (CCI, CCN).

Bad mode in Synchronous Abort handler detected, code 0x86000006 -- IABT (current EL)
CPU: 0 PID: 1371 Comm: perf_fuzzer Not tainted 3.19.0+ #249
Hardware name: V2F-1XV7 Cortex-A53x2 SMM (DT)
task: ffffffc07c73a280 ti: ffffffc07b0a0000 task.ti: ffffffc07b0a0000
PC is at 0x0
LR is at validate_event+0x90/0xa8
pc : [<0000000000000000>] lr : [<ffffffc000090228>] pstate: 00000145
sp : ffffffc07b0a3ba0

[<          (null)>]           (null)
[<ffffffc0000907d8>] armpmu_event_init+0x174/0x3cc
[<ffffffc00015d870>] perf_try_init_event+0x34/0x70
[<ffffffc000164094>] perf_init_event+0xe0/0x10c
[<ffffffc000164348>] perf_event_alloc+0x288/0x358
[<ffffffc000164c5c>] SyS_perf_event_open+0x464/0x98c
Code: bad PC value

Also cleans up the code to use the arm_pmu only when we know
that we are dealing with an arm pmu event.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/perf_event.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 25a5308..68a7415 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -322,22 +322,31 @@ out:
 }
 
 static int
-validate_event(struct pmu_hw_events *hw_events,
-	       struct perf_event *event)
+validate_event(struct pmu *pmu, struct pmu_hw_events *hw_events,
+				struct perf_event *event)
 {
-	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+	struct arm_pmu *armpmu;
 	struct hw_perf_event fake_event = event->hw;
 	struct pmu *leader_pmu = event->group_leader->pmu;
 
 	if (is_software_event(event))
 		return 1;
 
+	/*
+	 * Reject groups spanning multiple HW PMUs (e.g. CPU + CCI). The
+	 * core perf code won't check that the pmu->ctx == leader->ctx
+	 * until after pmu->event_init(event).
+	 */
+	if (event->pmu != pmu)
+		return 0;
+
 	if (event->pmu != leader_pmu || event->state < PERF_EVENT_STATE_OFF)
 		return 1;
 
 	if (event->state == PERF_EVENT_STATE_OFF && !event->attr.enable_on_exec)
 		return 1;
 
+	armpmu = to_arm_pmu(event->pmu);
 	return armpmu->get_event_idx(hw_events, &fake_event) >= 0;
 }
 
@@ -355,15 +364,15 @@ validate_group(struct perf_event *event)
 	memset(fake_used_mask, 0, sizeof(fake_used_mask));
 	fake_pmu.used_mask = fake_used_mask;
 
-	if (!validate_event(&fake_pmu, leader))
+	if (!validate_event(event->pmu, &fake_pmu, leader))
 		return -EINVAL;
 
 	list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
-		if (!validate_event(&fake_pmu, sibling))
+		if (!validate_event(event->pmu, &fake_pmu, sibling))
 			return -EINVAL;
 	}
 
-	if (!validate_event(&fake_pmu, event))
+	if (!validate_event(event->pmu, &fake_pmu, event))
 		return -EINVAL;
 
 	return 0;
-- 
1.7.9.5

[PATCH 3/3] arm-cci: Reject groups spanning multiple HW PMUs

[Suzuki K. Poulose]

From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>

Invalidate an event if the group has a hardware event from
a different PMU as we cannot schedule all of them in the same
context. The perf core code won't check the group consistency
until after pmu_event_init().

Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 drivers/bus/arm-cci.c |   19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 84fd660..68ef6f2 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -660,12 +660,21 @@ static void cci_pmu_del(struct perf_event *event, int flags)
 }
 
 static int
-validate_event(struct cci_pmu_hw_events *hw_events,
-	       struct perf_event *event)
+validate_event(struct pmu *cci_pmu,
+               struct cci_pmu_hw_events *hw_events,
+               struct perf_event *event)
 {
 	if (is_software_event(event))
 		return 1;
 
+	/*
+	 * Reject groups spanning multiple HW PMUs (e.g. CPU + CCI). The
+	 * core perf code won't check that the pmu->ctx == leader->ctx
+	 * until after pmu->event_init(event).
+	 */
+	if (event->pmu != cci_pmu)
+		return 0;
+
 	if (event->state < PERF_EVENT_STATE_OFF)
 		return 1;
 
@@ -687,15 +696,15 @@ validate_group(struct perf_event *event)
 		.used_mask = CPU_BITS_NONE,
 	};
 
-	if (!validate_event(&fake_pmu, leader))
+	if (!validate_event(event->pmu, &fake_pmu, leader))
 		return -EINVAL;
 
 	list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
-		if (!validate_event(&fake_pmu, sibling))
+		if (!validate_event(event->pmu, &fake_pmu, sibling))
 			return -EINVAL;
 	}
 
-	if (!validate_event(&fake_pmu, event))
+	if (!validate_event(event->pmu, &fake_pmu, event))
 		return -EINVAL;
 
 	return 0;
-- 
1.7.9.5

[PATCH 3/3] arm-cci: Reject groups spanning multiple HW PMUs

[Suzuki K. Poulose]

From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>

Invalidate an event if the group has a hardware event from
a different PMU as we cannot schedule all of them in the same
context. The perf core code won't check the group consistency
until after pmu_event_init().

Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 drivers/bus/arm-cci.c |   19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 84fd660..68ef6f2 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -660,12 +660,21 @@ static void cci_pmu_del(struct perf_event *event, int flags)
 }
 
 static int
-validate_event(struct cci_pmu_hw_events *hw_events,
-	       struct perf_event *event)
+validate_event(struct pmu *cci_pmu,
+               struct cci_pmu_hw_events *hw_events,
+               struct perf_event *event)
 {
 	if (is_software_event(event))
 		return 1;
 
+	/*
+	 * Reject groups spanning multiple HW PMUs (e.g. CPU + CCI). The
+	 * core perf code won't check that the pmu->ctx == leader->ctx
+	 * until after pmu->event_init(event).
+	 */
+	if (event->pmu != cci_pmu)
+		return 0;
+
 	if (event->state < PERF_EVENT_STATE_OFF)
 		return 1;
 
@@ -687,15 +696,15 @@ validate_group(struct perf_event *event)
 		.used_mask = CPU_BITS_NONE,
 	};
 
-	if (!validate_event(&fake_pmu, leader))
+	if (!validate_event(event->pmu, &fake_pmu, leader))
 		return -EINVAL;
 
 	list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
-		if (!validate_event(&fake_pmu, sibling))
+		if (!validate_event(event->pmu, &fake_pmu, sibling))
 			return -EINVAL;
 	}
 
-	if (!validate_event(&fake_pmu, event))
+	if (!validate_event(event->pmu, &fake_pmu, event))
 		return -EINVAL;
 
 	return 0;
-- 
1.7.9.5

[PATCHv2 0/3] [4.0] arm/arm64: Do not group hardware events from different PMUs

[Suzuki K. Poulose]

From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>

This is a collection of fixes which denies grouping hardware events
from different PMUs. They also fix crashes triggered by perf_fuzzer
on Linux-4.0-rc2.

This series hasn't changed since V1, except for better commit messages.

Similar fixes are required in ARM-CCN PMU driver and is not straight forward
as these changes.

Suzuki K. Poulose (3):
  arm/pmu: Reject groups spanning multiple hardware PMUs
  arm64/pmu: Reject groups spanning multiple HW PMUs
  arm-cci: Reject groups spanning multiple HW PMUs

 arch/arm/kernel/perf_event.c   |   21 +++++++++++++++------
 arch/arm64/kernel/perf_event.c |   21 +++++++++++++++------
 drivers/bus/arm-cci.c          |   19 ++++++++++++++-----
 3 files changed, 44 insertions(+), 17 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[Suzuki K. Poulose]

From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>

The perf core implicitly rejects events spanning multiple HW PMUs, as in
these cases the event->ctx will differ. However this validation is
performed after pmu::event_init() is called in perf_init_event(), and
thus pmu::event_init() may be called with a group leader from a
different HW PMU.

The ARM PMU driver does not take this fact into account, and when
validating groups assumes that it can call to_arm_pmu(event->pmu) for
any HW event. When the event in question is from another HW PMU this is
wrong, and results in dereferencing garbage.

This patch updates the ARM PMU driver to first test for and reject
events from other PMUs, moving the to_arm_pmu and related logic after
this test. Fixes a crash triggered by perf_fuzzer on Linux-4.0-rc2, with
a CCI PMU present:

 ---
CPU: 0 PID: 1527 Comm: perf_fuzzer Not tainted 4.0.0-rc2 #57
Hardware name: ARM-Versatile Express
task: bd8484c0 ti: be676000 task.ti: be676000
PC is at 0xbf1bbc90
LR is at validate_event+0x34/0x5c
pc : [<bf1bbc90>]    lr : [<80016060>]    psr: 00000013
...
[<80016060>] (validate_event) from [<80016198>] (validate_group+0x28/0x90)
[<80016198>] (validate_group) from [<80016398>] (armpmu_event_init+0x150/0x218)
[<80016398>] (armpmu_event_init) from [<800882e4>] (perf_try_init_event+0x30/0x48)
[<800882e4>] (perf_try_init_event) from [<8008f544>] (perf_init_event+0x5c/0xf4)
[<8008f544>] (perf_init_event) from [<8008f8a8>] (perf_event_alloc+0x2cc/0x35c)
[<8008f8a8>] (perf_event_alloc) from [<8009015c>] (SyS_perf_event_open+0x498/0xa70)
[<8009015c>] (SyS_perf_event_open) from [<8000e420>] (ret_fast_syscall+0x0/0x34)
Code: bf1be000 bf1bb380 802a2664 00000000 (00000002)
---[ end trace 01aff0ff00926a0a ]---

Also cleans up the code to use the arm_pmu only when we know that
we are dealing with an arm pmu event.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm/kernel/perf_event.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 557e128..4a86a01 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -259,20 +259,29 @@ out:
 }
 
 static int
-validate_event(struct pmu_hw_events *hw_events,
-	       struct perf_event *event)
+validate_event(struct pmu *pmu, struct pmu_hw_events *hw_events,
+			       struct perf_event *event)
 {
-	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+	struct arm_pmu *armpmu;
 
 	if (is_software_event(event))
 		return 1;
 
+	/*
+	 * Reject groups spanning multiple HW PMUs (e.g. CPU + CCI). The
+	 * core perf code won't check that the pmu->ctx == leader->ctx
+	 * until after pmu->event_init(event).
+	 */
+	if (event->pmu != pmu)
+		return 0;
+
 	if (event->state < PERF_EVENT_STATE_OFF)
 		return 1;
 
 	if (event->state == PERF_EVENT_STATE_OFF && !event->attr.enable_on_exec)
 		return 1;
 
+	armpmu = to_arm_pmu(event->pmu);
 	return armpmu->get_event_idx(hw_events, event) >= 0;
 }
 
@@ -288,15 +297,15 @@ validate_group(struct perf_event *event)
 	 */
 	memset(&fake_pmu.used_mask, 0, sizeof(fake_pmu.used_mask));
 
-	if (!validate_event(&fake_pmu, leader))
+	if (!validate_event(event->pmu, &fake_pmu, leader))
 		return -EINVAL;
 
 	list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
-		if (!validate_event(&fake_pmu, sibling))
+		if (!validate_event(event->pmu, &fake_pmu, sibling))
 			return -EINVAL;
 	}
 
-	if (!validate_event(&fake_pmu, event))
+	if (!validate_event(event->pmu, &fake_pmu, event))
 		return -EINVAL;
 
 	return 0;
-- 
1.7.9.5

[PATCH 0/3] [4.0] arm/arm64: Do not group hardware events from different PMUs

[a]

From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>

This is a collection of fixes which denies grouping hardware events
from different PMUs. They also fix crashes triggered by perf_fuzzer
on Linux-4.0-rc2.

Pawel,

Similar fix is required in ARM CCN PMU driver. I didn't find it
straight forward to fix it there. Could you please take a look
into it ?


Suzuki K. Poulose (3):
  arm/pmu: Reject groups spanning multiple hardware PMUs
  arm64/pmu: Reject groups spanning multiple HW PMUs
  arm-cci: Reject groups spanning multiple HW PMUs

 arch/arm/kernel/perf_event.c   |   21 +++++++++++++++------
 arch/arm64/kernel/perf_event.c |   21 +++++++++++++++------
 drivers/bus/arm-cci.c          |   19 ++++++++++++++-----
 3 files changed, 44 insertions(+), 17 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs

[a]

From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>

Don't allow grouping hardware events from different PMUs
 (eg. CCI + CPU).

Fixes a crash triggered by perf_fuzzer on Linux-4.0-rc2,
with CCI PMU turned on. The validate_event(), after certain checks,
assumes that the given hardware pmu event belongs to armpmu,
which may not be true always, with other hardware PMUs
around (CCI, CCN).

 ---
CPU: 0 PID: 1527 Comm: perf_fuzzer Not tainted 4.0.0-rc2 #57
Hardware name: ARM-Versatile Express
task: bd8484c0 ti: be676000 task.ti: be676000
PC is at 0xbf1bbc90
LR is at validate_event+0x34/0x5c
pc : [<bf1bbc90>]    lr : [<80016060>]    psr: 00000013
...
[<80016060>] (validate_event) from [<80016198>] (validate_group+0x28/0x90)
[<80016198>] (validate_group) from [<80016398>] (armpmu_event_init+0x150/0x218)
[<80016398>] (armpmu_event_init) from [<800882e4>] (perf_try_init_event+0x30/0x48)
[<800882e4>] (perf_try_init_event) from [<8008f544>] (perf_init_event+0x5c/0xf4)
[<8008f544>] (perf_init_event) from [<8008f8a8>] (perf_event_alloc+0x2cc/0x35c)
[<8008f8a8>] (perf_event_alloc) from [<8009015c>] (SyS_perf_event_open+0x498/0xa70)
[<8009015c>] (SyS_perf_event_open) from [<8000e420>] (ret_fast_syscall+0x0/0x34)
Code: bf1be000 bf1bb380 802a2664 00000000 (00000002)
---[ end trace 01aff0ff00926a0a ]---

Also cleans up the code to use the arm_pmu only when we know that
we are dealing with an arm pmu event.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm/kernel/perf_event.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 557e128..4a86a01 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -259,20 +259,29 @@ out:
 }
 
 static int
-validate_event(struct pmu_hw_events *hw_events,
-	       struct perf_event *event)
+validate_event(struct pmu *pmu, struct pmu_hw_events *hw_events,
+			       struct perf_event *event)
 {
-	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+	struct arm_pmu *armpmu;
 
 	if (is_software_event(event))
 		return 1;
 
+	/*
+	 * Reject groups spanning multiple HW PMUs (e.g. CPU + CCI). The
+	 * core perf code won't check that the pmu->ctx == leader->ctx
+	 * until after pmu->event_init(event).
+	 */
+	if (event->pmu != pmu)
+		return 0;
+
 	if (event->state < PERF_EVENT_STATE_OFF)
 		return 1;
 
 	if (event->state == PERF_EVENT_STATE_OFF && !event->attr.enable_on_exec)
 		return 1;
 
+	armpmu = to_arm_pmu(event->pmu);
 	return armpmu->get_event_idx(hw_events, event) >= 0;
 }
 
@@ -288,15 +297,15 @@ validate_group(struct perf_event *event)
 	 */
 	memset(&fake_pmu.used_mask, 0, sizeof(fake_pmu.used_mask));
 
-	if (!validate_event(&fake_pmu, leader))
+	if (!validate_event(event->pmu, &fake_pmu, leader))
 		return -EINVAL;
 
 	list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
-		if (!validate_event(&fake_pmu, sibling))
+		if (!validate_event(event->pmu, &fake_pmu, sibling))
 			return -EINVAL;
 	}
 
-	if (!validate_event(&fake_pmu, event))
+	if (!validate_event(event->pmu, &fake_pmu, event))
 		return -EINVAL;
 
 	return 0;
-- 
1.7.9.5