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: <CAP-5=fU13UYkXZVOcJP+xiRLhDDRycJn=P1wnJi4KnB9KGdpkQ@mail.gmail.com>
Date: Mon, 5 Aug 2024 12:48:23 -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 10:02 AM Ian Rogers <irogers@...gle.com> wrote:
>
> 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.

The problematic events all come from copy pasting ArchStdEvent. It
feels better to have an invariant that events appear once so I sent
out a series to clean this up:
https://lore.kernel.org/linux-perf-users/20240805194424.597244-1-irogers@google.com/
If you could test and add a tested-by tag that'd be great!

Thanks,
Ian