[PATCH v2 0/1] ftrace patches for 2.6.28 (update)

[PATCH v2 0/1] ftrace patches for 2.6.28 (update)

[Steven Rostedt]

[
  As Andrew pointed out, the previous version had a lot of ugly
  #ifdefs. This patch removes them and just does the check at
  the resize. It also hides the max latency recording functions
  behind the config option.

  The git tree now has this version instead.
]

Ingo,

The following patches are against mainline, and need to go in 2.6.28:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

    branch: devel


Steven Rostedt (1):
      ftrace: do not update max buffer with no users

----
 kernel/trace/trace.c |  171 ++++++++++++++++++++++++++------------------------
 1 files changed, 88 insertions(+), 83 deletions(-)

[PATCH v2 1/1] ftrace: do not update max buffer with no users

[Steven Rostedt]

[-- Attachment #1: 0001-ftrace-do-not-update-max-buffer-with-no-users.patch --]
[-- Type: text/plain, Size: 6976 bytes --]

Impact: only use max latency buffer when a user is configured

Pekka reported a bug with the resizing of the buffers when only the
MMIO tracer was configured. The issue was, to save memory, the max
latency trace buffer was only initialized if a tracer that uses it
is configured in.

What happened was that the max latency buffer was never initialized
when only the MMIO tracer was configurued. The MMIO tracer does not
use the max tracer, which kept it from being initialized. But the
resize code still tried to resize the max latency buffers, but because
they were never allocated, the resize code was passed a NULL pointer.

This patch checks if the max tracer is NULL before resizing it.
It also hides the modification functions of the max tracer behind
the TRACER_MAX_TRACE config.

Reported-by: Pekka Paalanen <pq@iki.fi>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/trace.c |  171 ++++++++++++++++++++++++++------------------------
 1 files changed, 88 insertions(+), 83 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index abfa810..6e03ec1 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -95,20 +95,6 @@ static struct trace_array	global_trace;
 
 static DEFINE_PER_CPU(struct trace_array_cpu, global_trace_cpu);
 
-/*
- * The max_tr is used to snapshot the global_trace when a maximum
- * latency is reached. Some tracers will use this to store a maximum
- * trace while it continues examining live traces.
- *
- * The buffers for the max_tr are set up the same as the global_trace.
- * When a snapshot is taken, the link list of the max_tr is swapped
- * with the link list of the global_trace and the buffers are reset for
- * the global_trace so the tracing can continue.
- */
-static struct trace_array	max_tr;
-
-static DEFINE_PER_CPU(struct trace_array_cpu, max_data);
-
 /* tracer_enabled is used to toggle activation of a tracer */
 static int			tracer_enabled = 1;
 
@@ -229,6 +215,21 @@ static raw_spinlock_t ftrace_max_lock =
 	(raw_spinlock_t)__RAW_SPIN_LOCK_UNLOCKED;
 
 /*
+ * The max_tr is used to snapshot the global_trace when a maximum
+ * latency is reached. Some tracers will use this to store a maximum
+ * trace while it continues examining live traces.
+ *
+ * The buffers for the max_tr are set up the same as the global_trace.
+ * When a snapshot is taken, the link list of the max_tr is swapped
+ * with the link list of the global_trace and the buffers are reset for
+ * the global_trace so the tracing can continue.
+ */
+static struct trace_array	max_tr;
+
+static DEFINE_PER_CPU(struct trace_array_cpu, max_data);
+
+#ifdef CONFIG_TRACER_MAX_TRACE
+/*
  * Copy the new maximum trace into the separate maximum-trace
  * structure. (this way the maximum trace is permanently saved,
  * for later retrieval via /debugfs/tracing/latency_trace)
@@ -256,6 +257,65 @@ __update_max_tr(struct trace_array *tr, struct task_struct *tsk, int cpu)
 }
 
 /**
+ * update_max_tr - snapshot all trace buffers from global_trace to max_tr
+ * @tr: tracer
+ * @tsk: the task with the latency
+ * @cpu: The cpu that initiated the trace.
+ *
+ * Flip the buffers between the @tr and the max_tr and record information
+ * about which task was the cause of this latency.
+ */
+void
+update_max_tr(struct trace_array *tr, struct task_struct *tsk, int cpu)
+{
+	struct ring_buffer *buf = tr->buffer;
+
+	WARN_ON_ONCE(!irqs_disabled());
+	__raw_spin_lock(&ftrace_max_lock);
+
+	tr->buffer = max_tr.buffer;
+	max_tr.buffer = buf;
+
+	ftrace_disable_cpu();
+	ring_buffer_reset(tr->buffer);
+	ftrace_enable_cpu();
+
+	__update_max_tr(tr, tsk, cpu);
+	__raw_spin_unlock(&ftrace_max_lock);
+}
+
+/**
+ * update_max_tr_single - only copy one trace over, and reset the rest
+ * @tr - tracer
+ * @tsk - task with the latency
+ * @cpu - the cpu of the buffer to copy.
+ *
+ * Flip the trace of a single CPU buffer between the @tr and the max_tr.
+ */
+void
+update_max_tr_single(struct trace_array *tr, struct task_struct *tsk, int cpu)
+{
+	int ret;
+
+	WARN_ON_ONCE(!irqs_disabled());
+	__raw_spin_lock(&ftrace_max_lock);
+
+	ftrace_disable_cpu();
+
+	ring_buffer_reset(max_tr.buffer);
+	ret = ring_buffer_swap_cpu(max_tr.buffer, tr->buffer, cpu);
+
+	ftrace_enable_cpu();
+
+	WARN_ON_ONCE(ret);
+
+	__update_max_tr(tr, tsk, cpu);
+	__raw_spin_unlock(&ftrace_max_lock);
+}
+
+#endif /* CONFIG_TRACER_MAX_TRACE */
+
+/**
  * trace_seq_printf - sequence printing of trace information
  * @s: trace sequence descriptor
  * @fmt: printf format string
@@ -397,63 +457,6 @@ trace_print_seq(struct seq_file *m, struct trace_seq *s)
 }
 
 /**
- * update_max_tr - snapshot all trace buffers from global_trace to max_tr
- * @tr: tracer
- * @tsk: the task with the latency
- * @cpu: The cpu that initiated the trace.
- *
- * Flip the buffers between the @tr and the max_tr and record information
- * about which task was the cause of this latency.
- */
-void
-update_max_tr(struct trace_array *tr, struct task_struct *tsk, int cpu)
-{
-	struct ring_buffer *buf = tr->buffer;
-
-	WARN_ON_ONCE(!irqs_disabled());
-	__raw_spin_lock(&ftrace_max_lock);
-
-	tr->buffer = max_tr.buffer;
-	max_tr.buffer = buf;
-
-	ftrace_disable_cpu();
-	ring_buffer_reset(tr->buffer);
-	ftrace_enable_cpu();
-
-	__update_max_tr(tr, tsk, cpu);
-	__raw_spin_unlock(&ftrace_max_lock);
-}
-
-/**
- * update_max_tr_single - only copy one trace over, and reset the rest
- * @tr - tracer
- * @tsk - task with the latency
- * @cpu - the cpu of the buffer to copy.
- *
- * Flip the trace of a single CPU buffer between the @tr and the max_tr.
- */
-void
-update_max_tr_single(struct trace_array *tr, struct task_struct *tsk, int cpu)
-{
-	int ret;
-
-	WARN_ON_ONCE(!irqs_disabled());
-	__raw_spin_lock(&ftrace_max_lock);
-
-	ftrace_disable_cpu();
-
-	ring_buffer_reset(max_tr.buffer);
-	ret = ring_buffer_swap_cpu(max_tr.buffer, tr->buffer, cpu);
-
-	ftrace_enable_cpu();
-
-	WARN_ON_ONCE(ret);
-
-	__update_max_tr(tr, tsk, cpu);
-	__raw_spin_unlock(&ftrace_max_lock);
-}
-
-/**
  * register_tracer - register a tracer with the ftrace system.
  * @type - the plugin for the tracer
  *
@@ -2719,19 +2722,21 @@ tracing_entries_write(struct file *filp, const char __user *ubuf,
 			goto out;
 		}
 
-		ret = ring_buffer_resize(max_tr.buffer, val);
-		if (ret < 0) {
-			int r;
-			cnt = ret;
-			r = ring_buffer_resize(global_trace.buffer,
-					       global_trace.entries);
-			if (r < 0) {
-				/* AARGH! We are left with different
-				 * size max buffer!!!! */
-				WARN_ON(1);
-				tracing_disabled = 1;
+		if (max_tr.buffer) {
+			ret = ring_buffer_resize(max_tr.buffer, val);
+			if (ret < 0) {
+				int r;
+				cnt = ret;
+				r = ring_buffer_resize(global_trace.buffer,
+						       global_trace.entries);
+				if (r < 0) {
+					/* AARGH! We are left with different
+					 * size max buffer!!!! */
+					WARN_ON(1);
+					tracing_disabled = 1;
+				}
+				goto out;
 			}
-			goto out;
 		}
 
 		global_trace.entries = val;
-- 
1.5.6.5

-- 

Re: [PATCH v2 1/1] ftrace: do not update max buffer with no users

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Impact: only use max latency buffer when a user is configured
> 
> Pekka reported a bug with the resizing of the buffers when only the 
> MMIO tracer was configured. The issue was, to save memory, the max 
> latency trace buffer was only initialized if a tracer that uses it 
> is configured in.
> 
> What happened was that the max latency buffer was never initialized 
> when only the MMIO tracer was configurued. The MMIO tracer does not 
> use the max tracer, which kept it from being initialized. But the 
> resize code still tried to resize the max latency buffers, but 
> because they were never allocated, the resize code was passed a NULL 
> pointer.
> 
> This patch checks if the max tracer is NULL before resizing it. It 
> also hides the modification functions of the max tracer behind the 
> TRACER_MAX_TRACE config.
> 
> Reported-by: Pekka Paalanen <pq@iki.fi>
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  kernel/trace/trace.c |  171 ++++++++++++++++++++++++++------------------------
>  1 files changed, 88 insertions(+), 83 deletions(-)

This is _way_ too much churn for .28, we need a much simpler solution.

	Ingo

Re: [PATCH v2 1/1] ftrace: do not update max buffer with no users

[Steven Rostedt]

On Thu, 13 Nov 2008, Ingo Molnar wrote:

> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Impact: only use max latency buffer when a user is configured
> > 
> > Pekka reported a bug with the resizing of the buffers when only the 
> > MMIO tracer was configured. The issue was, to save memory, the max 
> > latency trace buffer was only initialized if a tracer that uses it 
> > is configured in.
> > 
> > What happened was that the max latency buffer was never initialized 
> > when only the MMIO tracer was configurued. The MMIO tracer does not 
> > use the max tracer, which kept it from being initialized. But the 
> > resize code still tried to resize the max latency buffers, but 
> > because they were never allocated, the resize code was passed a NULL 
> > pointer.
> > 
> > This patch checks if the max tracer is NULL before resizing it. It 
> > also hides the modification functions of the max tracer behind the 
> > TRACER_MAX_TRACE config.
> > 
> > Reported-by: Pekka Paalanen <pq@iki.fi>
> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> > ---
> >  kernel/trace/trace.c |  171 ++++++++++++++++++++++++++------------------------
> >  1 files changed, 88 insertions(+), 83 deletions(-)
> 
> This is _way_ too much churn for .28, we need a much simpler solution.

The line count is very misleading. The 83 insertions and deletions where 
moved code or indentation:

-/*
- * The max_tr is used to snapshot the global_trace when a maximum
- * latency is reached. Some tracers will use this to store a maximum
- * trace while it continues examining live traces.
- *
- * The buffers for the max_tr are set up the same as the global_trace.
- * When a snapshot is taken, the link list of the max_tr is swapped
- * with the link list of the global_trace and the buffers are reset for
- * the global_trace so the tracing can continue.
- */
-static struct trace_array      max_tr;
-
-static DEFINE_PER_CPU(struct trace_array_cpu, max_data);
-
+ * The max_tr is used to snapshot the global_trace when a maximum
+ * latency is reached. Some tracers will use this to store a maximum
+ * trace while it continues examining live traces.
+ *
+ * The buffers for the max_tr are set up the same as the global_trace.
+ * When a snapshot is taken, the link list of the max_tr is swapped
+ * with the link list of the global_trace and the buffers are reset for
+ * the global_trace so the tracing can continue.
+ */
+static struct trace_array      max_tr;
+
+static DEFINE_PER_CPU(struct trace_array_cpu, max_data);
+
+#ifdef CONFIG_TRACER_MAX_TRACE   <--- Added line
+/*

+ * update_max_tr - snapshot all trace buffers from global_trace to max_tr
+ * @tr: tracer
+ * @tsk: the task with the latency
+ * @cpu: The cpu that initiated the trace.
+ *
+ * Flip the buffers between the @tr and the max_tr and record information
+ * about which task was the cause of this latency.
+ */
+void
+update_max_tr(struct trace_array *tr, struct task_struct *tsk, int cpu)
+{
+       struct ring_buffer *buf = tr->buffer;
+
+       WARN_ON_ONCE(!irqs_disabled());
+       __raw_spin_lock(&ftrace_max_lock);
+
+       tr->buffer = max_tr.buffer;
+       max_tr.buffer = buf;
+
+       ftrace_disable_cpu();
+       ring_buffer_reset(tr->buffer);
+       ftrace_enable_cpu();
+
+       __update_max_tr(tr, tsk, cpu);
+       __raw_spin_unlock(&ftrace_max_lock);
+}
+
+/**
- * update_max_tr - snapshot all trace buffers from global_trace to max_tr
- * @tr: tracer
- * @tsk: the task with the latency
- * @cpu: The cpu that initiated the trace.
- *
- * Flip the buffers between the @tr and the max_tr and record information
- * about which task was the cause of this latency.
- */
-void
-update_max_tr(struct trace_array *tr, struct task_struct *tsk, int cpu)
-{
-       struct ring_buffer *buf = tr->buffer;
-
-       WARN_ON_ONCE(!irqs_disabled());
-       __raw_spin_lock(&ftrace_max_lock);
-
-       tr->buffer = max_tr.buffer;
-       max_tr.buffer = buf;
-
-       ftrace_disable_cpu();
-       ring_buffer_reset(tr->buffer);
-       ftrace_enable_cpu();
-
-       __update_max_tr(tr, tsk, cpu);
-       __raw_spin_unlock(&ftrace_max_lock);
-}
-
-/**


+ * update_max_tr_single - only copy one trace over, and reset the rest
+ * @tr - tracer
+ * @tsk - task with the latency
+ * @cpu - the cpu of the buffer to copy.
+ *
+ * Flip the trace of a single CPU buffer between the @tr and the max_tr.
+ */
+void
+update_max_tr_single(struct trace_array *tr, struct task_struct *tsk, int 
cpu)
+{
+       int ret;
+
+       WARN_ON_ONCE(!irqs_disabled());
+       __raw_spin_lock(&ftrace_max_lock);
+
+       ftrace_disable_cpu();
+
+       ring_buffer_reset(max_tr.buffer);
+       ret = ring_buffer_swap_cpu(max_tr.buffer, tr->buffer, cpu);
+
+       ftrace_enable_cpu();
+
+       WARN_ON_ONCE(ret);
+
+       __update_max_tr(tr, tsk, cpu);
+       __raw_spin_unlock(&ftrace_max_lock);
+}
+
+#endif /* CONFIG_TRACER_MAX_TRACE */  <---- Added line
+                                      <---- Added line
+/**
- * update_max_tr_single - only copy one trace over, and reset the rest
- * @tr - tracer
- * @tsk - task with the latency
- * @cpu - the cpu of the buffer to copy.
- *
- * Flip the trace of a single CPU buffer between the @tr and the max_tr.
- */
-void
-update_max_tr_single(struct trace_array *tr, struct task_struct *tsk, int 
cpu)
-{
-       int ret;
-
-       WARN_ON_ONCE(!irqs_disabled());
-       __raw_spin_lock(&ftrace_max_lock);
-
-       ftrace_disable_cpu();
-
-       ring_buffer_reset(max_tr.buffer);
-       ret = ring_buffer_swap_cpu(max_tr.buffer, tr->buffer, cpu);
-
-       ftrace_enable_cpu();
-
-       WARN_ON_ONCE(ret);
-
-       __update_max_tr(tr, tsk, cpu);
-       __raw_spin_unlock(&ftrace_max_lock);
-}
-
-/**

-               ret = ring_buffer_resize(max_tr.buffer, val);
-               if (ret < 0) {
-                       int r;
-                       cnt = ret;
-                       r = ring_buffer_resize(global_trace.buffer,
-                                              global_trace.entries);
-                       if (r < 0) {
-                               /* AARGH! We are left with different
-                                * size max buffer!!!! */
-                               WARN_ON(1);
-                               tracing_disabled = 1;
+               if (max_tr.buffer) {       <---- Added line
+                       ret = ring_buffer_resize(max_tr.buffer, val);
+                       if (ret < 0) {
+                               int r;
+                               cnt = ret;
+                               r = ring_buffer_resize(global_trace.buffer,
+                                                      global_trace.entries);
+                               if (r < 0) {
+                                       /* AARGH! We are left with 
different
+                                        * size max buffer!!!! */
+                                       WARN_ON(1);
+                                       tracing_disabled = 1;
+                               }  <---- Added line
+                               goto out;
                        }
-                       goto out;


I annotated the 5 added lines.

The above which is the most confusing diff, because the diff did it funny,
is the one part of the patch that _is_ needed. Here's the before and after 
of that part:

-               ret = ring_buffer_resize(max_tr.buffer, val);
-               if (ret < 0) {
-                       int r;
-                       cnt = ret;
-                       r = ring_buffer_resize(global_trace.buffer,
-                                              global_trace.entries);
-                       if (r < 0) {
-                               /* AARGH! We are left with different
-                                * size max buffer!!!! */
-                               WARN_ON(1);
-                               tracing_disabled = 1;
                        }
-                       goto out;
                }


+               if (max_tr.buffer) {
+                       ret = ring_buffer_resize(max_tr.buffer, val);
+                       if (ret < 0) {
+                               int r;
+                               cnt = ret;
+                               r = ring_buffer_resize(global_trace.buffer,
+                                                      global_trace.entries);
+                               if (r < 0) {
+                                       /* AARGH! We are left with different
+                                        * size max buffer!!!! */
+                                       WARN_ON(1);
+                                       tracing_disabled = 1;
+                               }
+                               goto out;
                        }
                }

As you can easily see, the change only added the test for max_tr.buffer
exists, and indented the rest.

The other parts you can easily see that they are not changed.
But, we could separate out the moved code and ifdef protection for 29, 
even though I feel nervous that some config might use them, and break at 
runtime. This change will break the compile if it happens. Without the 
change, we find out at runtime :-(

-- Steve

Re: [PATCH v2 1/1] ftrace: do not update max buffer with no users

[Steven Rostedt]

On Thu, 13 Nov 2008, Steven Rostedt wrote:
> > > Reported-by: Pekka Paalanen <pq@iki.fi>
> > > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> > > ---
> > >  kernel/trace/trace.c |  171 ++++++++++++++++++++++++++------------------------
> > >  1 files changed, 88 insertions(+), 83 deletions(-)
> > 
> > This is _way_ too much churn for .28, we need a much simpler solution.
> 
> The line count is very misleading. The 83 insertions and deletions where 
> moved code or indentation:
> 

> 
> As you can easily see, the change only added the test for max_tr.buffer
> exists, and indented the rest.
> 
> The other parts you can easily see that they are not changed.
> But, we could separate out the moved code and ifdef protection for 29, 
> even though I feel nervous that some config might use them, and break at 
> runtime. This change will break the compile if it happens. Without the 
> change, we find out at runtime :-(

Also, if you are worried about line count, I could just leave the code in 
place, and add more ifdefs around them, and leave the consolidation of the 
code for 29.

-- Steve

Re: [PATCH v2 1/1] ftrace: do not update max buffer with no users

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> The line count is very misleading. The 83 insertions and deletions where 
> moved code or indentation:

but that indentation is completely unnecessary:

> -               ret = ring_buffer_resize(max_tr.buffer, val);
> -               if (ret < 0) {
> -                       int r;
> -                       cnt = ret;
> -                       r = ring_buffer_resize(global_trace.buffer,
> -                                              global_trace.entries);
> -                       if (r < 0) {
> -                               /* AARGH! We are left with different
> -                                * size max buffer!!!! */
> -                               WARN_ON(1);
> -                               tracing_disabled = 1;
>                         }
> -                       goto out;
>                 }
> 
> 
> +               if (max_tr.buffer) {
> +                       ret = ring_buffer_resize(max_tr.buffer, val);
> +                       if (ret < 0) {
> +                               int r;
> +                               cnt = ret;
> +                               r = ring_buffer_resize(global_trace.buffer,
> +                                                      global_trace.entries);
> +                               if (r < 0) {
> +                                       /* AARGH! We are left with different
> +                                        * size max buffer!!!! */
> +                                       WARN_ON(1);
> +                                       tracing_disabled = 1;
> +                               }
> +                               goto out;
>                         }
>                 }

the obvious solution is to add this to ring_buffer_resize():

	if (!buffer)
		return size;

resizing a non-existent buffer should succeed. A two-liner patch. Not 
160 lines of flux.

Really, you need to think _hard_ how to avoid invasive-looking changes 
in late -rc's, because every extra line to review uses up precious 
review resources.

	Ingo

Re: [PATCH v2 1/1] ftrace: do not update max buffer with no users

[Steven Rostedt]

On Thu, 13 Nov 2008, Ingo Molnar wrote:
> 
> the obvious solution is to add this to ring_buffer_resize():
> 
> 	if (!buffer)
> 		return size;

Having a NULL buffer return a successful resize is a bit worrisome to me.

And looking at the code I was trying to make sure could never be called
if there are no max_tr users:

in update_max_tr

	buf = tr->buffer;
	tr->buffer = max_tr.buffer;
	max_tr.buffer = buf;

Should all the ring buffer API return success on NULL pointers?

> 
> resizing a non-existent buffer should succeed. A two-liner patch. Not 
> 160 lines of flux.
> 
> Really, you need to think _hard_ how to avoid invasive-looking changes 
> in late -rc's, because every extra line to review uses up precious 
> review resources.

I appreciate the goal of minimal change, but I also want to keep things 
robust.

Right now I'm thinking my other suggestion is the best. Just allocate the 
max_tr.buffer and get rid of the CONFIG. This solution is very small, and 
covers all corner cases.

-- Steve

Re: [PATCH v2 1/1] ftrace: do not update max buffer with no users

[Steven Rostedt]

On Thu, 13 Nov 2008, Steven Rostedt wrote:

> Right now I'm thinking my other suggestion is the best. Just allocate the 
> max_tr.buffer and get rid of the CONFIG. This solution is very small, and 
> covers all corner cases.

And for 29, we can change the code so that the max_tr buffer is only 
allocated by request of a tracer at plugin time. In which we can do tests 
if the max_tr.buffer exists in the use cases, and warn if it does not (a 
plugin needs to request a max_tr buffer before using update_max_tr).

-- Steve

Re: [PATCH v2 1/1] ftrace: do not update max buffer with no users

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> On Thu, 13 Nov 2008, Ingo Molnar wrote:
> > 
> > the obvious solution is to add this to ring_buffer_resize():
> > 
> > 	if (!buffer)
> > 		return size;
> 
> Having a NULL buffer return a successful resize is a bit worrisome to me.
> 
> And looking at the code I was trying to make sure could never be called
> if there are no max_tr users:
> 
> in update_max_tr
> 
> 	buf = tr->buffer;
> 	tr->buffer = max_tr.buffer;
> 	max_tr.buffer = buf;
> 
> Should all the ring buffer API return success on NULL pointers?

well, update_max_tr() is only used from tracers which also actually 
allocate a max trace buffer: irqs/preemptoff, sched-wakeup.

So, can you tell me any way how the patch below could be broken?

	Ingo

------------->
>From ee51a1de7e3837577412be269e0100038068e691 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Thu, 13 Nov 2008 14:58:31 +0100
Subject: [PATCH] tracing: fix mmiotrace resizing crash

Pekka reported a crash when resizing the mmiotrace tracer (if only
mmiotrace is enabled).

This happens because in that case we do not allocate the max buffer,
but we try to use it.

Make ring_buffer_resize() idempotent against NULL buffers.

Reported-by: Pekka Paalanen <pq@iki.fi>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/trace/ring_buffer.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 231db20..036456c 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -538,6 +538,12 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size)
 	LIST_HEAD(pages);
 	int i, cpu;
 
+	/*
+	 * Always succeed at resizing a non-existent buffer:
+	 */
+	if (!buffer)
+		return size;
+
 	size = DIV_ROUND_UP(size, BUF_PAGE_SIZE);
 	size *= BUF_PAGE_SIZE;
 	buffer_size = buffer->pages * BUF_PAGE_SIZE;

Re: [PATCH v2 1/1] ftrace: do not update max buffer with no users

[Steven Rostedt]

On Thu, 13 Nov 2008, Ingo Molnar wrote:

> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > 
> > On Thu, 13 Nov 2008, Ingo Molnar wrote:
> > > 
> > > the obvious solution is to add this to ring_buffer_resize():
> > > 
> > > 	if (!buffer)
> > > 		return size;
> > 
> > Having a NULL buffer return a successful resize is a bit worrisome to me.
> > 
> > And looking at the code I was trying to make sure could never be called
> > if there are no max_tr users:
> > 
> > in update_max_tr
> > 
> > 	buf = tr->buffer;
> > 	tr->buffer = max_tr.buffer;
> > 	max_tr.buffer = buf;
> > 
> > Should all the ring buffer API return success on NULL pointers?
> 
> well, update_max_tr() is only used from tracers which also actually 
> allocate a max trace buffer: irqs/preemptoff, sched-wakeup.

Which is what we expect. But just the fact that there's nothing protecting
update_max_tr from being used by anything else means we really can not 
prove that those are the only three users, without reviewing the code.


> 
> So, can you tell me any way how the patch below could be broken?

I just do not see how we can say resizing a NULL pointer to a given size 
succeeds without actually resizing anything.

>From the comment above the code:

/**
 * ring_buffer_resize - resize the ring buffer
 * @buffer: the buffer to resize.
 * @size: the new size.
 *
 * The tracer is responsible for making sure that the buffer is
 * not being used while changing the size.
 * Note: We may be able to change the above requirement by using
 *  RCU synchronizations.
 *
 * Minimum size is 2 * BUF_PAGE_SIZE.
 *
 * Returns -1 on failure.
 */


To me, the correct response to !buffer is a -1, not size. And a -1 would 
make the ftrace code fail. At least it would not crash.

-- Steve


> 
> 	Ingo
> 
> ------------->
> >From ee51a1de7e3837577412be269e0100038068e691 Mon Sep 17 00:00:00 2001
> From: Ingo Molnar <mingo@elte.hu>
> Date: Thu, 13 Nov 2008 14:58:31 +0100
> Subject: [PATCH] tracing: fix mmiotrace resizing crash
> 
> Pekka reported a crash when resizing the mmiotrace tracer (if only
> mmiotrace is enabled).
> 
> This happens because in that case we do not allocate the max buffer,
> but we try to use it.
> 
> Make ring_buffer_resize() idempotent against NULL buffers.
> 
> Reported-by: Pekka Paalanen <pq@iki.fi>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  kernel/trace/ring_buffer.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 231db20..036456c 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -538,6 +538,12 @@ int ring_buffer_resize(struct ring_buffer *buffer, unsigned long size)
>  	LIST_HEAD(pages);
>  	int i, cpu;
>  
> +	/*
> +	 * Always succeed at resizing a non-existent buffer:
> +	 */
> +	if (!buffer)
> +		return size;
> +
>  	size = DIV_ROUND_UP(size, BUF_PAGE_SIZE);
>  	size *= BUF_PAGE_SIZE;
>  	buffer_size = buffer->pages * BUF_PAGE_SIZE;
> 
>