linux-kernel - Re: [PATCH] perf: arm_spe: Ensure trace is disabled when TRUNCATED flag is set

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] [day] [month] [year] [list]

Message-ID: <20251103171446.GY281971@e132581.arm.com>
Date: Mon, 3 Nov 2025 17:14:46 +0000
From: Leo Yan <leo.yan@....com>
To: Will Deacon <will@...nel.org>
Cc: Mark Rutland <mark.rutland@....com>,
	Alexandru Elisei <alexandru.elisei@....com>,
	James Clark <james.clark@...aro.org>,
	linux-arm-kernel@...ts.infradead.org,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf: arm_spe: Ensure trace is disabled when TRUNCATED
 flag is set

On Mon, Nov 03, 2025 at 02:14:56PM +0000, Will Deacon wrote:

[...]

> > @@ -765,6 +759,15 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
> >  		break;
> >  	}
> >  
> > +	/*
> > +	 * The TRUNCATED flag is set when data loss is detected by PMBSR_EL1.DL,
> > +	 * or arm_spe_perf_aux_output_begin() sets the flag if runs out of free
> > +	 * space. Ensure that all perf callbacks have completed for disabling
> > +	 * the profiling buffer.
> > +	 */
> > +	if (handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)
> > +		irq_work_run();
> 
> I'm not sure about this. afaict, perf_aux_output_begin() can fail
> (return NULL) without setting the TRUNCATED flag but with a disable
> pending in irq work. It also feels a little fragile moving the existing
> irq_work_run() call as that could easily break in future if e.g. irq
> work is used for other manipulation of the buffer controls.
> 
> Given that this isn't a fast path, isn't it simpler and safer just to
> do something like the below?

This is fine for me.  However, arm_spe_perf_aux_output_begin() misses to
set the 'hw.state' if arm_spe_pmu_next_off() fails to calculate a valid
limit. We need an extra fix for this (see the pasted code piece).

As a result, we will have two fixes:

  1) Fix 'hw.state' setting in arm_spe_perf_aux_output_begin();
  2) Invoke irq_work_run() if 'hw.state' is non-zero.

Please let me know if this works for you.  Thanks!

Leo

diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index fa50645fedda..ff7b1447db0a 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -597,7 +597,6 @@ static void arm_spe_perf_aux_output_begin(struct perf_output_handle *handle,
 	/* Start a new aux session */
 	buf = perf_aux_output_begin(handle, event);
 	if (!buf) {
-		event->hw.state |= PERF_HES_STOPPED;
 		/*
 		 * We still need to clear the limit pointer, since the
 		 * profiler might only be disabled by virtue of a fault.
@@ -608,14 +607,18 @@ static void arm_spe_perf_aux_output_begin(struct perf_output_handle *handle,
 
 	limit = buf->snapshot ? arm_spe_pmu_next_snapshot_off(handle)
 			      : arm_spe_pmu_next_off(handle);
-	if (limit)
-		limit |= PMBLIMITR_EL1_E;
+	if (!limit)
+		goto out_write_limit;
 
-	limit += (u64)buf->base;
+	limit |= (u64)buf->base | PMBLIMITR_EL1_E;
 	base = (u64)buf->base + PERF_IDX2OFF(handle->head, buf);
 	write_sysreg_s(base, SYS_PMBPTR_EL1);
 
 out_write_limit:
+	/* Stop tracing due to an invalid limit pointer */
+	if (!limit)
+		event->hw.state |= PERF_HES_STOPPED;
 	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
 }
 

> Will
> 
> --->8
> 
> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index fa50645fedda..8a38d69e9600 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -758,6 +758,10 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev)
>                 if (!(handle->aux_flags & PERF_AUX_FLAG_TRUNCATED)) {
>                         arm_spe_perf_aux_output_begin(handle, event);
>                         isb();
> +
> +                       /* Insightful comment here */
> +                       if (event->hw.state)
> +                               irq_work_run();
>                 }
>                 break;
>         case SPE_PMU_BUF_FAULT_ACT_SPURIOUS:
>