Date: Tue, 7 Apr 2015 09:36:33 -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: <20150407123633.GG11983@kernel.org>
References: <1428399071-7141-1-git-send-email-hekuang@huawei.com>
 <1428399071-7141-2-git-send-email-hekuang@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1428399071-7141-2-git-send-email-hekuang@huawei.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2940
Lines: 85

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

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