2017-12-26 07:21:55

by Chunyan Zhang

Subject: [PATCH] tracing: Fix crash when it fails to alloc ring buffer

From: Jing Xia <[email protected]>

Double free of the ring buffer happens when it fails to alloc new
ring buffer instance for max_buffer if TRACER_MAX_TRACE is configured.
The root cause is that the pointer is not set to NULL after the buffer
is freed in allocate_trace_buffers(), and the freeing of the ring
buffer is invoked again later if the pointer is not equal to Null,
as:

instance_mkdir()
|-allocate_trace_buffers()
|-allocate_trace_buffer(tr, &tr->trace_buffer...)
|-allocate_trace_buffer(tr, &tr->max_buffer...)

// allocate fail(-ENOMEM),first free
// and the buffer pointer is not set to null
|-ring_buffer_free(tr->trace_buffer.buffer)

// out_free_tr
|-free_trace_buffers()
|-free_trace_buffer(&tr->trace_buffer);

//if trace_buffer is not null, free again
|-ring_buffer_free(buf->buffer)
|-rb_free_cpu_buffer(buffer->buffers[cpu])
// ring_buffer_per_cpu is null, and
// crash in ring_buffer_per_cpu->pages

Signed-off-by: Jing Xia <[email protected]>
Signed-off-by: Chunyan Zhang <[email protected]>
---
kernel/trace/trace.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 73e67b6..ed1c0d1 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -7622,7 +7622,9 @@ static int allocate_trace_buffers(struct trace_array *tr, int size)
allocate_snapshot ? size : 1);
if (WARN_ON(ret)) {
ring_buffer_free(tr->trace_buffer.buffer);
+ tr->trace_buffer.buffer = NULL;
free_percpu(tr->trace_buffer.data);
+ tr->trace_buffer.data = NULL;
return -ENOMEM;
}
tr->allocated_snapshot = allocate_snapshot;
--
2.7.4

2017-12-26 16:22:28

by Steven Rostedt

Subject: Re: [PATCH] tracing: Fix crash when it fails to alloc ring buffer

On Tue, 26 Dec 2017 15:12:53 +0800
Chunyan Zhang <[email protected]> wrote:

> From: Jing Xia <[email protected]>
>
> Double free of the ring buffer happens when it fails to alloc new
> ring buffer instance for max_buffer if TRACER_MAX_TRACE is configured.
> The root cause is that the pointer is not set to NULL after the buffer
> is freed in allocate_trace_buffers(), and the freeing of the ring
> buffer is invoked again later if the pointer is not equal to Null,
> as:
>
> instance_mkdir()
> |-allocate_trace_buffers()
> |-allocate_trace_buffer(tr, &tr->trace_buffer...)
> |-allocate_trace_buffer(tr, &tr->max_buffer...)
>
> // allocate fail(-ENOMEM),first free
> // and the buffer pointer is not set to null
> |-ring_buffer_free(tr->trace_buffer.buffer)
>
> // out_free_tr
> |-free_trace_buffers()
> |-free_trace_buffer(&tr->trace_buffer);
>
> //if trace_buffer is not null, free again
> |-ring_buffer_free(buf->buffer)
> |-rb_free_cpu_buffer(buffer->buffers[cpu])
> // ring_buffer_per_cpu is null, and
> // crash in ring_buffer_per_cpu->pages
>
> Signed-off-by: Jing Xia <[email protected]>
> Signed-off-by: Chunyan Zhang <[email protected]>

Thanks!

I'll apply this to my tree and start testing it.

-- Steve

> ---
> kernel/trace/trace.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 73e67b6..ed1c0d1 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -7622,7 +7622,9 @@ static int allocate_trace_buffers(struct trace_array *tr, int size)
> allocate_snapshot ? size : 1);
> if (WARN_ON(ret)) {
> ring_buffer_free(tr->trace_buffer.buffer);
> + tr->trace_buffer.buffer = NULL;
> free_percpu(tr->trace_buffer.data);
> + tr->trace_buffer.data = NULL;
> return -ENOMEM;
> }
> tr->allocated_snapshot = allocate_snapshot;