Re: [PATCH v10 3/5] arm64: perf: Add userspace counter access disable switch

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Sep 14, 2021 at 03:47:58PM -0500, Rob Herring wrote:
> Like x86, some users may want to disable userspace PMU counter
> altogether. Add a sysctl 'perf_user_access' file to control userspace
> counter access. The default is '0' which is disabled. Writing '1'
> enables access.
> 
> Note that x86 also supports writing '2' to globally enable user access.

For clarity it might be worth mentioning that on x86 this is controlled
by the PMU's `rdpmc` sysfs attribute, i.e.

  Note that x86 supports globally enabling user access by writing '2' to
  /sys/bus/event_source/devices/cpu/rdpmc

> As there's not existing userspace support to worry about, this shouldn't
> be necessary for Arm. It could be added later if the need arises.
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: linux-perf-users@vger.kernel.org
> Acked-by: Will Deacon <will@kernel.org>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> v10:
>  - Add documentation
>  - Use a custom handler (needed on the next patch)
> v9:
>  - Use sysctl instead of sysfs attr
>  - Default to disabled
> v8:
>  - New patch
> ---
>  Documentation/admin-guide/sysctl/kernel.rst | 11 +++++++++
>  arch/arm64/kernel/perf_event.c              | 27 +++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index 426162009ce9..346a0dba5703 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -905,6 +905,17 @@ enabled, otherwise writing to this file will return ``-EBUSY``.
>  The default value is 8.
>  
>  
> +perf_user_access (arm64 only)
> +=================================
> +
> +Controls user space access for reading perf event counters. When set to 1,
> +user space can read performance monitor counter registers directly.
> +
> +The default value is 0 (access disabled).
> +
> +See Documentation/arm64/perf.rst for more information.

Looking at the existing perf sysctls:

# ls /proc/sys/kernel/perf*
/proc/sys/kernel/perf_cpu_time_max_percent
/proc/sys/kernel/perf_event_max_contexts_per_stack
/proc/sys/kernel/perf_event_max_sample_rate
/proc/sys/kernel/perf_event_max_stack
/proc/sys/kernel/perf_event_mlock_kb
/proc/sys/kernel/perf_event_paranoid

I see that other than `perf_cpu_time_max_percent`, we've used
`perf_event_` as the prefix, and I suspect we should do the same here,
but I guess it may not matter either way.

> +
> +
>  pid_max
>  =======
>  
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index b4044469527e..a8f8dd741aeb 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -286,6 +286,8 @@ static const struct attribute_group armv8_pmuv3_events_attr_group = {
>  PMU_FORMAT_ATTR(event, "config:0-15");
>  PMU_FORMAT_ATTR(long, "config1:0");
>  
> +static int sysctl_perf_user_access __read_mostly;
> +
>  static inline bool armv8pmu_event_is_64bit(struct perf_event *event)
>  {
>  	return event->attr.config1 & 0x1;
> @@ -1104,6 +1106,29 @@ static int armv8pmu_probe_pmu(struct arm_pmu *cpu_pmu)
>  	return probe.present ? 0 : -ENODEV;
>  }
>  
> +int armv8pmu_proc_user_access_handler(struct ctl_table *table, int write,
> +                void *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> +	if (ret || !write || sysctl_perf_user_access)
> +		return ret;
> +
> +	return 0;
> +}

Maybe this is needed in the next patch, but the if statement is entirely
redundant on this patch and looks really odd.

Can we please either:

1) Use proc_dointvec_minmax() directly in this patch (which is what Will
Acked in v9) and add the wrapper in the next patch when we need it.

2) make this:

| int armv8pmu_proc_user_access_handler(struct ctl_table *table, int write,
|                 void *buffer, size_t *lenp, loff_t *ppos)
| {
| 	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
| }

... and flesh it out in the next patch.

With either of those two options (and regardless of whether the
attribute is renamed):

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

Thanks,
Mark.

> +
> +	return 0;
> +}
> +
> +static struct ctl_table armv8_pmu_sysctl_table[] = {
> +	{
> +		.procname       = "perf_user_access",
> +		.data		= &sysctl_perf_user_access,
> +		.maxlen		= sizeof(unsigned int),
> +		.mode           = 0644,
> +		.proc_handler	= armv8pmu_proc_user_access_handler,
> +		.extra1		= SYSCTL_ZERO,
> +		.extra2		= SYSCTL_ONE,
> +	},
> +	{ }
> +};
> +
>  static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
>  			  int (*map_event)(struct perf_event *event),
>  			  const struct attribute_group *events,
> @@ -1136,6 +1161,8 @@ static int armv8_pmu_init(struct arm_pmu *cpu_pmu, char *name,
>  	cpu_pmu->attr_groups[ARMPMU_ATTR_GROUP_CAPS] = caps ?
>  			caps : &armv8_pmuv3_caps_attr_group;
>  
> +	register_sysctl("kernel", armv8_pmu_sysctl_table);
> +
>  	return 0;
>  }
>  
> -- 
> 2.30.2
>