Re: [PATCH 2/2] perf trace: Fix segmentfault on perf trace

From: He Kuang
Date: Tue Apr 07 2015 - 23:15:51 EST

Hi, Arnaldo
On 2015/4/7 20:36, Arnaldo Carvalho de Melo wrote:
Em Tue, Apr 07, 2015 at 05:31:11PM +0800, He Kuang escreveu:
After perf_evlist__filter_pollfd() filters out fds and releases
perf_mmap by using perf_evlist__mmap_put(), refcnt of perf_mmap hits 1
then perf_evlist__mmap_consume() will do the final unmap. In this
condition, perf_evlist__mmap_read() will crash by referencing invalid
mmap. Put refcnt check before use.

Can be reproduced as following:
After applying 1/2 in this series and trying to reproduce I couldn't, it
works, looking at the code...

Let me get my head around this, idea was that after all fds associated
with a mmap would be closed, i.e. the perf_mmap->refcnt hits zero, then
we would have to drain whatever was left in the mmap, but looking again
that doesn't look like that is what is doing, becaue in filter_pollfd we
will munmap it before being able to "drain" it, as all mmaps were
closed, thus filter_pollfd returned zero...

In function __perf_evlist__mmap(), refcnt is initialized to 2, see commit:
823969860329 ("perf evlist: Refcount mmaps")

After filter_pollfd, perf_mmap->refcnt is 1 not 0.

perf_evlist__filter_pollfd() -- refcnt=1
draining = true
if (perf_evlist__mmap_read() != NULL)
perf_evlist__mmap_consume() -- unmap, refcnt = 0
perf_evlist__mmap_read() -- segfault

I noticed that this issue also exists in builtin-record.c, but it
checks before mmap_read():

if (rec->evlist->mmap[i].base) {
if (record__mmap_read(rec, i, draining) != 0) {

So we can either do the check outside
builtin-trace.c:perf_evlist__mmap_read() like what
builtin-record.c do or inside. What's your opinion?

Reading on, thanks for the patch!

- Arnaldo

$ perf trace --duration 1.0 ls
perf: Segmentation fault
Obtained 14 stack frames.
./perf(dump_stack+0x2e) [0x503c2d]
/lib64/ [0x7f5fd9a4adf0]
./perf() [0x4a8fda]
./perf() [0x470b28]
./perf(cmd_trace+0xada) [0x4727bd]
./perf() [0x49c4f4]
./perf() [0x49c74d]
./perf() [0x49c899]
./perf() [0x434ea5]

Signed-off-by: He Kuang <hekuang@xxxxxxxxxx>
tools/perf/util/evlist.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 76ef7ee..9d36433 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -634,11 +634,18 @@ static struct perf_evsel *perf_evlist__event2evsel(struct perf_evlist *evlist,
union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
struct perf_mmap *md = &evlist->mmap[idx];
- unsigned int head = perf_mmap__read_head(md);
- unsigned int old = md->prev;
- unsigned char *data = md->base + page_size;
+ unsigned int head;
+ unsigned int old;
+ unsigned char *data;
union perf_event *event = NULL;
+ if (md == NULL || md->refcnt == 0)
+ return NULL;
+ head = perf_mmap__read_head(md);
+ old = md->prev;
+ data = md->base + page_size;
if (evlist->overwrite) {
* If we're further behind than half the buffer, there's a chance

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at