Re: [PATCH v1] perf stat: Fix aggr mode initialization

[Namhyung Kim]

Hi Ian,

On Tue, Sep 5, 2023 at 5:39 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> Generating metrics llc_code_read_mpi_demand_plus_prefetch,
> llc_data_read_mpi_demand_plus_prefetch,
> llc_miss_local_memory_bandwidth_read,
> llc_miss_local_memory_bandwidth_write,
> nllc_miss_remote_memory_bandwidth_read, memory_bandwidth_read,
> memory_bandwidth_write, uncore_frequency, upi_data_transmit_bw,
> C2_Pkg_Residency, C3_Core_Residency, C3_Pkg_Residency,
> C6_Core_Residency, C6_Pkg_Residency, C7_Core_Residency,
> C7_Pkg_Residency, UNCORE_FREQ and tma_info_system_socket_clks would
> trigger an address sanitizer heap-buffer-overflows on a SkylakeX.
>
> ```
> ==2567752==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x5020003ed098 at pc 0x5621a816654e bp 0x7fffb55d4da0 sp 0x7fffb55d4d98
> READ of size 4 at 0x5020003eee78 thread T0
>     #0 0x558265d6654d in aggr_cpu_id__is_empty tools/perf/util/cpumap.c:694:12
>     #1 0x558265c914da in perf_stat__get_aggr tools/perf/builtin-stat.c:1490:6
>     #2 0x558265c914da in perf_stat__get_global_cached tools/perf/builtin-stat.c:1530:9
>     #3 0x558265e53290 in should_skip_zero_counter tools/perf/util/stat-display.c:947:31
>     #4 0x558265e53290 in print_counter_aggrdata tools/perf/util/stat-display.c:985:18
>     #5 0x558265e51931 in print_counter tools/perf/util/stat-display.c:1110:3
>     #6 0x558265e51931 in evlist__print_counters tools/perf/util/stat-display.c:1571:5
>     #7 0x558265c8ec87 in print_counters tools/perf/builtin-stat.c:981:2
>     #8 0x558265c8cc71 in cmd_stat tools/perf/builtin-stat.c:2837:3
>     #9 0x558265bb9bd4 in run_builtin tools/perf/perf.c:323:11
>     #10 0x558265bb98eb in handle_internal_command tools/perf/perf.c:377:8
>     #11 0x558265bb9389 in run_argv tools/perf/perf.c:421:2
>     #12 0x558265bb9389 in main tools/perf/perf.c:537:3
> ```
>
> The issue was the use of testing a cpumap with NULL rather than using
> empty, as a map containing the dummy value isn't NULL and the -1
> results in an empty aggr map being allocated which legitimately
> overflows when any member is accessed.

Right, evlist__create_maps() set it to non-NULL.

>
> Fixes: 8a96f454f566 ("perf stat: Avoid SEGV if core.cpus isn't set")
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>

Acked-by: Namhyung Kim <namhyung@xxxxxxxxxx>

Thanks,
Namhyung


> ---
>  tools/perf/builtin-stat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 07b48f6df48e..a3af805a1d57 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1622,7 +1622,7 @@ static int perf_stat_init_aggr_mode(void)
>          * taking the highest cpu number to be the size of
>          * the aggregation translate cpumap.
>          */
> -       if (evsel_list->core.user_requested_cpus)
> +       if (!perf_cpu_map__empty(evsel_list->core.user_requested_cpus))
>                 nr = perf_cpu_map__max(evsel_list->core.user_requested_cpus).cpu;
>         else
>                 nr = 0;
> --
> 2.42.0.283.g2d96d420d3-goog
>