[PATCH][git pull] tracing: limit the number of loops the ring buffer self test can make

[PATCH][git pull] tracing: limit the number of loops the ring buffer self test can make

[Steven Rostedt]

Ingo,

I added one more fix. Thinking about the solution, although the
disabling of the ring buffer is good enough to prevent the bug
you hit. I became worried about a corrupted ring buffer that can
cause an inifinite loop. This patch adds a fix for that too.

Please pull the latest tip/tracing/urgent tree, which can be found at:

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


Steven Rostedt (1):
      tracing: limit the number of loops the ring buffer self test can make

----
 kernel/trace/trace_selftest.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
---------------------------
commit 4b3e3d228429c75d398f1aa24532e468d3220c49
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Wed Feb 18 22:50:01 2009 -0500

    tracing: limit the number of loops the ring buffer self test can make
    
    Impact: prevent deadlock if ring buffer gets corrupted
    
    This patch adds a paranoid check to make sure the ring buffer consumer
    does not go into an infinite loop. Since the ring buffer has been set
    to read only, the consumer should not loop for more than the ring buffer
    size. A check is added to make sure the consumer does not loop more than
    the ring buffer size.
    
    Signed-off-by: Steven Rostedt <srostedt@redhat.com>

diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
index a7e0ef6..bc8e80a 100644
--- a/kernel/trace/trace_selftest.c
+++ b/kernel/trace/trace_selftest.c
@@ -23,10 +23,20 @@ static int trace_test_buffer_cpu(struct trace_array *tr, int cpu)
 {
 	struct ring_buffer_event *event;
 	struct trace_entry *entry;
+	unsigned int loops = 0;
 
 	while ((event = ring_buffer_consume(tr->buffer, cpu, NULL))) {
 		entry = ring_buffer_event_data(event);
 
+		/*
+		 * The ring buffer is a size of trace_buf_size, if
+		 * we loop more than the size, there's something wrong
+		 * with the ring buffer.
+		 */
+		if (loops++ > trace_buf_size) {
+			printk(KERN_CONT ".. bad ring buffer ");
+			goto failed;
+		}
 		if (!trace_valid_entry(entry)) {
 			printk(KERN_CONT ".. invalid entry %d ",
 				entry->type);

Re: [PATCH][git pull] tracing: limit the number of loops the ring buffer self test can make

[Ingo Molnar]

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

> Ingo,
> 
> I added one more fix. Thinking about the solution, although 
> the disabling of the ring buffer is good enough to prevent the 
> bug you hit. I became worried about a corrupted ring buffer 
> that can cause an inifinite loop. This patch adds a fix for 
> that too.

yes, that's a good idea too:

> @@ -23,10 +23,20 @@ static int trace_test_buffer_cpu(struct trace_array *tr, int cpu)
>  {
>  	struct ring_buffer_event *event;
>  	struct trace_entry *entry;
> +	unsigned int loops = 0;
>  
>  	while ((event = ring_buffer_consume(tr->buffer, cpu, NULL))) {
>  		entry = ring_buffer_event_data(event);
>  
> +		/*
> +		 * The ring buffer is a size of trace_buf_size, if
> +		 * we loop more than the size, there's something wrong
> +		 * with the ring buffer.
> +		 */
> +		if (loops++ > trace_buf_size) {
> +			printk(KERN_CONT ".. bad ring buffer ");
> +			goto failed;
> +		}

Would be nice to also emit a WARN_ONCE() message about the test 
failure, to make sure automated tests like mine pick even 
soft-failures up.

	Ingo

Re: [PATCH][git pull] tracing: limit the number of loops the ring buffer self test can make

[Steven Rostedt]

On Thu, 19 Feb 2009, Ingo Molnar wrote:

> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Ingo,
> > 
> > I added one more fix. Thinking about the solution, although 
> > the disabling of the ring buffer is good enough to prevent the 
> > bug you hit. I became worried about a corrupted ring buffer 
> > that can cause an inifinite loop. This patch adds a fix for 
> > that too.
> 
> yes, that's a good idea too:
> 
> > @@ -23,10 +23,20 @@ static int trace_test_buffer_cpu(struct trace_array *tr, int cpu)
> >  {
> >  	struct ring_buffer_event *event;
> >  	struct trace_entry *entry;
> > +	unsigned int loops = 0;
> >  
> >  	while ((event = ring_buffer_consume(tr->buffer, cpu, NULL))) {
> >  		entry = ring_buffer_event_data(event);
> >  
> > +		/*
> > +		 * The ring buffer is a size of trace_buf_size, if
> > +		 * we loop more than the size, there's something wrong
> > +		 * with the ring buffer.
> > +		 */
> > +		if (loops++ > trace_buf_size) {
> > +			printk(KERN_CONT ".. bad ring buffer ");
> > +			goto failed;
> > +		}
> 
> Would be nice to also emit a WARN_ONCE() message about the test 
> failure, to make sure automated tests like mine pick even 
> soft-failures up.

I could put a warn on in the register_tracer code. It checks all self 
tests there, and if one fails, it disables that tracer.

-- Steve