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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPqJEFrkurD9B9smy908Y-z-f6ckv+ZFJzo6ptwXmxD0ru5=CA@mail.gmail.com>
Date: Sun, 21 Jul 2024 23:44:59 +0800
From: Eric Lin <eric.lin@...ive.com>
To: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Ian Rogers <irogers@...gle.com>, 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

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

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