linux-kernel - Re: [PATCH v2] drivers/perf: arm_spe: Use perf_allow

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240816124459.GA24323@willie-the-truck>
Date: Fri, 16 Aug 2024 13:45:00 +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 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...

>  }
>  
> @@ -745,9 +745,8 @@ static int arm_spe_pmu_event_init(struct perf_event *event)
>  
>  	set_spe_event_has_cx(event);
>  	reg = arm_spe_event_to_pmscr(event);
> -	if (!perfmon_capable() &&
> -	    (reg & (PMSCR_EL1_PA | PMSCR_EL1_PCT)))
> -		return -EACCES;
> +	if (reg & (PMSCR_EL1_PA | PMSCR_EL1_PCT))

Similarly here. What does the physical counter have to do with physical
address sampling other than sharing the word "physical"?

Will