diff mbox series

perf jevent: fix core dump on software events on s390

Message ID	20230913125157.2790375-1-tmricht@linux.ibm.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3; From: Thomas Richter <tmricht@linux.ibm.com> To: linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, acme@kernel.org, sumanthk@linux.ibm.com, irogers@google.com, dengler@linux.ibm.com Cc: svens@linux.ibm.com, gor@linux.ibm.com, hca@linux.ibm.com, Thomas Richter <tmricht@linux.ibm.com> Subject: [PATCH] perf jevent: fix core dump on software events on s390 Date: Wed, 13 Sep 2023 14:51:57 +0200 Message-Id: <20230913125157.2790375-1-tmricht@linux.ibm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	perf jevent: fix core dump on software events on s390 \| perf jevent: fix core dump on software events on s390

Commit Message

Thomas Richter Sept. 13, 2023, 12:51 p.m. UTC

  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>
---
 tools/perf/pmu-events/jevents.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ian Rogers Sept. 13, 2023, 4:31 p.m. UTC | #1

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>

Thanks!
Ian

> ---
>  tools/perf/pmu-events/jevents.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index a7e88332276d..72ba4a9239c6 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
> @@ -991,7 +991,7 @@ const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu)
>                  }
>          }
>          free(cpuid);
> -        if (!pmu)
> +        if (!pmu || !table)
>                  return table;
>
>          for (i = 0; i < table->num_pmus; i++) {
> --
> 2.41.0
>

Namhyung Kim Sept. 15, 2023, 11:40 p.m. UTC | #2

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

Ian Rogers Sept. 16, 2023, 4:10 a.m. UTC | #3

On Fri, Sep 15, 2023 at 4:40 PM Namhyung Kim <namhyung@gmail.com> 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")

Looks good, thanks!
Ian

> Thanks,
> Namhyung

Namhyung Kim Sept. 17, 2023, 5:28 a.m. UTC | #4

On Fri, Sep 15, 2023 at 9:11 PM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Sep 15, 2023 at 4:40 PM Namhyung Kim <namhyung@gmail.com> 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")
>
> Looks good, thanks!
> Ian

Applied to perf-tools, thanks!

Namhyung

Thomas Richter Sept. 18, 2023, 8:28 a.m. UTC | #5

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.

diff mbox series

Patch

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index a7e88332276d..72ba4a9239c6 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -991,7 +991,7 @@  const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu)
                 }
         }
         free(cpuid);
-        if (!pmu)
+        if (!pmu || !table)
                 return table;
 
         for (i = 0; i < table->num_pmus; i++) {