linux-kernel - Re: [PATCH 2/3] perf: arm_spe: Disable buffer before writing to PMBPTR_EL1 or PMBSR

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: <20250704155016.GI1039028@e132581.arm.com>
Date: Fri, 4 Jul 2025 16:50:16 +0100
From: Leo Yan <leo.yan@....com>
To: James Clark <james.clark@...aro.org>
Cc: Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	Alexandru Elisei <Alexandru.Elisei@....com>,
	Anshuman Khandual <Anshuman.Khandual@....com>,
	Rob Herring <Rob.Herring@....com>,
	Suzuki Poulose <Suzuki.Poulose@....com>,
	Robin Murphy <Robin.Murphy@....com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] perf: arm_spe: Disable buffer before writing to
 PMBPTR_EL1 or PMBSR_EL1

On Tue, Jul 01, 2025 at 04:31:58PM +0100, James Clark wrote:

[...]

> @@ -661,16 +666,24 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
>  	 */
>  	irq_work_run();
>  
> +	/*
> +	 * arm_spe_pmu_buf_get_fault_act() already drained, and PMBSR_EL1.S == 1
> +	 * means that StatisticalProfilingEnabled() == false. So now we can
> +	 * safely disable the buffer.
> +	 */
> +	write_sysreg_s(0, SYS_PMBLIMITR_EL1);
> +	isb();
> +
> +	/* Status can be cleared now that PMBLIMITR_EL1.E == 0 */
> +	write_sysreg_s(0, SYS_PMBSR_EL1);
> +

An important thing is about sequence:
As described in arm_spe_pmu_disable_and_drain_local(), should we always
clear ELs bits in PMSCR_EL1 before clear PMBLIMITR_EL1.E bit? As a
reference, we could see TRBE always clear ELx bits before disable trace
buffer.

And a trivial flaw:

If the TRUNCATED flag has been set, the irq_work_run() above runs the
IRQ work to invoke the arm_spe_pmu_stop() to disable trace buffer, which
clear SYS_PMBLIMITR_EL1.E bit. This is why the current code does not
explictly clear SYS_PMBLIMITR_EL1.E bit.

With this patch, the interrupt handler will clear SYS_PMBLIMITR_EL1.E
bit twice for a trunacated case.


>  	switch (act) {
>  	case SPE_PMU_BUF_FAULT_ACT_FATAL:
>  		/*
> -		 * If a fatal exception occurred then leaving the profiling
> -		 * buffer enabled is a recipe waiting to happen. Since
> -		 * fatal faults don't always imply truncation, make sure
> -		 * that the profiling buffer is disabled explicitly before
> -		 * clearing the syndrome register.
> +		 * To complete the full disable sequence, also disable profiling
> +		 * at EL0 and EL1, we don't want to continue at all anymore.
>  		 */
> -		arm_spe_pmu_disable_and_drain_local();
> +		write_sysreg_s(0, SYS_PMSCR_EL1);
>  		break;
>  	case SPE_PMU_BUF_FAULT_ACT_OK:
>  		/*
> @@ -679,18 +692,14 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
>  		 * PMBPTR might be misaligned, but we'll burn that bridge
>  		 * when we get to it.
>  		 */
> -		if (!(handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)) {
> +		if (!(handle->aux_flags & PERF_AUX_FLAG_TRUNCATED))
>  			arm_spe_perf_aux_output_begin(handle, event);
> -			isb();

I am a bit suspecious we can remove this isb().

As a reference to the software usage PKLXF in Arm ARM (DDI 0487 L.a),
after enable TRBE trace unit, an ISB is mandatory. Maybe check a bit
for this?

Thanks,
Leo

> -		}
>  		break;
>  	case SPE_PMU_BUF_FAULT_ACT_SPURIOUS:
>  		/* We've seen you before, but GCC has the memory of a sieve. */
>  		break;
>  	}
>  
> -	/* The buffer pointers are now sane, so resume profiling. */
> -	write_sysreg_s(0, SYS_PMBSR_EL1);
>  	return IRQ_HANDLED;
>  }
>  
> 
> -- 
> 2.34.1
> 
>