Re: [PATCH v12 06/33] perf callchain: Don't pass evsel and sample

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

Pre-existing issues:
- [High] Unchecked dereference of `sample->evsel` in callchain resolution triggered by synthesized samples from `jitdump`, leading to a segmentation fault.
--

--- Patch [6]: [PATCH v12 06/33] perf callchain: Don't pass evsel and sample ---
commit 27ec660b6457cfb9bcb1b7bd6fcdf72bb85c9ee4
Author: Ian Rogers <[email protected]>

perf callchain: Don't pass evsel and sample

Change callchain resolve code to not pass an evsel with the sample,
instead just read the evsel from the sample.

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index e76f8c86e62a6..c2e0a99efe97d 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -2778,13 +2778,13 @@ static u64 get_leaf_frame_caller(struct perf_sample *sample,
>  
>  static int thread__resolve_callchain_sample(struct thread *thread,
>  					    struct callchain_cursor *cursor,
> -					    struct evsel *evsel,
>  					    struct perf_sample *sample,
>  					    struct symbol **parent,
>  					    struct addr_location *root_al,
>  					    int max_stack,
>  					    bool symbols)
>  {
> +	struct evsel *evsel = sample->evsel;

[Severity: High]
This is a pre-existing issue, but could this lead to a NULL pointer
dereference when processing synthesized samples from jitdump?

When processing a JIT_CODE_MOVE record, jit_repipe_mmap() in
tools/perf/util/jitdump.c synthesizes a perf_sample using
perf_sample__init(), which leaves sample.evsel as NULL. It then calls
build_id__mark_dso_hit(), which unconditionally passes the sample to
sample__for_each_callchain_node().

The sample is propagated here, where sample->evsel is extracted and
subsequently dereferenced in evsel__has_branch_callstack(evsel) without
a NULL check. This issue existed before this patch, as jit_repipe_mmap()
previously passed a NULL evsel argument directly.

>  	struct branch_stack *branch = sample->branch_stack;
>  	struct branch_entry *entries = perf_sample__branch_entries(sample);
>  	struct ip_callchain *chain = sample->callchain;
[ ... ]
> @@ -2986,10 +2986,11 @@ static int unwind_entry(struct unwind_entry *entry, void *arg)
>  
>  static int thread__resolve_callchain_unwind(struct thread *thread,
>  					    struct callchain_cursor *cursor,
> -					    struct evsel *evsel,
>  					    struct perf_sample *sample,
>  					    int max_stack, bool symbols)
>  {
> +	struct evsel *evsel = sample->evsel;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this share the same
risk as thread__resolve_callchain_sample() above?

The extracted evsel is blindly dereferenced immediately on the next line
to access evsel->core.attr.sample_type. A synthesized sample from jitdump
without a set evsel would cause a segmentation fault here as well.

>  	/* Can we do dwarf post unwind? */
>  	if (!((evsel->core.attr.sample_type & PERF_SAMPLE_REGS_USER) &&
>  	      (evsel->core.attr.sample_type & PERF_SAMPLE_STACK_USER)))

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