[PATCH] arm_pmu: Validate single/group leader events

[PATCH] arm_pmu: Validate single/group leader events

[Rob Herring]

In the case where there is only a cycle counter available (i.e.
PMCR_EL0.N is 0) and an event other than CPU cycles is opened, the open
should fail as the event can never possibly be scheduled. However, the
event validation when an event is opened is skipped when the group
leader is opened. Fix this by always validating the group leader events.

Reported-by: Al Grant <al.grant@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/perf/arm_pmu.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
index 9694370651fa..59d3980b8ca2 100644
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -400,6 +400,9 @@ validate_group(struct perf_event *event)
 	if (!validate_event(event->pmu, &fake_pmu, leader))
 		return -EINVAL;
 
+	if (event == leader)
+		return 0;
+
 	for_each_sibling_event(sibling, leader) {
 		if (!validate_event(event->pmu, &fake_pmu, sibling))
 			return -EINVAL;
@@ -489,12 +492,7 @@ __hw_perf_event_init(struct perf_event *event)
 		local64_set(&hwc->period_left, hwc->sample_period);
 	}
 
-	if (event->group_leader != event) {
-		if (validate_group(event) != 0)
-			return -EINVAL;
-	}
-
-	return 0;
+	return validate_group(event);
 }
 
 static int armpmu_event_init(struct perf_event *event)
-- 
2.32.0

Re: [PATCH] arm_pmu: Validate single/group leader events

[Mark Rutland]

Hi Rob,

On Fri, Apr 08, 2022 at 03:33:30PM -0500, Rob Herring wrote:
> In the case where there is only a cycle counter available (i.e.
> PMCR_EL0.N is 0) and an event other than CPU cycles is opened, the open
> should fail as the event can never possibly be scheduled. However, the
> event validation when an event is opened is skipped when the group
> leader is opened. Fix this by always validating the group leader events.
> 
> Reported-by: Al Grant <al.grant@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Rob Herring <robh@kernel.org>

This looks obviously correct to me, so FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Just to check, have you tested this (e.g. by running on a platform with
PMCR_EL0.N == 0, or hacking the PMU probing to report just the cycle counter)

Thanks,
Mark.

> ---
>  drivers/perf/arm_pmu.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index 9694370651fa..59d3980b8ca2 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -400,6 +400,9 @@ validate_group(struct perf_event *event)
>  	if (!validate_event(event->pmu, &fake_pmu, leader))
>  		return -EINVAL;
>  
> +	if (event == leader)
> +		return 0;
> +
>  	for_each_sibling_event(sibling, leader) {
>  		if (!validate_event(event->pmu, &fake_pmu, sibling))
>  			return -EINVAL;
> @@ -489,12 +492,7 @@ __hw_perf_event_init(struct perf_event *event)
>  		local64_set(&hwc->period_left, hwc->sample_period);
>  	}
>  
> -	if (event->group_leader != event) {
> -		if (validate_group(event) != 0)
> -			return -EINVAL;
> -	}
> -
> -	return 0;
> +	return validate_group(event);
>  }
>  
>  static int armpmu_event_init(struct perf_event *event)
> -- 
> 2.32.0
> 

Re: [PATCH] arm_pmu: Validate single/group leader events

[Rob Herring]

On Mon, Apr 11, 2022 at 11:04:26AM +0100, Mark Rutland wrote:
> Hi Rob,
> 
> On Fri, Apr 08, 2022 at 03:33:30PM -0500, Rob Herring wrote:
> > In the case where there is only a cycle counter available (i.e.
> > PMCR_EL0.N is 0) and an event other than CPU cycles is opened, the open
> > should fail as the event can never possibly be scheduled. However, the
> > event validation when an event is opened is skipped when the group
> > leader is opened. Fix this by always validating the group leader events.
> > 
> > Reported-by: Al Grant <al.grant@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> 
> This looks obviously correct to me, so FWIW:
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks.

> Just to check, have you tested this (e.g. by running on a platform with
> PMCR_EL0.N == 0, or hacking the PMU probing to report just the cycle counter)

Yes, tested on FVP with 0 counters running the libperf evsel userspace 
access tests as that tries both the cycle counter and instruction 
counts.

Rob

Re: [PATCH] arm_pmu: Validate single/group leader events

[Mark Rutland]

On Mon, Apr 11, 2022 at 09:14:03AM -0500, Rob Herring wrote:
> On Mon, Apr 11, 2022 at 11:04:26AM +0100, Mark Rutland wrote:
> > Hi Rob,
> > 
> > On Fri, Apr 08, 2022 at 03:33:30PM -0500, Rob Herring wrote:
> > > In the case where there is only a cycle counter available (i.e.
> > > PMCR_EL0.N is 0) and an event other than CPU cycles is opened, the open
> > > should fail as the event can never possibly be scheduled. However, the
> > > event validation when an event is opened is skipped when the group
> > > leader is opened. Fix this by always validating the group leader events.
> > > 
> > > Reported-by: Al Grant <al.grant@arm.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Signed-off-by: Rob Herring <robh@kernel.org>
> > 
> > This looks obviously correct to me, so FWIW:
> > 
> > Acked-by: Mark Rutland <mark.rutland@arm.com>
> 
> Thanks.
> 
> > Just to check, have you tested this (e.g. by running on a platform with
> > PMCR_EL0.N == 0, or hacking the PMU probing to report just the cycle counter)
> 
> Yes, tested on FVP with 0 counters running the libperf evsel userspace 
> access tests as that tries both the cycle counter and instruction 
> counts.

Perfect, thanks!

Will, I assume that you'll pick this up.

Mark.

Re: [PATCH] arm_pmu: Validate single/group leader events

[Will Deacon]

On Fri, 8 Apr 2022 15:33:30 -0500, Rob Herring wrote:
> In the case where there is only a cycle counter available (i.e.
> PMCR_EL0.N is 0) and an event other than CPU cycles is opened, the open
> should fail as the event can never possibly be scheduled. However, the
> event validation when an event is opened is skipped when the group
> leader is opened. Fix this by always validating the group leader events.
> 
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm_pmu: Validate single/group leader events
      https://git.kernel.org/arm64/c/e5c23779f93d

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev