| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 13 Oct 2021 18:30:19 +0100
From: Mark Rutland <mark.rutland@....com>
To: Rob Herring <robh@...nel.org>
Cc: Will Deacon <will@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Catalin Marinas <catalin.marinas@....com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Jiri Olsa <jolsa@...hat.com>,
Kan Liang <kan.liang@...ux.intel.com>,
Ian Rogers <irogers@...gle.com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
honnappa.nagarahalli@....com, Zachary.Leaf@....com,
Raphael Gault <raphael.gault@....com>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Namhyung Kim <namhyung@...nel.org>,
Itaru Kitayama <itaru.kitayama@...il.com>,
Vince Weaver <vincent.weaver@...ne.edu>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v10 3/5] arm64: perf: Add userspace counter access
disable switch
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@....com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Arnaldo Carvalho de Melo <acme@...nel.org>
> Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
> Cc: Jiri Olsa <jolsa@...hat.com>
> Cc: Namhyung Kim <namhyung@...nel.org>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: linux-perf-users@...r.kernel.org
> Acked-by: Will Deacon <will@...nel.org>
> Signed-off-by: Rob Herring <robh@...nel.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@....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
>
Powered by blists - more mailing lists