Re: [PATCH] perf jevent: fix core dump on software events on s390

[Thomas Richter <tmricht@linux.ibm.com>]

On 9/16/23 01:40, Namhyung Kim wrote:
> Hello,
> 
> On Thu, Sep 14, 2023 at 6:14 AM Ian Rogers <irogers@google.com> wrote:
>>
>> On Wed, Sep 13, 2023 at 5:52 AM Thomas Richter <tmricht@linux.ibm.com> wrote:
>>>
>>> Running commands such as
>>>  # ./perf stat -e cs -- true
>>>  Segmentation fault (core dumped)
>>>  # ./perf stat -e cpu-clock-- true
>>>  Segmentation fault (core dumped)
>>>  #
>>>
>>> dump core. This should not happen as these events are defined
>>> even when no hardware PMU is available.
>>> Debugging this reveals this call chain:
>>>
>>>   perf_pmus__find_by_type(type=1)
>>>   +--> pmu_read_sysfs(core_only=false)
>>>        +--> perf_pmu__find2(dirfd=3, name=0x152a113 "software")
>>>             +--> perf_pmu__lookup(pmus=0x14f0568 <other_pmus>, dirfd=3,
>>>                                   lookup_name=0x152a113 "software")
>>>                  +--> perf_pmu__find_events_table (pmu=0x1532130)
>>>
>>> Now the pmu is "software" and it tries to find a proper table
>>> generated by the pmu-event generation process for s390:
>>>
>>>  # cd pmu-events/
>>>  # ./jevents.py  s390 all /root/linux/tools/perf/pmu-events/arch |\
>>>         grep -E '^const struct pmu_table_entry'
>>>  const struct pmu_table_entry pmu_events__cf_z10[] = {
>>>  const struct pmu_table_entry pmu_events__cf_z13[] = {
>>>  const struct pmu_table_entry pmu_metrics__cf_z13[] = {
>>>  const struct pmu_table_entry pmu_events__cf_z14[] = {
>>>  const struct pmu_table_entry pmu_metrics__cf_z14[] = {
>>>  const struct pmu_table_entry pmu_events__cf_z15[] = {
>>>  const struct pmu_table_entry pmu_metrics__cf_z15[] = {
>>>  const struct pmu_table_entry pmu_events__cf_z16[] = {
>>>  const struct pmu_table_entry pmu_metrics__cf_z16[] = {
>>>  const struct pmu_table_entry pmu_events__cf_z196[] = {
>>>  const struct pmu_table_entry pmu_events__cf_zec12[] = {
>>>  const struct pmu_table_entry pmu_metrics__cf_zec12[] = {
>>>  const struct pmu_table_entry pmu_events__test_soc_cpu[] = {
>>>  const struct pmu_table_entry pmu_metrics__test_soc_cpu[] = {
>>>  const struct pmu_table_entry pmu_events__test_soc_sys[] = {
>>>  #
>>>
>>> However event "software" is not listed, as can be seen in the
>>> generated const struct pmu_events_map pmu_events_map[].
>>> So in function perf_pmu__find_events_table(), the variable
>>> table is initialized to NULL, but never set to a proper
>>> value. The function scans all generated &pmu_events_map[]
>>> tables, but no table matches, because the tables are
>>> s390 CPU Measurement unit specific:
>>>
>>>   i = 0;
>>>   for (;;) {
>>>       const struct pmu_events_map *map = &pmu_events_map[i++];
>>>       if (!map->arch)
>>>            break;
>>>
>>>       --> the maps are there because the build generated them
>>>
>>>            if (!strcmp_cpuid_str(map->cpuid, cpuid)) {
>>>                 table = &map->event_table;
>>>                 break;
>>>            }
>>>       --> Since no matching CPU string the table var remains 0x0
>>>       }
>>>       free(cpuid);
>>>       if (!pmu)
>>>            return table;
>>>
>>>       --> The pmu is "software" so it exists and no return
>>>
>>>       --> and here perf dies because table is 0x0
>>>       for (i = 0; i < table->num_pmus; i++) {
>>>               ...
>>>       }
>>>       return NULL;
>>>
>>> Fix this and do not access the table variable. Instead return 0x0
>>> which is the same return code when the for-loop was not successful.
>>>
>>> Output after:
>>>  # ./perf stat -e cs -- true
>>>
>>>  Performance counter stats for 'true':
>>>
>>>                  0      cs
>>>
>>>        0.000853105 seconds time elapsed
>>>
>>>        0.000061000 seconds user
>>>        0.000827000 seconds sys
>>>
>>>  # ./perf stat -e cpu-clock -- true
>>>
>>>  Performance counter stats for 'true':
>>>
>>>               0.25 msec cpu-clock #    0.341 CPUs utilized
>>>
>>>        0.000728383 seconds time elapsed
>>>
>>>        0.000055000 seconds user
>>>        0.000706000 seconds sys
>>>
>>>  # ./perf stat -e cycles -- true
>>>
>>>  Performance counter stats for 'true':
>>>
>>>    <not supported>      cycles
>>>
>>>        0.000767298 seconds time elapsed
>>>
>>>        0.000055000 seconds user
>>>        0.000739000 seconds sys
>>>
>>>  #
>>>
>>> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
>>
>> Reviewed-by: Ian Rogers <irogers@google.com>
> 
> I'll add this too, ok?
> 
> Fixes: 7c52f10c0d4d8 ("perf pmu: Cache JSON events table")
> 
> Thanks,
> Namhyung

Yep fine with me.
-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294