[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240823150236.GA32691@willie-the-truck>
Date: Fri, 23 Aug 2024 16:02:36 +0100
From: Will Deacon <will@...nel.org>
To: James Clark <james.clark@...aro.org>
Cc: linux-arm-kernel@...ts.infradead.org, peterz@...radead.org,
Al Grant <al.grant@....com>, Mark Rutland <mark.rutland@....com>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
Adrian Hunter <adrian.hunter@...el.com>,
"Liang, Kan" <kan.liang@...ux.intel.com>,
linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v2] drivers/perf: arm_spe: Use perf_allow_kernel() for
permissions
On Fri, Aug 16, 2024 at 02:27:19PM +0100, James Clark wrote:
>
>
> On 16/08/2024 1:45 pm, Will Deacon wrote:
> > On Wed, Aug 07, 2024 at 04:51:53PM +0100, James Clark wrote:
> > > For other PMUs, PERF_SAMPLE_PHYS_ADDR requires perf_allow_kernel()
> > > rather than just perfmon_capable(). Because PMSCR_EL1_PA is another form
> > > of physical address, make it consistent and use perf_allow_kernel() for
> > > SPE as well. PMSCR_EL1_PCT and PMSCR_EL1_CX also get the same change.
> > >
> > > This improves consistency and indirectly fixes the following error
> > > message which is misleading because perf_event_paranoid is not taken
> > > into account by perfmon_capable():
> > >
> > > $ perf record -e arm_spe/pa_enable/
> > >
> > > Error:
> > > Access to performance monitoring and observability operations is
> > > limited. Consider adjusting /proc/sys/kernel/perf_event_paranoid
> > > setting ...
> > >
> > > Suggested-by: Al Grant <al.grant@....com>
> > > Signed-off-by: James Clark <james.clark@...aro.org>
> > > ---
> > > Changes since v1:
> > >
> > > * Export perf_allow_kernel() instead of sysctl_perf_event_paranoid
> > >
> > > drivers/perf/arm_spe_pmu.c | 9 ++++-----
> > > include/linux/perf_event.h | 8 +-------
> > > kernel/events/core.c | 9 +++++++++
> > > 3 files changed, 14 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> > > index 9100d82bfabc..3569050f9cf3 100644
> > > --- a/drivers/perf/arm_spe_pmu.c
> > > +++ b/drivers/perf/arm_spe_pmu.c
> > > @@ -41,7 +41,7 @@
> > > /*
> > > * Cache if the event is allowed to trace Context information.
> > > - * This allows us to perform the check, i.e, perfmon_capable(),
> > > + * This allows us to perform the check, i.e, perf_allow_kernel(),
> > > * in the context of the event owner, once, during the event_init().
> > > */
> > > #define SPE_PMU_HW_FLAGS_CX 0x00001
> > > @@ -50,7 +50,7 @@ static_assert((PERF_EVENT_FLAG_ARCH & SPE_PMU_HW_FLAGS_CX) == SPE_PMU_HW_FLAGS_C
> > > static void set_spe_event_has_cx(struct perf_event *event)
> > > {
> > > - if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && perfmon_capable())
> > > + if (IS_ENABLED(CONFIG_PID_IN_CONTEXTIDR) && !perf_allow_kernel(&event->attr))
> > > event->hw.flags |= SPE_PMU_HW_FLAGS_CX;
> >
> > The rationale for this change in the commit message is because other
> > drivers gate PERF_SAMPLE_PHYS_ADDR on perf_allow_kernel(). However,
> > putting the PID in contextidr doesn't seem to have anything to do with
> > that...
> >
>
> That is true, I suppose I was thinking of two reasons to do it this way that
> I didn't really elaborate on:
>
> #1 because context IDs and physical timestamps didn't seem to be any more
> sensitive than physical addresses, so it wouldn't make sense for them to
> have a stricter permissions model than addresses.
>
> #2 (although this is indirect and not really related to the driver) but Perf
> will still print the misleading warning when physical timestamps are
> requested. So some other fix would eventually have to be made for that.
>
> I'm not sure if you are objecting to the permissions change for the other
> two things, or it's just a lack of reasoning in the commit message?
I'm just objecting to the rationale in the commit message not being
applicable to some of the changes made in the code. A much better rationale
to use perf_allow_kernel() is because of its integration with the LSM hooks.
Will
Powered by blists - more mailing lists