[PATCH] perf inject corrupts file by deleting event

* [PATCH] perf inject corrupts file by deleting event
       [not found] <83633eb2-04dc-4a13-3ad7-abd3a7459ac1@foss.arm.com>
@ 2020-11-13 20:38 ` Al Grant
  2020-11-16 16:25   ` Namhyung Kim
  0 siblings, 1 reply; 7+ messages in thread
From: Al Grant @ 2020-11-13 20:38 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	linux-perf-users

"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>
---
  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;

^ permalink raw reply related	[flat|nested] 7+ messages in thread