Re: [PATCH v11 7/9] perf cs-etm: Support call indentation

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Unnecessary branch stack allocation and processing overhead when only callstack tracking is requested

Pre-existing issues:
- [High] NULL pointer dereference when handling allocation failure in cs_etm__etmq_get_traceid_queue()
--

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

perf cs-etm: Support call indentation

The perf script callindent is derived from call stack in thread context.
This patch enables thread-stack when either itrace thread-stack support
or last branch entries are requested, allocating the branch stack storage
accordingly, and feeding taken branches to thread_stack__event().

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

[ ... ]

> @@ -656,7 +658,7 @@ static int cs_etm__init_traceid_queue(struct cs_etm_queue *etmq,
>  	if (!tidq->prev_packet)
>  		goto out_free;
>  
> -	if (etm->synth_opts.last_branch) {
> +	if (etm->use_thread_stack) {
>  		size_t sz = sizeof(struct branch_stack);
>  
>  		sz += etm->synth_opts.last_branch_sz *

[ ... ]

> @@ -1572,7 +1574,7 @@ static void cs_etm__add_stack_event(struct cs_etm_queue *etmq,
>  	if (!cs_etm__packet_has_taken_branch(tidq->prev_packet))
>  		return;
>  
> -	if (etmq->etm->synth_opts.last_branch) {
> +	if (etmq->etm->use_thread_stack) {
>  		from = cs_etm__last_executed_instr(tidq->prev_packet);
>  		to = cs_etm__first_executed_instr(tidq->packet);
>  
> @@ -1581,7 +1583,8 @@ static void cs_etm__add_stack_event(struct cs_etm_queue *etmq,
>  		/* Enable callchain so thread stack entry can be allocated */
>  		thread_stack__event(tidq->frontend_thread, tidq->prev_packet->cpu,
>  				    tidq->prev_packet->flags, from, to, size,
> -				    etmq->buffer->buffer_nr + 1, false,
> +				    etmq->buffer->buffer_nr + 1,
> +				    etmq->etm->use_callchain,
>  				    tidq->br_stack_sz, 0);

[Severity: Medium]
Does this code unnecessarily allocate and process branch stacks when only
call stack tracking is requested?

When callindent is requested without last branch entries,
synth_opts.thread_stack is true while synth_opts.last_branch is false.

Coupling the allocation of tidq->last_branch and tidq->br_stack_sz to
use_thread_stack rather than synth_opts.last_branch seems like it could
cause thread_stack__event() to perform unneeded branch stack ring buffer
updates on every executed branch.

[ ... ]

This is a pre-existing issue, but looking at cs_etm__etmq_get_traceid_queue()
in tools/perf/util/cs-etm.c:

cs_etm__etmq_get_traceid_queue() {
    ...
    inode = intlist__findnew(traceid_queues_list, trace_chan_id);
    if (!inode)
        goto out_free;
    ...
out_free:
    ...
    intlist__remove(traceid_queues_list, inode);
    free(tidq);
    ...
}

[Severity: High]
Can this sequence cause a null pointer dereference?

If memory allocation fails inside intlist__findnew(), it returns null into
inode. The code immediately jumps to the out_free label and calls
intlist__remove() with the null inode.

Since intlist__remove() unconditionally attempts to remove the node, this
looks like it would crash when trying to access the internal rb_node.

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