Skip to contentSkip to conversation

ring_buffer: wrap a list.next reference with rb_list_head()

PATCH In review David Sharp 2 participants 3 messages Jan 7, 2010 – Jan 9, 2010

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ring_buffer: wrap a list.next reference with rb_list_head()

This reference at the end of rb_get_reader_page() was causing off-by-one writes to the prev pointer of the page after the reader page when that page is the head page, and therefore the reader page has the RB_PAGE_HEAD flag in its list.next pointer. This eventually results in a GPF in a subsequent call to rb_set_head_page() (usually from rb_get_reader_page()) when that prev pointer is dereferenced. The dereferenced register would characteristically have an address that appears shifted left by one byte (eg, ffxxxxxxxxxxxxyy instead of ffffxxxxxxxxxxxx) due to being written at an address one byte too high.

---
 kernel/trace/ring_buffer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/trace/ring_buffer.c +1 -1
index 2326b04..d5b7308 100644
@@ -2906,7 +2906,7 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 	 *
 	 * Now make the new head point back to the reader page.
 	 */
-	reader->list.next->prev = &cpu_buffer->reader_page->list;
+	rb_list_head(reader->list.next)->prev = &cpu_buffer->reader_page->list;
 	rb_inc_page(cpu_buffer, &cpu_buffer->head_page);
 
 	/* Finally update the reader page to the new head */
Signature · 1 line
1.6.5.3
#1 lore raw

Just a FYI, it's best to send me email to my [email protected] account. I don't always read my messages on my redhat account.

Quoted 20 lines · David Sharp

This reference at the end of rb_get_reader_page() was causing off-by-one writes to the prev pointer of the page after the reader page when that page is the head page, and therefore the reader page has the RB_PAGE_HEAD flag in its list.next pointer. This eventually results in a GPF in a subsequent call to rb_set_head_page() (usually from rb_get_reader_page()) when that prev pointer is dereferenced. The dereferenced register would characteristically have an address that appears shifted left by one byte (eg, ffxxxxxxxxxxxxyy instead of ffffxxxxxxxxxxxx) due to being written at an address one byte too high.

---
 kernel/trace/ring_buffer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/trace/ring_buffer.c +0 -0
index 2326b04..d5b7308 100644

kernel/trace/ring_buffer.c rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer) · line 2910

Quoted 6 lines
kernel/trace/ring_buffer.c +1 -1
@@ -2906,7 +2906,7 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 	 *
 	 * Now make the new head point back to the reader page.
 	 */
-	reader->list.next->prev = &cpu_buffer->reader_page->list;
+	rb_list_head(reader->list.next)->prev = &cpu_buffer->reader_page->list;
 	rb_inc_page(cpu_buffer, &cpu_buffer->head_page);

Wow, this is a nasty little race. The writer had to move the header page forward by one between the time we swapped to now, which is the command just outside the diff.

kernel/trace/ring_buffer.c rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer) · line 2911

Quoted 2 lines
kernel/trace/ring_buffer.c +0 -0
 	/* Finally update the reader page to the new head */

We probably should add a change at the top too, just to be safe.

kernel/trace/ring_buffer.c +1 -1
index 2326b04..edefe3b 100644
@@ -2869,7 +2869,7 @@ 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);
-	cpu_buffer->reader_page->list.next = reader->list.next;
+	cpu_buffer->reader_page->list.next = rb_list_head(reader->list.next);
 	cpu_buffer->reader_page->list.prev = reader->list.prev;
 
 	/*

Thanks, I'll apply this.

-- Steve

#2 lore raw

[tip:tracing/urgent] ring-buffer: Wrap a list.next reference with rb_list_head()

In reply to David Sharp
Commit-ID:  5ded3dc6a3c7549b36a8ac27bbd81b33756a2c29
Gitweb:     http://git.kernel.org/tip/5ded3dc6a3c7549b36a8ac27bbd81b33756a2c29
Author:     David Sharp <[email protected]>
AuthorDate: Wed, 6 Jan 2010 17:12:07 -0800
Committer:  Steven Rostedt <[email protected]>
CommitDate: Wed, 6 Jan 2010 20:38:25 -0500

ring-buffer: Wrap a list.next reference with rb_list_head()

This reference at the end of rb_get_reader_page() was causing off-by-one writes to the prev pointer of the page after the reader page when that page is the head page, and therefore the reader page has the RB_PAGE_HEAD flag in its list.next pointer. This eventually results in a GPF in a subsequent call to rb_set_head_page() (usually from rb_get_reader_page()) when that prev pointer is dereferenced. The dereferenced register would characteristically have an address that appears shifted left by one byte (eg, ffxxxxxxxxxxxxyy instead of ffffxxxxxxxxxxxx) due to being written at an address one byte too high.

LKML-Reference: <[email protected]>

---
 kernel/trace/ring_buffer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/trace/ring_buffer.c +1 -1
index 2326b04..d5b7308 100644
@@ -2906,7 +2906,7 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 	 *
 	 * Now make the new head point back to the reader page.
 	 */
-	reader->list.next->prev = &cpu_buffer->reader_page->list;
+	rb_list_head(reader->list.next)->prev = &cpu_buffer->reader_page->list;
 	rb_inc_page(cpu_buffer, &cpu_buffer->head_page);
 
 	/* Finally update the reader page to the new head */