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

[He Kuang]

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


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