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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Mon, May 11, 2015 at 08:11:14PM +0800, He Kuang escreveu:
> 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...

Right, I noticed it as well sometimes, will apply the bandaid and leave
properly researching it for later.

- Arnaldo