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

[Eric Lin <eric.lin@sifive.com>]

Hi,

On Sun, Jul 21, 2024 at 11:44 PM Eric Lin <eric.lin@sifive.com> wrote:
>
> Hi Athira,
>
> On Sat, Jul 20, 2024 at 4:35 PM Athira Rajeev
> <atrajeev@linux.vnet.ibm.com> wrote:
> >
> >
> >
> > > 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 ?
> >
>
> 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.


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@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
> > >
> > >
> >