On Sat, Apr 29, 2023 at 11:49 AM Atish Patra <atishp@xxxxxxxxxxxxxx> wrote: > > On Wed, Apr 26, 2023 at 6:55 PM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> wrote: > > > > On Wed, Apr 26, 2023 at 03:17:01PM +0200, Alexandre Ghiti wrote: > > > On Wed, Apr 26, 2023 at 2:57 PM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> wrote: > > > > > > > > On Thu, Apr 13, 2023 at 06:17:25PM +0200, Alexandre Ghiti wrote: > > > > > We used to unconditionnally expose the cycle and instret csrs to > > > > > userspace, which gives rise to security concerns. > > > > > > > > > > So only allow access to hw counters from userspace through the perf > > > > > framework which will handle context switchs, per-task events...etc. But > > > > > as we cannot break userspace, we give the user the choice to go back to > > > > > the previous behaviour by setting the sysctl perf_user_access. > > > > > > > > > > We also introduce a means to directly map the hardware counters to > > > > > userspace, thus avoiding the need for syscalls whenever an application > > > > > wants to access counters values. > > > > > > > > > > Note that arch_perf_update_userpage is a copy of arm64 code. > > > > > > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@xxxxxxxxxxxx> > > > > > --- > > > > > Documentation/admin-guide/sysctl/kernel.rst | 23 +++- > > > > > arch/riscv/include/asm/perf_event.h | 3 + > > > > > arch/riscv/kernel/Makefile | 2 +- > > > > > arch/riscv/kernel/perf_event.c | 65 +++++++++++ > > > > > drivers/perf/riscv_pmu.c | 42 ++++++++ > > > > > drivers/perf/riscv_pmu_legacy.c | 17 +++ > > > > > drivers/perf/riscv_pmu_sbi.c | 113 ++++++++++++++++++-- > > > > > include/linux/perf/riscv_pmu.h | 3 + > > > > > tools/lib/perf/mmap.c | 65 +++++++++++ > > > > > 9 files changed, 322 insertions(+), 11 deletions(-) > > > > > create mode 100644 arch/riscv/kernel/perf_event.c > > > > > > > > > > diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst > > > > > index 4b7bfea28cd7..02b2a40a3647 100644 > > > > > --- a/Documentation/admin-guide/sysctl/kernel.rst > > > > > +++ b/Documentation/admin-guide/sysctl/kernel.rst > > > > > @@ -941,16 +941,31 @@ enabled, otherwise writing to this file will return ``-EBUSY``. > > > > > The default value is 8. > > > > > > > > > > > > > > > -perf_user_access (arm64 only) > > > > > -================================= > > > > > +perf_user_access (arm64 and riscv only) > > > > > +======================================= > > > > > + > > > > > +Controls user space access for reading perf event counters. > > > > > > > > > > -Controls user space access for reading perf event counters. When set to 1, > > > > > -user space can read performance monitor counter registers directly. > > > > > +arm64 > > > > > +===== > > > > > > > > > > The default value is 0 (access disabled). > > > > > +When set to 1, user space can read performance monitor counter registers > > > > > +directly. > > > > > > > > > > See Documentation/arm64/perf.rst for more information. > > > > > > > > > > +riscv > > > > > +===== > > > > > + > > > > > +When set to 0, user access is disabled. > > > > > + > > > > > +When set to 1, user space can read performance monitor counter registers > > > > > +directly only through perf, any direct access without perf intervention will > > > > > +trigger an illegal instruction. > > > > > + > > > > > +The default value is 2, it enables the legacy mode, that is user space has > > > > > +direct access to cycle, time and insret CSRs only. > > > > > > > > I think this default value should be a Kconfig symbol, allowing kernels to > > > > be built with a secure default. > > > > > > Actually I was more in favor of having the default to 1 (ie the secure > > > option) and let the distros deal with the legacy mode (via a sysctl > > > parameter on the command line) as long as user-space has not been > > > fixed: does that make sense? > > > > Yes, I'd prefer that too. I assumed the default was 2 in this patch > > because we couldn't set it to 1 for some reason. > > > > I would prefer that too. However, it was set to 2 because it would break > the user space application depending on the legacy behavior as soon as the > patches are upstream. That is the reason > palmer suggested keeping the default value to 2 in order to avoid that. > > +distro folks (cc'd) > If the distro maintainer can confirm that this would be a non-issue, I am okay > with setting the default to 1. > @David Abdurachmanov reminds me of ARM64 code where it is set to zero. The upstream kernel doesn't even enable userspace via perf. The default in x86 is 1 though. +Rob Herring (who enabled the ARM64 support[1]) @Rob: If you can shed some light on the reasoning behind setting to disabled, that would help us make a more informed decision. https://github.com/torvalds/linux/commit/e2012600810c9ded81f6f63a8d04781be3c300ad > > > Thanks, > > drew > > > > -- > Regards, > Atish -- Regards, Atish
