linux-kernel - Re: [RFC V1 08/11] arm64/perf: Enable branch stack sampling

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 24 Jan 2022 18:02:51 +0000
From:   James Clark <james.clark@....com>
To:     Anshuman Khandual <anshuman.khandual@....com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        linux-perf-users@...r.kernel.org
Subject: Re: [RFC V1 08/11] arm64/perf: Enable branch stack sampling



On 24/01/2022 04:30, Anshuman Khandual wrote:
> Now that all the required pieces are already in place, just enable the perf
> branch stack sampling support on arm64 platform, by removing the gate which
> blocks it in armpmu_event_init().
> 
> Cc: Mark Rutland <mark.rutland@....com>
> Cc: Will Deacon <will@...nel.org>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: linux-kernel@...r.kernel.org
> Cc: linux-arm-kernel@...ts.infradead.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
> ---
>  drivers/perf/arm_pmu.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index 0800c8858ed8..2117bf5d3232 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -537,9 +537,28 @@ static int armpmu_event_init(struct perf_event *event)
>  		!cpumask_test_cpu(event->cpu, &armpmu->supported_cpus))
>  		return -ENOENT;
>  
> -	/* does not support taken branch sampling */
> -	if (has_branch_stack(event))
> -		return -EOPNOTSUPP;
> +	if (has_branch_stack(event)) {
> +		/*
> +		 * BRBE support is absent. Select CONFIG_ARM_BRBE_PMU
> +		 * in the config, before branch stack sampling events
> +		 * can be requested.
> +		 */
> +		if (!IS_ENABLED(CONFIG_ARM_BRBE_PMU)) {
> +			pr_warn_once("BRBE is disabled, select CONFIG_ARM_BRBE_PMU\n");
> +			return -EOPNOTSUPP;
> +		}
> +
> +		/*
> +		 * Branch stack sampling event can not be supported in
> +		 * case either the required driver itself is absent or
> +		 * BRBE buffer, is not supported. Besides checking for
> +		 * the callback prevents a crash in case it's absent.
> +		 */
> +		if (!armpmu->brbe_supported || !armpmu->brbe_supported(event)) {
> +			pr_warn_once("BRBE is not supported\n");
> +			return -EOPNOTSUPP;

brbe_supported() returns false for one permission case, rather than a "not supported" case
so EOPNOTSUPP is the wrong thing to return here for this case otherwise it makes perf
print confusing error messages. 

The brbe_supported() function needs to be split into two, one that handles support and one
that handles permissions and different errors need to be reported. Here is the permission
bit from that function:

  +	if (event->attr.branch_sample_type & PERF_SAMPLE_BRANCH_KERNEL) {
  +		if (!perfmon_capable()) {
  +			pr_warn_once("does not have permission for kernel branch filter\n");
  +			return false;
  +		}
  +	}

> +		}
> +	}
>  
>  	if (armpmu->map_event(event) == -ENOENT)
>  		return -ENOENT;
>