lkml.org 
[lkml]   [2015]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] perf trace: Fix segmentfault on perf trace
From
Date
Hi, Arnaldo

On 2015/4/8 11:15, He Kuang wrote:
> 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?

I found the issue is still there, so ping...

>>
>> 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 <hekuang@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
>>
>
>




\
 
 \ /
  Last update: 2015-05-11 14:41    [W:0.065 / U:34.016 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site