Re: [PATCH 3/3] perf: arm_spe: Add support for a 'software' discard mode

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Writes to control-plane system registers (`SYS_PMBPTR_EL1` and `SYS_PMBLIMITR_EL1`) lack an immediate `isb()` synchronization barrier, violating ARM64 architectural invariants.
- [Medium] User-triggerable allocations for the software discard buffer lack `__GFP_ACCOUNT`, allowing a user to bypass memory cgroup limits.
--

--- Patch [3]: [PATCH 3/3] perf: arm_spe: Add support for a 'software' discard mode ---
commit af3311701234ae1a3b87d9c00cb9f6f3748862b7
Author: James Clark <[email protected]>

perf: arm_spe: Add support for a 'software' discard mode

This patch implements a software discard mode for ARM SPE. It falls back
to writing to a single driver-allocated page and not emitting aux records 
when hardware discard support is unavailable.

> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c

[ ... ]

> +static int arm_spe_pmu_alloc_discard_buf(struct perf_event *event)
> +{
> +	int node = (event->cpu == -1) ? -1 : cpu_to_node(event->cpu);
> +	int nr_pages = ARM_SPE_DISCARD_BUF_SIZE / PAGE_SIZE;
> +	struct arm_spe_pmu_sw_discard_buf *buf;
> +	struct page **pglist;
> +	int i;
> +
> +	if (event->parent) {

[ ... ]

> +		return 0;
> +	}
> +
> +	buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, node);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	buf->page = alloc_pages_node(node, GFP_KERNEL, 0);
> +	if (!buf->page)
> +		goto out_free_buf;
> +
> +	pglist = kvmalloc_array(nr_pages, sizeof(*pglist), GFP_KERNEL);

[Severity: Medium]
Should these GFP_KERNEL allocations include __GFP_ACCOUNT? 

Since this memory is allocated on behalf of a user process creating an 
SPE event, omitting __GFP_ACCOUNT could allow a container to allocate 
kernel memory without being charged, potentially bypassing memory limits.

> +	if (!pglist)
> +		goto out_free_page;

[ ... ]

> @@ -617,10 +698,26 @@ static u64 arm_spe_pmu_next_off(struct perf_output_handle *handle)
> -static void arm_spe_pmu_begin_discard(struct perf_output_handle *handle,
> -				      struct perf_event *event)
> +static void arm_spe_pmu_begin_discard(struct perf_output_handle *handle,
> +				      struct perf_event *event)
>  {
> -	u64 limit;
> +	struct arm_spe_pmu *spe_pmu = to_spe_pmu(event->pmu);
> +	struct arm_spe_pmu_sw_discard_buf *discard_buf = event->pmu_private;
> +	u64 base, limit;
> +
> +	if (spe_pmu->features & SPE_PMU_FEAT_HW_DISCARD) {
> +		limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
> +		limit |= PMBLIMITR_EL1_E;
> +	} else {
> +		base = (u64) discard_buf->base;
> +		limit = base + ARM_SPE_DISCARD_BUF_SIZE;
> +		limit |= PMBLIMITR_EL1_E;
> +		write_sysreg_s(base, SYS_PMBPTR_EL1);
> +
> +		/*
> +		 * Track the event so we can access the discard buffer through
> +		 * event->pmu_private. perf_aux_output_begin() normally handles
> +		 * this, but we can do it manually if that's not being used.
> +		 */
> +		handle->event = event;

[Severity: High]
Does this write to SYS_PMBPTR_EL1 require an immediate isb() synchronization
barrier? 

Without it, the subsequent memory store (handle->event = event) might execute
while the CPU pipeline is operating under an architecturally undefined state.

> +	}
>  
> -	limit = FIELD_PREP(PMBLIMITR_EL1_FM, PMBLIMITR_EL1_FM_DISCARD);
> -	limit |= PMBLIMITR_EL1_E;
> +	write_sysreg_s(limit, SYS_PMBLIMITR_EL1);
> +}

[Severity: High]
Is an isb() barrier also missing after this write to SYS_PMBLIMITR_EL1? 

Because it is at the very end of arm_spe_pmu_begin_discard(), the function
return and any subsequent conditional branches in the caller will execute 
before the pipeline state is synchronized.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3