linux-kernel - Re: [PATCH] perf evlist: Avoid iteration for empty evlist.

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=fUkgp0G+Hj8fnO0RdMdbcKMesEHHE5SgS5OPpj_iW_D9w@mail.gmail.com>
Date:   Wed, 16 Mar 2022 13:16:30 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Jiri Olsa <olsajiri@...il.com>
Cc:     Namhyung Kim <namhyung@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] perf evlist: Avoid iteration for empty evlist.

On Wed, Mar 16, 2022 at 7:08 AM Jiri Olsa <olsajiri@...il.com> wrote:
>
> On Wed, Mar 16, 2022 at 12:10:49AM -0700, Ian Rogers wrote:
> > As seen with 'perf stat --null ..' and reported in:
> > https://lore.kernel.org/lkml/YjCLcpcX2peeQVCH@kernel.org/
> >
> > Fixes: 472832d2c000 ("perf evlist: Refactor evlist__for_each_cpu()")
> > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > ---
> >  tools/perf/util/evlist.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index 8134d45e2164..a2dba9e00765 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -354,7 +354,10 @@ struct evlist_cpu_iterator evlist__cpu_begin(struct evlist *evlist, struct affin
> >               .affinity = affinity,
> >       };
> >
> > -     if (itr.affinity) {
> > +     if (evlist__empty(evlist)) {
> > +             /* Ensure the empty list doesn't iterate. */
> > +             itr.evlist_cpu_map_idx = itr.evlist_cpu_map_nr;
>
> I can't see the crash anymore, but I'm bit confused with the code
>
> if evlist is empty then itr.evsel is bogus.. and the loop code
> __run_perf_stat is just lucky, right?

The itr.evsel is the list head, so bogus.

> I think we need to set itr.evsel to NULL and skip the loop in
> case evlist is empty

So that's the effect of this change except that the evsel is the list
head. I'm not sure it is worth adding the condition to setting the
evsel to capture that given that with the end condition having been
met it would be invalid to read it. There's a similar problem for the
other fields of the iterator.

Thanks,
Ian

> thanks,
> jirka
>
> > +     } else if (itr.affinity) {
> >               itr.cpu = perf_cpu_map__cpu(evlist->core.all_cpus, 0);
> >               affinity__set(itr.affinity, itr.cpu.cpu);
> >               itr.cpu_map_idx = perf_cpu_map__idx(itr.evsel->core.cpus, itr.cpu);
> > --
> > 2.35.1.723.g4982287a31-goog
> >