Re: [PATCH] perf pmus: Fix duplicate events caused segfault

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

> On 19 Jul 2024, at 1:46 PM, Eric Lin <eric.lin@sifive.com> wrote:
> 
> Currently, if vendor JSON files have two duplicate event names,
> the "perf list" command will trigger a segfault.
> 
> In commit e6ff1eed3584 ("perf pmu: Lazily add JSON events"),
> pmu_events_table__num_events() gets the number of JSON events
> from table_pmu->num_entries, which includes duplicate events
> if there are duplicate event names in the JSON files.

Hi Eric,

Let us consider there are duplicate event names in the JSON files, say :

metric.json with: EventName as pmu_cache_miss, EventCode as 0x1
cache.json with:  EventName as pmu_cache_miss, EventCode as 0x2

If we fix the segfault and proceed, still “perf list” will list only one entry for pmu_cache_miss with may be 0x1/0x2 as event code ?
Can you check the result to confirm what “perf list” will list in this case ? If it’s going to have only one entry in perf list, does it mean there are two event codes for pmu_cache_miss and it can work with either of the event code ?

If it happens to be a mistake in json file to have duplicate entry with different event code (ex: with some broken commit), I am thinking if the better fix is to keep only the valid entry in json file ?

Thanks
Athira

> 
> perf_pmu__for_each_event() adds JSON events to the pmu->alias
> list and copies sevent data to the aliases array. However, the
> pmu->alias list does not contain duplicate events, as
> perf_pmu__new_alias() checks if the name was already created.
> 
> When sorting the alias data, if there are two duplicate events,
> it causes a segfault in cmp_sevent() due to invalid aliases in
> the aliases array.
> 
> To avoid such segfault caused by duplicate event names in the
> JSON files, the len should be updated before sorting the aliases.
> 
> Fixes: e6ff1eed3584 ("perf pmu: Lazily add JSON events")
> Signed-off-by: Eric Lin <eric.lin@sifive.com>
> ---
> tools/perf/util/pmus.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index b9b4c5eb5002..e38c3fd4d1ff 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -443,7 +443,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> {
> struct perf_pmu *pmu;
> int printed = 0;
> - int len;
> + size_t len, j;
> struct sevent *aliases;
> struct events_callback_state state;
> bool skip_duplicate_pmus = print_cb->skip_duplicate_pmus(print_state);
> @@ -474,8 +474,9 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> perf_pmu__for_each_event(pmu, skip_duplicate_pmus, &state,
> perf_pmus__print_pmu_events__callback);
> }
> + len = state.index;
> qsort(aliases, len, sizeof(struct sevent), cmp_sevent);
> - for (int j = 0; j < len; j++) {
> + for (j = 0; j < len; j++) {
> /* Skip duplicates */
> if (j > 0 && pmu_alias_is_duplicate(&aliases[j], &aliases[j - 1]))
> continue;
> -- 
> 2.43.2
> 
>