Re: [PATCH] perf inject corrupts file by deleting event

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Tue, Nov 17, 2020 at 01:25:09AM +0900, Namhyung Kim escreveu:
> Hello,
> 
> On Sat, Nov 14, 2020 at 5:38 AM Al Grant <al.grant@foss.arm.com> wrote:
> >
> > "perf inject" can create corrupt files when synthesizing sample
> > events from AUX data. This happens when in the input file, the
> > first event (for the AUX data) has a different sample_type from
> > the second event (generally dummy). Specifically, they differ in
> > the bits that indicate the standard fields appended to perf
> > records in the mmap buffer. "perf inject" deletes the first event
> > and moves up the second event to first position. The problem is
> > with the synthetic PERF_RECORD_MMAP (etc.) events created by
> > "perf record". Since these are synthetic versions of events which
> > are normally produced by the kernel, they have to have the
> > standard fields appended as described by sample_type. "perf record"
> > fills these in with zeroes, including the IDENTIFIER field;
> > perf readers interpret records with zero IDENTIFIER using the
> > descriptor for the first event in the file. Since "perf inject"
> > changes the first event, these synthetic records are then
> > processed with the wrong value of sample_type, and the perf
> > reader reads bad data, reports on incorrect length records etc.
> >
> > Mismatching sample_types are seen with "perf record -e cs_etm//",
> > where the AUX event has TID|TIME|CPU|IDENTIFIER and the dummy
> > event has TID|TIME|IDENTIFIER. Perhaps they could be the same,
> > but it isn't normally a problem if they aren't - perf has
> > no problems reading the file. The sample_types have to agree on
> > the position of IDENTIFIER, because that's how perf finds the
> > right event descriptor in the first place, but they don't normally
> > have to agree on other fields, and perf doesn't check that they do.
> > The problem is specific to the way "perf inject" reorganizes the
> > events and the way synthetic MMAP events are recorded with a zero
> > identifier. A simple solution is to stop "perf inject" deleting
> > the tracing event.
> >
> > Signed-off-by: Al Grant <al.grant@arm.com>
> > Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Cc: Jiri Olsa <jolsa@redhat.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> 
> While I'm ok with this change,

So, to make progress, I'll take your phrase as an Acked-by, as described
in Documentation/process/submitting-patches.rst, ok?

The rest of your comments below can be done as some follow up work,

Thanks,

- Arnaldo

> I think we can put the dummy events
> to the front of the evlist (during record) so that we can make sure that
> tracking records would refer to them in order to parse the data.
> 
> And I also think that we should omit the dummy events from the
> perf report output.
> 
> Thanks,
> Namhyung
> 
> 
> > ---
> >   tools/perf/builtin-inject.c | 7 -------
> >   1 file changed, 7 deletions(-)
> >
> > diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> > index 452a75fe68e5..f4968ebf5f3a 100644
> > --- a/tools/perf/builtin-inject.c
> > +++ b/tools/perf/builtin-inject.c
> > @@ -791,13 +791,6 @@ static int __cmd_inject(struct perf_inject *inject)
> >                              inject->itrace_synth_opts.add_last_branch)
> >                                  perf_header__set_feat(&session->header,
> >                                                        HEADER_BRANCH_STACK);
> > -                       evsel = perf_evlist__id2evsel_strict(session->evlist,
> > -                                                            inject->aux_id);
> > -                       if (evsel) {
> > -                               pr_debug("Deleting %s\n", evsel__name(evsel));
> > -                               evlist__remove(session->evlist, evsel);
> > -                               evsel__delete(evsel);
> > -                       }
> >                  }
> >                  session->header.data_offset = output_data_offset;
> >                  session->header.data_size = inject->bytes_written;

-- 

- Arnaldo