linux-kernel - Re: [PATCH 7/7] perf inject: Remove stale build-id processing

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: <CAM9d7cg3f_J6QjgZzgcpK46C5Pgpp1KUuMoKSJSK+CqoUx6nCQ@mail.gmail.com>
Date:   Thu, 24 Sep 2020 23:23:11 +0900
From:   Namhyung Kim <namhyung@...nel.org>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Stephane Eranian <eranian@...gle.com>,
        Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>
Subject: Re: [PATCH 7/7] perf inject: Remove stale build-id processing

On Thu, Sep 24, 2020 at 10:34 PM Jiri Olsa <jolsa@...hat.com> wrote:
>
> On Wed, Sep 23, 2020 at 05:05:37PM +0900, Namhyung Kim wrote:
> > I think we don't need to call build_id__mark_dso_hit() in the
> > perf_event__repipe_sample() as it's not used by -b option.  In case of
> > the -b option is used, it uses perf_event__inject_buildid() instead.
> > This can remove unnecessary overhead of finding thread/map for each
> > sample event.
> >
> > Also I suspect HEADER_BUILD_ID feature bit setting since we already
> > generated/injected BUILD_ID event into the output stream.  So this
> > header information seems redundant.  I'm not 100% sure about the
> > auxtrace usage, but it looks like not related to this directly.
> >
> > And we now have --buildid-all so users can get the same behavior if
> > they want.
> >
> > Cc: Adrian Hunter <adrian.hunter@...el.com>
> > Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> > ---
> >  tools/perf/builtin-inject.c | 12 ------------
> >  1 file changed, 12 deletions(-)
> >
> > diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> > index 500428aaa576..0191d72be7c4 100644
> > --- a/tools/perf/builtin-inject.c
> > +++ b/tools/perf/builtin-inject.c
> > @@ -277,8 +277,6 @@ static int perf_event__repipe_sample(struct perf_tool *tool,
> >               return f(tool, event, sample, evsel, machine);
> >       }
> >
> > -     build_id__mark_dso_hit(tool, event, sample, evsel, machine);
> > -
>
> I recalled using simple 'perf inject -i .. -o .. ' to get uncompressed data
> from 'perf record -z' and I though this change will force inject not to store
> all build ids ... but it's happening even without your change ;-)

I wasn't aware of that use case.  Will check..

Anyway, basically perf record saves build-id in the file header unless -B
option is used.  I think we don't need to update feature data if user didn't
request something explicit (maybe like when -b or --itrace is used).

Now I've realized that current code always updates the feature data
including build-id so it needs to mark dso.  But I think it's meaningless
because 1) if the file already has build-id feature no need to do the
marking again, or 2) if the input is a pipe we need -b option and
then the marking code won't run.

Maybe the only case we are concerned about is that the data file
doesn't have a build-id feature and we don't want to inject build-id
events but want to update the build-id feature data in the header.
I'm not sure if it's a good practice, but if we really want to support
it, I think we should add a dedicated option.

>
>         $ ./perf record ls
>         ...
>         [ perf record: Woken up 1 times to write data ]
>         [ perf record: Captured and wrote 0.016 MB perf.data (15 samples) ]
>
>         $ ./perf inject -o perf.data.new -i perf.data
>
>         $ ./perf buildid-list
>
>         17f4e448cc746582ea1881528deb549f7fdb3fd5 [kernel.kallsyms]
>         b516839521ded07bb1fbd0a0276be9820ee8908e /usr/bin/ls
>         1805c738c8f3ec0f47b7ea09080c28f34d18a82b /usr/lib64/ld-2.31.so
>         f22785ea7e42e8aa9097a567a3cc8ae214cae4b6 [vdso]
>         d278249792061c6b74d1693ca59513be1def13f2 /usr/lib64/libc-2.31.so
>
>         $ ./perf buildid-list -i perf.data.new
>         f22785ea7e42e8aa9097a567a3cc8ae214cae4b6 [vdso]

Oh... I found that the current code doesn't handle mmap events
unless the -b (or other) option is used.  So even if it tries to
mark dso it couldn't find one.  But vdso is created separately
so you can see it only IMHO.

Yes, we need to fix this.. but I'm not sure what's the meaning
of running perf inject without any option. :)

Thanks
Namhyung

>
> jirka
>
>
> >       if (inject->itrace_synth_opts.set && sample->aux_sample.size)
> >               event = perf_inject__cut_auxtrace_sample(inject, event, sample);
> >
> > @@ -767,16 +765,6 @@ static int __cmd_inject(struct perf_inject *inject)
> >               return ret;
> >
> >       if (!data_out->is_pipe) {
> > -             if (inject->build_ids)
> > -                     perf_header__set_feat(&session->header,
> > -                                           HEADER_BUILD_ID);
> > -             /*
> > -              * Keep all buildids when there is unprocessed AUX data because
> > -              * it is not known which ones the AUX trace hits.
> > -              */
> > -             if (perf_header__has_feat(&session->header, HEADER_BUILD_ID) &&
> > -                 inject->have_auxtrace && !inject->itrace_synth_opts.set)
> > -                     dsos__hit_all(session);
> >               /*
> >                * The AUX areas have been removed and replaced with
> >                * synthesized hardware events, so clear the feature flag and
> > --
> > 2.28.0.681.g6f77f65b4e-goog
> >
>