Date: Mon, 11 May 2015 10:47:34 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: He Kuang <hekuang@huawei.com>
Cc: a.p.zijlstra@chello.nl, mingo@redhat.com, jolsa@kernel.org,
        wangnan0@huawei.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] perf trace: Fix segmentfault on perf trace
Message-ID: <20150511134734.GD28183@kernel.org>
References: <1428399071-7141-1-git-send-email-hekuang@huawei.com>
 <1428399071-7141-2-git-send-email-hekuang@huawei.com>
 <20150407123633.GG11983@kernel.org>
 <55249D3D.7020706@huawei.com>
 <55509C62.6020608@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <55509C62.6020608@huawei.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2398
Lines: 58

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/