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

[He Kuang <hekuang@huawei.com>]

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