Re: [PATCH 1/2] perf cs-etm: Queue context packets for frontend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] CS_ETM_CONTEXT packets fail to act as branch sample boundaries because they are not swapped into prev_packet, causing dead code, missing boundaries, and misattributed samples.
- [High] Helper functions do not handle CS_ETM_CONTEXT, which would lead to invalid address reads and leakage of CS_ETM_INVAL_ADDR if context packets were correctly processed as boundaries.

Pre-existing issues:
- [High] Dereferencing a potentially NULL thread pointer during memory access under memory pressure.
--

commit a4e7b7904659c1a9053158e048dc4b6daf7453f5
Author: Leo Yan <leo.yan@arm.com>

perf cs-etm: Queue context packets for frontend

Add a CS_ETM_CONTEXT packet carrying the TID and EL to the frontend to keep
context changes ordered. Separate memory access functions for decoding and
frontend. Treat context packets as boundaries for branch sample generation.

Reported-by: Amir Ayupov <aaupov@meta.com>
Closes: https://lore.kernel.org/linux-perf-users/20260515021135.1729028-1-aaupov@meta.com/

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c

[ ... ]

> @@ -1083,9 +1083,10 @@ static u8 cs_etm__cpu_mode(struct cs_etm_queue *etmq, u64 address,
>  	}
>  }
>  
> -static u32 cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
> -			      u64 address, size_t size, u8 *buffer,
> -			      const ocsd_mem_space_acc_t mem_space)
> +static u32 __cs_etm__mem_access(struct cs_etm_queue *etmq, u8 trace_chan_id,
> +				u64 address, size_t size, u8 *buffer,
> +				const ocsd_mem_space_acc_t mem_space,
> +				bool is_decoder)
>  {

[ ... ]

> +	if (is_decoder) {
> +		el = tidq->decode_el;
> +		thread = tidq->decode_thread;
> +	} else {
> +		el = tidq->frontend_el;
> +		thread = tidq->frontend_thread;
> +	}

[ ... ]

>  	map__load(al.map);
>  
> -	len = dso__data_read_offset(dso, maps__machine(thread__maps(tidq->thread)),
> +	len = dso__data_read_offset(dso, maps__machine(thread__maps(thread)),
>  				    offset, buffer, size);

[Severity: High]
This isn't a bug introduced by this patch, but if the fallback allocation in
machine__idle_thread() fails under memory pressure and returns a NULL pointer,
could thread__maps(thread) cause a null pointer dereference here?

[ ... ]

> @@ -1874,7 +1920,8 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
>  		bool generate_sample = false;
>  
>  		/* Generate sample for tracing on packet */
> -		if (tidq->prev_packet->sample_type == CS_ETM_DISCONTINUITY)
> +		if (tidq->prev_packet->sample_type == CS_ETM_DISCONTINUITY ||
> +		    tidq->prev_packet->sample_type == CS_ETM_CONTEXT)
>  			generate_sample = true;

[Severity: High]
If a context packet were correctly swapped into prev_packet, generate_sample
would be true and cs_etm__synth_branch_sample() would be called.

Looking at cs_etm__synth_branch_sample(), it calls
cs_etm__last_executed_instr(tidq->prev_packet), which doesn't seem to handle
context packets and would return CS_ETM_INVAL_ADDR. Additionally,
cs_etm__copy_insn() doesn't appear to exempt context packets.

Could this lead to invalid address reads and out-of-bounds memory accesses?
Do we need to add exemptions for context packets in these helper functions
similar to how we handle discontinuity packets?

[ ... ]

> @@ -2451,6 +2499,9 @@ static int cs_etm__process_traceid_queue(struct cs_etm_queue *etmq,
>  			 */
>  			cs_etm__sample(etmq, tidq);
>  			break;
> +		case CS_ETM_CONTEXT:
> +			cs_etm__etmq_set_frontend_tid_el(etmq, tidq);
> +			break;
>  		case CS_ETM_EXCEPTION:
>  		case CS_ETM_EXCEPTION_RET:

[Severity: High]
Because the context packet case breaks out of the switch here, we never reach
the cs_etm__packet_swap() call at the end of cs_etm__sample().

Does this mean the context packet is overwritten on the next iteration and
tidq->prev_packet never actually becomes a context packet? If so, this would
make the new boundary check in cs_etm__sample() unreachable dead code.
Should context packets also trigger a packet swap?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-james-cs-context-tracking-fix-v1-0-ebd602e18287@linaro.org?part=1