Messages in this thread | | | Date | Mon, 18 Nov 2019 13:22:14 +0100 | From | Jiri Olsa <> | Subject | Re: [PATCH v2] perf session: fix decompression of PERF_RECORD_COMPRESSED records |
| |
On Mon, Nov 18, 2019 at 02:30:04PM +0300, Alexey Budankov wrote: > > Avoid termination of trace loading in case the last record in > the decompressed buffer partly resides in the following > mmaped PERF_RECORD_COMPRESSED record. In this case NULL value > returned by fetch_mmaped_event() means to proceed to the next > mmaped record then decompress it and load compressed events. > > The issue can be reproduced like this: > > $ perf record -z -- some_long_running_workload > $ perf report --stdio -vv > decomp (B): 44519 to 163000 > decomp (B): 48119 to 174800 > decomp (B): 65527 to 131072 > fetch_mmaped_event: head=0x1ffe0 event->header_size=0x28, mmap_size=0x20000: fuzzed perf.data? > Error: > failed to process sample > ... > > Testing: > 71: Zstd perf.data compression/decompression : Ok > > Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size") > Link: https://marc.info/?l=linux-kernel&m=156580812427554&w=2 > Co-developed-by: Jiri Olsa <jolsa@kernel.org> > Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com> > --- > Changes in v2: > - avoided static declaration of prefetch_event(); > - renamed 'ret' to 'error' for returning in case of split compressed > or overlapping records; > - passed only needs_swap quality into fetch_*_event() instead of > the whole session object; > --- > tools/perf/util/session.c | 44 ++++++++++++++++++++++++--------------- > 1 file changed, 27 insertions(+), 17 deletions(-) > > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c > index f07b8ecb91bc..c2b0703d6587 100644 > --- a/tools/perf/util/session.c > +++ b/tools/perf/util/session.c > @@ -1958,8 +1958,8 @@ static int __perf_session__process_pipe_events(struct perf_session *session) > } > > static union perf_event * > -fetch_mmaped_event(struct perf_session *session, > - u64 head, size_t mmap_size, char *buf) > +prefetch_event(char *buf, u64 head, size_t mmap_size, > + bool needs_swap, union perf_event *error) > { > union perf_event *event; > > @@ -1971,20 +1971,32 @@ fetch_mmaped_event(struct perf_session *session, > return NULL; > > event = (union perf_event *)(buf + head); > + if (needs_swap) > + perf_event_header__bswap(&event->header); > > - if (session->header.needs_swap) > + if (head + event->header.size <= mmap_size) > + return event; > + > + /* We're not fetching the event so swap back again */ > + if (needs_swap) > perf_event_header__bswap(&event->header); > > - if (head + event->header.size > mmap_size) { > - /* We're not fetching the event so swap back again */ > - if (session->header.needs_swap) > - perf_event_header__bswap(&event->header); > - pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx: fuzzed perf.data?\n", > - __func__, head, event->header.size, mmap_size); > - return ERR_PTR(-EINVAL); > - } > + pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx\n", > + __func__, head, event->header.size, mmap_size);
you're missign the 'fuzzed perf.data?' in here
I think we should keep it just change to: 'fuzzed or compressed perf.data?'
thanks, jirka
| |