Re: [PATCH v11 8/9] perf cs-etm: Synthesize callchains for instruction samples

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] NULL pointer dereference when cs_etm__context() fails on a trace channel.
- [Low] Missing explicit POSIX libc header inclusion (`<string.h>`) for `memset`, violating subsystem musl compatibility guidelines.
--

commit 962c8fcda00377c91b3a6ef8a062825a14d33107
Author: Leo Yan <leo.yan@linux.dev>

perf cs-etm: Synthesize callchains for instruction samples

This commit adds support for synthesizing callchains for instruction samples
in CS ETM. It allocates a callchain buffer per queue and uses
thread_stack__sample() when synthesizing instruction samples. It also
introduces cs_etm__context() for handling context packets.

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index f76648fa67681..95e3ec1171acc 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c

[ ... ]

> @@ -1602,6 +1616,26 @@ static void cs_etm__sample_branch_stack(struct cs_etm_auxtrace *etm,
>  					tidq->last_branch, tidq->br_stack_sz);
>  		sample->branch_stack = tidq->last_branch;
>  	}
> +
> +	if (etm->synth_opts.callchain) {
> +		if (tidq->kernel_start)
> +			thread_stack__sample(tidq->frontend_thread,
> +					     tidq->packet->cpu,
> +					     tidq->callchain,
> +					     etm->synth_opts.callchain_sz + 1,
> +					     sample->ip, tidq->kernel_start);
> +		else
> +			/*
> +			 * Clear the callchain when the kernel start address is
> +			 * not available yet. The empty callchain can then be
> +			 * consumed by cs_etm__inject_event().
> +			 */
> +			memset(tidq->callchain, 0,
> +			       struct_size(tidq->callchain, ips,
> +					   etm->synth_opts.callchain_sz + 1));

[Severity: Low]
Does this new use of memset() require an explicit inclusion of <string.h>?

The subsystem musl compatibility guidelines state that all POSIX libc functions
must have explicit header inclusions to prevent compilation failures on strict
libc implementations like musl.

> +
> +		sample->callchain = tidq->callchain;
> +	}
>  }

[ ... ]

> @@ -1931,6 +1968,34 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
>  	return 0;
>  }
>  
> +static int cs_etm__context(struct cs_etm_queue *etmq,
> +			   struct cs_etm_traceid_queue *tidq)
> +{
> +	ocsd_ex_level el = tidq->packet->el;
> +	struct machine *machine;
> +	int ret;
> +
> +	machine = cs_etm__get_machine(etmq, el);
> +	if (!machine) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	tidq->kernel_start = machine__kernel_start(machine);
> +
> +	ret = cs_etm__etmq_update_thread(etmq, el, tidq->packet->tid,
> +					 &tidq->frontend_thread);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	thread__zput(tidq->frontend_thread);
> +	tidq->kernel_start = 0;
> +	return ret;

[Severity: Medium]
If cs_etm__context() fails (for example, if cs_etm__get_machine() cannot find
a machine context), the error path sets tidq->frontend_thread to NULL via
thread__zput().

If the caller cs_etm__process_traceid_queue() returns this error, but the
outer decoding loop (like cs_etm__run_per_cpu_timeless_decoder()) ignores
the error and continues, could the next CS_ETM_RANGE packet trigger a NULL
pointer dereference?

Specifically, cs_etm__synth_instruction_sample() unconditionally accesses
the now-NULL thread pointer:

	sample.pid = thread__pid(tidq->frontend_thread);
	sample.tid = thread__tid(tidq->frontend_thread);

Would it be safer to ensure tidq->frontend_thread is not accessed if it is
NULL, or avoid continuing trace processing when the context fails?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-b4-arm_cs_callchain_support_v1-v11-0-846abbf600d7@arm.com?part=8