ring-buffer: updates for 2.6.28

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

From: Steven Rostedt <[email protected]>

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.

---
 kernel/trace/ring_buffer.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

index 668bbb5..f64aee5 100644
@@ -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

-- 

From: Steven Rostedt <[email protected]>

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.

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

index f64aee5..71202ac 100644
@@ -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

-- 

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

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

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

Ingo