Re: [PATCH] perf top & record: Fix segfault when default cycles event is not supported

[Yang Jihong]

Hello,

On 2023/6/15 0:18, Ian Rogers wrote:
> On Wed, Jun 14, 2023 at 8:18 AM Yang Jihong <yangjihong1@xxxxxxxxxx> wrote:
> > The perf-record and perf-top call parse_event() to add a cycles event to
> > an empty evlist. For the system that does not support hardware cycles
> > event, such as QEMU, the evlist is empty due to the following code process:
> >
> >    parse_event(evlist, "cycles:P" or ""cycles:Pu")
> >      parse_events(evlist, "cycles:P")
> >        __parse_events
> >          ...
> >          ret = parse_events__scanner(str, &parse_state);
> >          // ret = 0
> >          ...
> >          ret2 = parse_events__sort_events_and_fix_groups()
> >          if (ret2 < 0)
> >            return ret;
> >          // The cycles event is not supported, here ret2 = -EINVAL,
> >          // Here return 0.
> >          ...
> >          evlist__splice_list_tail(evlist)
> >          // The code here does not execute to, so the evlist is still empty.
> >
> > A null pointer occurs when the content in the evlist is accessed later.
> >
> > Before:
> >
> >    # perf list hw
> >
> >    List of pre-defined events (to be used in -e or -M):
> >
> >    # perf record true
> >    libperf: Miscounted nr_mmaps 0 vs 1
> >    WARNING: No sample_id_all support, falling back to unordered processing
> >    perf: Segmentation fault
> >    Obtained 1 stack frames.
> >    [0xc5beff]
> >    Segmentation fault
> >
> > Solution:
> >    If cycles event is not supported, try to fall back to cpu-clock event.
> >
> > After:
> >    # perf record true
> >    [ perf record: Woken up 1 times to write data ]
> >    [ perf record: Captured and wrote 0.006 MB perf.data ]
> >    #
> >
> > Fixes: 7b100989b4f6 ("perf evlist: Remove __evlist__add_default")
> > Signed-off-by: Yang Jihong <yangjihong1@xxxxxxxxxx>
>
> Thanks, useful addition. The cpu-clock fall back wasn't present before
> 7b100989b4f6 so is the fixes tag correct?
Before 7b100989b4f6, perf-record call evlist__add_default() to create an 
evsel and directly add it to the evlist, it does not search for the 
corresponding PMU in the sysfs. Therefore, the evlist is not empty 
before the commit 7b100989b4f6.

> Wrt segv, I'm beginning to think that we should always forcibly create
> a core PMU even if we can't find one one in sysfs, my guess is that is
> what triggers the segv.

Yes, that's the reason.

Thanks,
Yang.

> evlist__add_default doesn't really explain what the function is doing
> and default can have >1 meaning. Perhaps, evlist__add_cycles.
>
> Thanks,
> Ian
>
> > ---
> >   tools/perf/builtin-record.c |  4 +---
> >   tools/perf/builtin-top.c    |  3 +--
> >   tools/perf/util/evlist.c    | 18 ++++++++++++++++++
> >   tools/perf/util/evlist.h    |  1 +
> >   4 files changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index aec18db7ff23..29ae2b84a63a 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -4161,9 +4161,7 @@ int cmd_record(int argc, const char **argv)
> >                  record.opts.tail_synthesize = true;
> >
> >          if (rec->evlist->core.nr_entries == 0) {
> > -               bool can_profile_kernel = perf_event_paranoid_check(1);
> > -
> > -               err = parse_event(rec->evlist, can_profile_kernel ? "cycles:P" : "cycles:Pu");
> > +               err = evlist__add_default(rec->evlist);
> >                  if (err)
> >                          goto out;
> >          }
> > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> > index c363c04e16df..798cb9252a5f 100644
> > --- a/tools/perf/builtin-top.c
> > +++ b/tools/perf/builtin-top.c
> > @@ -1665,8 +1665,7 @@ int cmd_top(int argc, const char **argv)
> >                  goto out_delete_evlist;
> >
> >          if (!top.evlist->core.nr_entries) {
> > -               bool can_profile_kernel = perf_event_paranoid_check(1);
> > -               int err = parse_event(top.evlist, can_profile_kernel ? "cycles:P" : "cycles:Pu");
> > +               int err = evlist__add_default(top.evlist);
> >
> >                  if (err)
> >                          goto out_delete_evlist;
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index 7ef43f72098e..60efa762405e 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -287,6 +287,24 @@ struct evsel *evlist__add_aux_dummy(struct evlist *evlist, bool system_wide)
> >          return evsel;
> >   }
> >
> > +int evlist__add_default(struct evlist *evlist)
> > +{
> > +       bool can_profile_kernel;
> > +       int err;
> > +
> > +       can_profile_kernel = perf_event_paranoid_check(1);
> > +       err = parse_event(evlist, can_profile_kernel ? "cycles:P" : "cycles:Pu");
> > +       if (err)
> > +               return err;
> > +
> > +       if (!evlist->core.nr_entries) {
> > +               pr_debug("The cycles event is not supported, trying to fall back to cpu-clock event\n");
> > +               return parse_event(evlist, "cpu-clock");
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >   #ifdef HAVE_LIBTRACEEVENT
> >   struct evsel *evlist__add_sched_switch(struct evlist *evlist, bool system_wide)
> >   {
> > diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> > index 664c6bf7b3e0..47eea809ee91 100644
> > --- a/tools/perf/util/evlist.h
> > +++ b/tools/perf/util/evlist.h
> > @@ -116,6 +116,7 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs);
> >
> >   int evlist__add_dummy(struct evlist *evlist);
> >   struct evsel *evlist__add_aux_dummy(struct evlist *evlist, bool system_wide);
> > +int evlist__add_default(struct evlist *evlist);
> >   static inline struct evsel *evlist__add_dummy_on_all_cpus(struct evlist *evlist)
> >   {
> >          return evlist__add_aux_dummy(evlist, true);
> > --
> > 2.30.GIT
> .