Messages in this thread | | | Date | Mon, 11 May 2015 10:47:34 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 2/2] perf trace: Fix segmentfault on perf trace |
| |
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
| |