Re: [PATCH v2] drivers/perf: arm_spe: Use perf_allow_kernel() for permissions

[Will Deacon <will@kernel.org>]

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@arm.com>
> > > Signed-off-by: James Clark <james.clark@linaro.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