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. Thanks, drew
