Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752487AbcDZNbU (ORCPT ); Tue, 26 Apr 2016 09:31:20 -0400 Received: from mail.kernel.org ([198.145.29.136]:38659 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752194AbcDZNbS (ORCPT ); Tue, 26 Apr 2016 09:31:18 -0400 Date: Tue, 26 Apr 2016 10:31:14 -0300 From: Arnaldo Carvalho de Melo To: Wang Nan Cc: linux-kernel@vger.kernel.org, Arnaldo Carvalho de Melo , Peter Zijlstra , Zefan Li , pi3orama@163.com Subject: Re: [PATCH 1/5] perf tools: Enforce ring buffer reading Message-ID: <20160426133114.GG16708@kernel.org> References: <1461637738-62722-1-git-send-email-wangnan0@huawei.com> <1461637738-62722-2-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1461637738-62722-2-git-send-email-wangnan0@huawei.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3052 Lines: 95 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 > Cc: Arnaldo Carvalho de Melo > Cc: Peter Zijlstra > Cc: Zefan Li > Cc: pi3orama@163.com > --- > 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