Re: [PATCH 1/5] perf tools: Enforce ring buffer reading

From: Arnaldo Carvalho de Melo
Date: Tue Apr 26 2016 - 09:31:23 EST


Em Tue, Apr 26, 2016 at 02:28:54AM +0000, Wang Nan escreveu:
> Don't read broken data after 'head' pointer.
>
> Following commits will feed perf_evlist__mmap_read() with some 'head'
> pointers not maintained by kernel. If 'head' pointer breaks an event,
> we should avoid reading from the broken event. This can happen in
> backward ring buffer.

Looks good, applied.

- Arnaldo

> For example:
>
> old head
> | |
> V V
> +---+------+----------+----+-----+--+
> |..E|D....D|C........C|B..B|A....|E.|
> +---+------+----------+----+-----+--+
>
> 'old' pointer points to the beginning of 'A' and trying read from it,
> but 'A' has been overwritten. In this case, don't try to read from 'A',
> simply return NULL.
>
> Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Zefan Li <lizefan@xxxxxxxxxx>
> Cc: pi3orama@xxxxxxx
> ---
> tools/perf/util/evlist.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 6fb5725..85271e5 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -684,6 +684,7 @@ union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
> struct perf_mmap *md = &evlist->mmap[idx];
> u64 head;
> u64 old = md->prev;
> + int diff;
> unsigned char *data = md->base + page_size;
> union perf_event *event = NULL;
>
> @@ -694,6 +695,7 @@ union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
> return NULL;
>
> head = perf_mmap__read_head(md);
> + diff = head - old;
> if (evlist->overwrite) {
> /*
> * If we're further behind than half the buffer, there's a chance
> @@ -703,7 +705,6 @@ union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
> *
> * In either case, truncate and restart at head.
> */
> - int diff = head - old;
> if (diff > md->mask / 2 || diff < 0) {
> fprintf(stderr, "WARNING: failed to keep up with mmap data.\n");
>
> @@ -711,15 +712,21 @@ union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
> * head points to a known good entry, start there.
> */
> old = head;
> + diff = 0;
> }
> }
>
> - if (old != head) {
> + if (diff >= (int)sizeof(event->header)) {
> size_t size;
>
> event = (union perf_event *)&data[old & md->mask];
> size = event->header.size;
>
> + if (size < sizeof(event->header) || diff < (int)size) {
> + event = NULL;
> + goto broken_event;
> + }
> +
> /*
> * Event straddles the mmap boundary -- header should always
> * be inside due to u64 alignment of output.
> @@ -743,6 +750,7 @@ union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
> old += size;
> }
>
> +broken_event:
> md->prev = old;
>
> return event;
> --
> 1.8.3.4