[PATCH 0/2] ring-buffer: updates for 2.6.28

[PATCH 0/2] ring-buffer: updates for 2.6.28

[Steven Rostedt]

Ingo,

These two patches should go to 2.6.28. They are fixes to the ring buffer
for bugs that will make the ring buffer stop reading, and/or produce
a false positive.

If these are too late for 2.6.28, then we probably should get them at least
to the stable branch and queued for 2.6.28.1.

The following patches are in:

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

    branch: devel


Steven Rostedt (2):
      ring-buffer: fix dangling commit race
      ring-buffer: prevent false positive warning

----
 kernel/trace/ring_buffer.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

[PATCH 1/2] ring-buffer: fix dangling commit race

[Steven Rostedt]

[-- Attachment #1: 0001-ring-buffer-fix-dangling-commit-race.patch --]
[-- Type: text/plain, Size: 1616 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: fix to stuck buffers

If an interrupt comes in during the rb_set_commit_to_write and
pushes the tail page forward just at the right time, the commit
updates will miss the adding of the interrupt data. This will
cause the commit pointer to cease from moving forward.

Thanks to Jiaying Zhang for finding this race.

Reported-by: Jiaying Zhang <jiayingz@google.com>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/ring_buffer.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 668bbb5..f64aee5 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -769,6 +769,7 @@ rb_set_commit_to_write(struct ring_buffer_per_cpu *cpu_buffer)
 	 * back to us). This allows us to do a simple loop to
 	 * assign the commit to the tail.
 	 */
+ again:
 	while (cpu_buffer->commit_page != cpu_buffer->tail_page) {
 		cpu_buffer->commit_page->commit =
 			cpu_buffer->commit_page->write;
@@ -783,6 +784,17 @@ rb_set_commit_to_write(struct ring_buffer_per_cpu *cpu_buffer)
 			cpu_buffer->commit_page->write;
 		barrier();
 	}
+
+	/* again, keep gcc from optimizing */
+	barrier();
+
+	/*
+	 * If an interrupt came in just after the first while loop
+	 * and pushed the tail page forward, we will be left with
+	 * a dangling commit that will never go forward.
+	 */
+	if (unlikely(cpu_buffer->commit_page != cpu_buffer->tail_page))
+		goto again;
 }
 
 static void rb_reset_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
-- 
1.5.6.5

-- 

[PATCH 2/2] ring-buffer: prevent false positive warning

[Steven Rostedt]

[-- Attachment #1: 0002-ring-buffer-prevent-false-positive-warning.patch --]
[-- Type: text/plain, Size: 1912 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: fix to false WARN_ON

If an interrupt goes off after the setting of the local variable
tail_page and before incrementing the write index of that page,
the interrupt could push the commit forward to the next page.

Later a check is made to see if interrupts pushed the buffer around
the entire ring buffer by comparing the next page to the last commited
page. This can produce a false positive if the interrupt had pushed
the commit page forward as stated above.

Thanks to Jiaying Zhang for finding this race.

Reported-by: Jiaying Zhang <jiayingz@google.com>
Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/ring_buffer.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index f64aee5..71202ac 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -892,12 +892,15 @@ static struct ring_buffer_event *
 __rb_reserve_next(struct ring_buffer_per_cpu *cpu_buffer,
 		  unsigned type, unsigned long length, u64 *ts)
 {
-	struct buffer_page *tail_page, *head_page, *reader_page;
+	struct buffer_page *tail_page, *head_page, *reader_page, *commit_page;
 	unsigned long tail, write;
 	struct ring_buffer *buffer = cpu_buffer->buffer;
 	struct ring_buffer_event *event;
 	unsigned long flags;
 
+	commit_page = cpu_buffer->commit_page;
+	/* we just need to protect against interrupts */
+	barrier();
 	tail_page = cpu_buffer->tail_page;
 	write = local_add_return(length, &tail_page->write);
 	tail = write - length;
@@ -921,7 +924,7 @@ __rb_reserve_next(struct ring_buffer_per_cpu *cpu_buffer,
 		 * it all the way around the buffer, bail, and warn
 		 * about it.
 		 */
-		if (unlikely(next_page == cpu_buffer->commit_page)) {
+		if (unlikely(next_page == commit_page)) {
 			WARN_ON_ONCE(1);
 			goto out_unlock;
 		}
-- 
1.5.6.5

-- 

Re: [PATCH 0/2] ring-buffer: updates for 2.6.28

[Steven Rostedt]

My scripts did not CC to the 'Reported-by' field, so they did not include 
Jiaying. Please add him to any replys to the patches. I privately forward 
them to him so he did get them.

Thanks,

-- Steve


On Tue, 23 Dec 2008, Steven Rostedt wrote:

> 
> Ingo,
> 
> These two patches should go to 2.6.28. They are fixes to the ring buffer
> for bugs that will make the ring buffer stop reading, and/or produce
> a false positive.
> 
> If these are too late for 2.6.28, then we probably should get them at least
> to the stable branch and queued for 2.6.28.1.
> 
> The following patches are in:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>     branch: devel
> 
> 
> Steven Rostedt (2):
>       ring-buffer: fix dangling commit race
>       ring-buffer: prevent false positive warning
> 
> ----
>  kernel/trace/ring_buffer.c |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)
> 
> 

Re: [PATCH 0/2] ring-buffer: updates for 2.6.28

[Ingo Molnar]

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

> Ingo,
> 
> These two patches should go to 2.6.28. They are fixes to the ring buffer 
> for bugs that will make the ring buffer stop reading, and/or produce a 
> false positive.
> 
> If these are too late for 2.6.28, then we probably should get them at 
> least to the stable branch and queued for 2.6.28.1.

yes, they are too late - the .28 release is tomorrow.

> Steven Rostedt (2):
>       ring-buffer: fix dangling commit race
>       ring-buffer: prevent false positive warning
> 
> ----
>  kernel/trace/ring_buffer.c |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)

pulled into tip/tracing/ring-buffer. I also marked them Cc: 
stable@kernel.org, so they will be picked up for .28.1. (they apply 
cleanly to both the latest tracing tree and latest mainline as well)

	Ingo