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

[Arnaldo Carvalho de Melo]

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