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
else
exit
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]
./perf(sighandler_dump_stack+0x2e)
[0x503d0c]
/lib64/libc.so.6(+0x34df0) [0x7f5fd9a4adf0]
./perf() [0x4a8fda]
./perf(perf_evlist__mmap_read+0x56)
[0x4aae93]
./perf() [0x470b28]
./perf(cmd_trace+0xada) [0x4727bd]
./perf() [0x49c4f4]
./perf() [0x49c74d]
./perf() [0x49c899]
./perf(main+0x23b)
[0x49cbfa]
/lib64/libc.so.6(__libc_start_main+0xf5)
[0x7f5fd9a377b5]
./perf() [0x434ea5]
[(nil)]
Signed-off-by: He Kuang <heku...@huawei.com>
---
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
--
2.3.3.220.g9ab698f
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
