2014-06-06 17:22:50

by Steven Rostedt

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

From: "Steven Rostedt (Red Hat)" <[email protected]>

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 <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
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;
+ }
+#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);
--
2.0.0.rc2

2014-06-10 05:25:24

by Namhyung Kim

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)" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> 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);

2014-06-10 15:58:56

by Steven Rostedt

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

On Tue, 10 Jun 2014 14:25:21 +0900
Namhyung Kim <[email protected]> wrote:


> > +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()..
>

Originally, instances didn't need them. But I'm looking now and yes
they do allocate. I guess this doesn't trigger kmem-leak checks, as I
ran creation and deletions with that running and it didn't report any
leaks.

I'll update,

Thanks!

-- Steve