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: <BB8E0B26-4E5F-416F-B8D1-AC745F141383@linux.vnet.ibm.com>
Date: Mon, 5 Aug 2024 19:54:33 +0530
From: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
To: Eric Lin <eric.lin@...ive.com>, Ian Rogers <irogers@...gle.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: 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 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

Namhyung, Ian, Arnaldo
Any comments here ?

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