Message-ID: <CAP-5=fVttTnWV7KpiaxNjyiDXt9Uf5zZEm4v5V4mGXMyRr6nSg@mail.gmail.com>
Date: Mon, 5 Aug 2024 10:02:04 -0700
From: Ian Rogers <irogers@...gle.com>
To: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
Cc: Eric Lin <eric.lin@...ive.com>, Namhyung Kim <namhyung@...nel.org>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	James Clark <james.clark@....com>, linux-perf-users <linux-perf-users@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, vincent.chen@...ive.com, 
	greentime.hu@...ive.com, Samuel Holland <samuel.holland@...ive.com>
Subject: Re: [PATCH] perf pmus: Fix duplicate events caused segfault

On Mon, Aug 5, 2024 at 7:24 AM Athira Rajeev
<atrajeev@...ux.vnet.ibm.com> wrote:
>
>
>
> > On 4 Aug 2024, at 8:36 PM, Eric Lin <eric.lin@...ive.com> wrote:
> >
> > Hi,
> >
> > On Sun, Jul 21, 2024 at 11:44 PM Eric Lin <eric.lin@...ive.com> wrote:
> >>
> >> Hi Athira,
> >>
> >> On Sat, Jul 20, 2024 at 4:35 PM Athira Rajeev
> >> <atrajeev@...ux.vnet.ibm.com> wrote:
> >>>
> >>>
> >>>
> >>>> On 19 Jul 2024, at 1:46 PM, Eric Lin <eric.lin@...ive.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 ?
> >>>
> >>
> >> Sorry for the late reply.
> >> Yes, I've checked if there are duplicate pmu_cache_miss events in the
> >> JSON files, the perf list will have only one entry in perf list.
> >>
> >>> 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 ?
> >>>
> >>
> >> Yes, I agree we should fix the duplicate events in vendor JSON files.
> >>
> >> According to this code snippet [1], it seems the perf tool originally
> >> allowed duplicate events to exist and it will skip the duplicate
> >> events not shown on the perf list.
> >> However, after this commit e6ff1eed3584 ("perf pmu: Lazily add JSON
> >> events"),  if there are two duplicate events, it causes a segfault.
> >>
> >> Can I ask, do you have any suggestions? Thanks.
> >>
> >> [1] https://github.com/torvalds/linux/blob/master/tools/perf/util/pmus.c#L491
> >>
> >
> > Kindly ping.
> >
> > Can I ask, are there any more comments about this patch? Thanks.
> >
> Hi Eric,
>
> The functions there says alias and to skip duplicate alias. I am not sure if that is for events

Fwiw, I'm trying to get rid of the term alias it should mean event.
For some reason the code always referred to events as aliases as
exemplified by:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.c?h=tmp.perf-tools-next#n55
But it is possible to have an "alias" (different) name for a PMU and
I'm sure for other things too. So the term alias is ambiguous and
these things are events, so let's just call them events to be most
intention revealing.

> Namhyung, Ian, Arnaldo
> Any comments here ?

I'll take a look.

Thanks,
Ian

> Thank
> Athira
>
> >
> > Regards,
> > Eric Lin
> >
> >> Regards,
> >> Eric Lin
> >>
> >>> 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@...ive.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
>
>

Powered by blists - more mailing lists