linux-kernel - Re: [PATCH] perf data: Fix releasing event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56A9D887.8030508@huawei.com>
Date:	Thu, 28 Jan 2016 16:59:51 +0800
From:	"Wangnan (F)" <wangnan0@...wei.com>
To:	Jiri Olsa <jolsa@...nel.org>
CC:	<acme@...hat.com>, <linux-kernel@...r.kernel.org>,
	Jérémie Galarneau 
	<jeremie.galarneau@...icios.com>, Zefan Li <lizefan@...wei.com>,
	<pi3orama@....com>
Subject: Re: [PATCH] perf data: Fix releasing event_class

Hi Jiri, What's your opinion on this patch?

Thank you.

On 2016/1/27 19:21, Wang Nan wrote:
> A new patch of libbabeltrace [1] reveals a object leak problem in
> perf data CTF support: perf code never release event_class which is
> allocated in add_event() and stored in evsel's private field.
>
> If libbabeltrace has the above patch applied, leaking event_class
> prevent writer being destroied and flushing metadata. For example:
>
>   $ ./perf record ls
>   Lowering default frequency rate to 500.
>   Please consider tweaking /proc/sys/kernel/perf_event_max_sample_rate.
>   perf.data
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.012 MB perf.data (12 samples) ]
>   $ ./perf data convert --to-ctf ./out.ctf
>   [ perf data convert: Converted 'perf.data' into CTF data './out.ctf' ]
>   [ perf data convert: Converted and wrote 0.000 MB (12 samples) ]
>   $ cat ./out.ctf/metadata
>   $ ls -l  ./out.ctf/metadata
>   -rw-r----- 1 w00229757 mm 0 Jan 27 10:49 ./out.ctf/metadata
>
> The correct result should be:
>   ...
>   $ cat ./out.ctf/metadata
>   /* CTF 1.8 */
>
>   trace {
>   [SNIP]
>
>   $ ls -l  ./out.ctf/metadata
>   -rw-r----- 1 w00229757 mm 2446 Jan 27 10:52 ./out.ctf/metadata
>
> The full story is:
>
>   Patch [1] of babeltrace redesign reference counting scheme. In that
>   patch:
>
>    * writer <- trace (bt_ctf_writer_create)
>    * trace <- stream_class (bt_ctf_trace_add_stream_class)
>    * stream_class <- event_class (bt_ctf_stream_class_add_event_class)
>    ('<-' means 'is a parent of')
>
>    Holding of event_class causes reference count of corresponding
>    'writer' increases through parent chain. Perf expect 'writer' is
>    released (so metadata is flushed) through bt_ctf_writer_put() in
>    ctf_writer__cleanup(). However, since it never release event_class,
>    the reference of 'writer' won't be reduced, so bt_ctf_writer_put()
>    won't lead releasing of writer.
>
>   Before this CTF patch, !(writer <- trace). Even event_class leak,
>   writer is able to be released.
>
> [1] https://github.com/efficios/babeltrace/commit/e6a8e8e4744633807083a077ff9f101eb97d9801
>
> Signed-off-by: Wang Nan <wangnan0@...wei.com>
> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
> Cc: Jiri Olsa <jolsa@...nel.org>
> Cc: Jérémie Galarneau <jeremie.galarneau@...icios.com>
> Cc: Zefan Li <lizefan@...wei.com>
> Cc: pi3orama@....com
> ---
>   tools/perf/util/data-convert-bt.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/tools/perf/util/data-convert-bt.c b/tools/perf/util/data-convert-bt.c
> index 34cd1e4..e553321 100644
> --- a/tools/perf/util/data-convert-bt.c
> +++ b/tools/perf/util/data-convert-bt.c
> @@ -858,6 +858,19 @@ static int setup_events(struct ctf_writer *cw, struct perf_session *session)
>   	return 0;
>   }
>   
> +static void cleanup_events(struct perf_session *session)
> +{
> +	struct perf_evlist *evlist = session->evlist;
> +	struct perf_evsel *evsel;
> +
> +	evlist__for_each(evlist, evsel) {
> +		struct evsel_priv *priv;
> +
> +		priv = evsel->priv;
> +		bt_ctf_event_class_put(priv->event_class);
> +	}
> +}
> +
>   static int setup_streams(struct ctf_writer *cw, struct perf_session *session)
>   {
>   	struct ctf_stream **stream;
> @@ -1171,6 +1184,7 @@ int bt_convert__perf2ctf(const char *input, const char *path, bool force)
>   		(double) c.events_size / 1024.0 / 1024.0,
>   		c.events_count);
>   
> +	cleanup_events(session);
>   	perf_session__delete(session);
>   	ctf_writer__cleanup(cw);
>