linux-kernel - Re: [for-next][PATCH 6/6] tracing: Fix leak of ring buffer data when new instances creation fails

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 for Android: free password hash cracker in your pocket

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

Message-ID: <87k38pbake.fsf@sejong.aot.lge.com>
Date:	Tue, 10 Jun 2014 14:25:21 +0900
From:	Namhyung Kim <namhyung@...il.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@...achi.com>
Subject: Re: [for-next][PATCH 6/6] tracing: Fix leak of ring buffer data when new instances creation fails

On Fri, 06 Jun 2014 12:30:40 -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@...dmis.org>
>
> Yoshihiro Yunomae reported that the ring buffer data for a trace
> instance does not get properly cleaned up when it fails. He proposed
> a patch that manually cleaned the data up and addad a bunch of labels.
> The labels are not needed because all trace array is allocated with
> a kzalloc which initializes it to 0 and all kfree()s can take a NULL
> pointer and will ignore it.
>
> Adding a new helper function free_trace_buffers() that can also take
> null buffers to free the buffers that were allocated by
> allocate_trace_buffers().
>
> Link: http://lkml.kernel.org/r/20140605223522.32311.31664.stgit@yunodevel
>
> Reported-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@...achi.com>
> Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
> ---
>  kernel/trace/trace.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index e29edee1542a..26cfff38e2ab 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -6232,6 +6232,25 @@ static int allocate_trace_buffers(struct trace_array *tr, int size)
>  	return 0;
>  }
>  
> +static void free_trace_buffers(struct trace_array *tr)
> +{
> +	if (!tr)
> +		return;
> +
> +	if (tr->trace_buffer.buffer) {
> +		ring_buffer_free(tr->trace_buffer.buffer);
> +		tr->trace_buffer.buffer = NULL;
> +		free_percpu(tr->trace_buffer.data);
> +	}
> +
> +#ifdef CONFIG_TRACER_MAX_TRACE
> +	if (tr->max_buffer.buffer) {
> +		ring_buffer_free(tr->max_buffer.buffer);
> +		tr->max_buffer.buffer = NULL;

Hmm.. why doesn't it free tr->max_buffer.data here?  And I think it's
better to reset *_buffer.data to NULL also - maybe by adding another
helper function free_trace_buffer()..

Thanks,
Namhyung


> +	}
> +#endif
> +}
> +
>  static int new_instance_create(const char *name)
>  {
>  	struct trace_array *tr;
> @@ -6290,8 +6309,7 @@ static int new_instance_create(const char *name)
>  	return 0;
>  
>   out_free_tr:
> -	if (tr->trace_buffer.buffer)
> -		ring_buffer_free(tr->trace_buffer.buffer);
> +	free_trace_buffers(tr);
>  	free_cpumask_var(tr->tracing_cpumask);
>  	kfree(tr->name);
>  	kfree(tr);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/