ring-buffer: Bug fixes

The function rb_set_head_page() searches the list of ring buffer pages for a the page that has the HEAD page flag set. If it does not find it, it will do a WARN_ON(), disable the ring buffer and return NULL, as this should never happen.

But if this bug happens to happen, not all callers of this function can handle a NULL pointer being returned from it. That needs to be fixed.

Cc [email protected] # 3.0+

Signed-off-by Steven Rostedt <[email protected]>

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

Quoted 9 lines · Steven Rostedt

The function rb_set_head_page() searches the list of ring buffer pages for a the page that has the HEAD page flag set. If it does not find it, it will do a WARN_ON(), disable the ring buffer and return NULL, as this should never happen.

But if this bug happens to happen, not all callers of this function can handle a NULL pointer being returned from it. That needs to be fixed.

Quoted 5 lines

Cc [email protected] # 3.0+

Signed-off-by Steven Rostedt <[email protected]>

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

I don't understand why you sent these to stable@ right now, I don't see them in Linus's tree, so why are they needed here?

Care to resend them when they hit Linus's tree?

Quoted 10 lines · Greg KH

Quoted 9 lines · Steven Rostedt

The function rb_set_head_page() searches the list of ring buffer pages for a the page that has the HEAD page flag set. If it does not find it, it will do a WARN_ON(), disable the ring buffer and return NULL, as this should never happen.

But if this bug happens to happen, not all callers of this function can handle a NULL pointer being returned from it. That needs to be fixed.

Quoted 8 lines

Quoted 5 lines

Cc [email protected] # 3.0+

Signed-off-by Steven Rostedt <[email protected]>

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

I don't understand why you sent these to stable@ right now, I don't see them in Linus's tree, so why are they needed here?

Answer below.

Quoted 1 line

Quoted 1 line

Care to resend them when they hit Linus's tree?

I'll try to remember, but I will most likely forget :-)

I wrote the patches for each stable version while they were still fresh in my mind. I posted them so they would be there when they get into mainline and be easier to port for stable. By the time they are in mainline, they will be completely out of my mind :-)

The function rb_set_head_page() searches the list of ring buffer pages for a the page that has the HEAD page flag set. If it does not find it, it will do a WARN_ON(), disable the ring buffer and return NULL, as this should never happen.

But if this bug happens to happen, not all callers of this function can handle a NULL pointer being returned from it. That needs to be fixed.

Cc [email protected] # 3.0+

Signed-off-by Steven Rostedt <[email protected]>

From: Steven Rostedt <[email protected]>

The function rb_set_head_page() searches the list of ring buffer pages for a the page that has the HEAD page flag set. If it does not find it, it will do a WARN_ON(), disable the ring buffer and return NULL, as this should never happen.

But if this bug happens to happen, not all callers of this function can handle a NULL pointer being returned from it. That needs to be fixed.

Cc [email protected] # 3.0+

Signed-off-by Steven Rostedt <[email protected]>

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

kernel/trace/ring_buffer.c +7 -2

index b979426..ec01803 100644
@@ -1396,6 +1396,8 @@ rb_insert_pages(struct ring_buffer_per_cpu *cpu_buffer)
 		struct list_head *head_page_with_bit;
 
 		head_page = &rb_set_head_page(cpu_buffer)->list;
+		if (!head_page)
+			break;
 		prev_page = head_page->prev;
 
 		first_page = pages->next;

@@ -2934,7 +2936,7 @@ unsigned long ring_buffer_oldest_event_ts(struct ring_buffer *buffer, int cpu)
 	unsigned long flags;
 	struct ring_buffer_per_cpu *cpu_buffer;
 	struct buffer_page *bpage;
-	unsigned long ret;
+	unsigned long ret = 0;
 
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
 		return 0;

@@ -2949,7 +2951,8 @@ unsigned long ring_buffer_oldest_event_ts(struct ring_buffer *buffer, int cpu)
 		bpage = cpu_buffer->reader_page;
 	else
 		bpage = rb_set_head_page(cpu_buffer);
-	ret = bpage->page->time_stamp;
+	if (bpage)
+		ret = bpage->page->time_stamp;
 	raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
 
 	return ret;

@@ -3260,6 +3263,8 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 	 * Splice the empty reader page into the list around the head.
 	 */
 	reader = rb_set_head_page(cpu_buffer);
+	if (!reader)
+		goto out;
 	cpu_buffer->reader_page->list.next = rb_list_head(reader->list.next);
 	cpu_buffer->reader_page->list.prev = reader->list.prev;
 

From: Steven Rostedt <[email protected]>

The function rb_check_pages() was added to make sure the ring buffer's pages were sane. This check is done when the ring buffer size is modified as well as when the iterator is released (closing the "trace" file), as that was considered a non fast path and a good place to do a sanity check.

The problem is that the check does not have any locks around it. If one process were to read the trace file, and another were to read the raw binary file, the check could happen while the reader is reading the file.

The issues with this is that the check requires to clear the HEAD page before doing the full check and it restores it afterward. But readers require the HEAD page to exist before it can read the buffer, otherwise it gives a nasty warning and disables the buffer.

By adding the reader lock around the check, this keeps the race from happening.

Cc [email protected] # 3.6

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/ring_buffer.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

kernel/trace/ring_buffer.c +6 -1

index ec01803..4cb5e51 100644
@@ -3783,12 +3783,17 @@ void
 ring_buffer_read_finish(struct ring_buffer_iter *iter)
 {
 	struct ring_buffer_per_cpu *cpu_buffer = iter->cpu_buffer;
+	unsigned long flags;
 
 	/*
 	 * Ring buffer is disabled from recording, here's a good place
-	 * to check the integrity of the ring buffer. 
+	 * to check the integrity of the ring buffer.
+	 * Must prevent readers from trying to read, as the check
+	 * clears the HEAD page and readers require it.
 	 */
+	raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
 	rb_check_pages(cpu_buffer);
+	raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
 
 	atomic_dec(&cpu_buffer->record_disabled);
 	atomic_dec(&cpu_buffer->buffer->resize_disabled);

Ingo,

This is based off of my last urgent pull request.

Please pull the latest tip/perf/urgent-2 tree, which can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/perf/urgent-2

Head SHA1: 9366c1ba13fbc41bdb57702e75ca4382f209c82f

Steven Rostedt (2):
ring-buffer: Fix NULL pointer if rb_set_head_page() fails
ring-buffer: Fix race between integrity check and readers

----

 kernel/trace/ring_buffer.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)