[PATCH 0/2] [GIT PULL][v3.7] ring-buffer: Bug fixes

[PATCH 0/2] [GIT PULL][v3.7] ring-buffer: Bug fixes

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]


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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[PATCH 1/2] ring-buffer: Fix NULL pointer if rb_set_head_page() fails

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 2118 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

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: stable@vger.kernel.org # 3.0+
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ring_buffer.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index b979426..ec01803 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -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;
 
-- 
1.7.10.4



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[v3.0 stable version][PATCH 1/2] ring-buffer: Fix NULL pointer if rb_set_head_page() fails

[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.

Cc: stable@vger.kernel.org # 3.0+
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ring_buffer.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

Index: linux-test.git/kernel/trace/ring_buffer.c
===================================================================
--- linux-test.git.orig/kernel/trace/ring_buffer.c
+++ linux-test.git/kernel/trace/ring_buffer.c
@@ -2926,6 +2926,8 @@ rb_get_reader_page(struct ring_buffer_pe
 	 * 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;
 

Re: [v3.0 stable version][PATCH 1/2] ring-buffer: Fix NULL pointer if rb_set_head_page() fails

[Greg KH]

On Fri, Nov 30, 2012 at 11:16:43AM -0500, Steven Rostedt wrote:
> 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: stable@vger.kernel.org # 3.0+
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  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?

thanks,

greg k-h

Re: [v3.0 stable version][PATCH 1/2] ring-buffer: Fix NULL pointer if rb_set_head_page() fails

[Steven Rostedt]

On Thu, 2012-12-06 at 12:13 -0800, Greg KH wrote:
> On Fri, Nov 30, 2012 at 11:16:43AM -0500, Steven Rostedt wrote:
> > 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: stable@vger.kernel.org # 3.0+
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > ---
> >  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.

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

Also, it seems that Ingo has been so busy with NUMA, that my patches
have been forgotten about. I may have to push to Linus directly :-/

-- Steve

Re: [v3.0 stable version][PATCH 1/2] ring-buffer: Fix NULL pointer if rb_set_head_page() fails

[Greg KH]

On Thu, Dec 06, 2012 at 04:41:55PM -0500, Steven Rostedt wrote:
> On Thu, 2012-12-06 at 12:13 -0800, Greg KH wrote:
> > On Fri, Nov 30, 2012 at 11:16:43AM -0500, Steven Rostedt wrote:
> > > 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: stable@vger.kernel.org # 3.0+
> > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > > ---
> > >  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.
> 
> > 
> > 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 :-)

Ok, if they are that important, consider them forgotten from my mind as
well :)

greg k-h

[v3.2-v3.4 stable version][PATCH 1/2] ring-buffer: Fix NULL pointer if rb_set_head_page() fails

[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.

Cc: stable@vger.kernel.org # 3.0+
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ring_buffer.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

Index: linux-test.git/kernel/trace/ring_buffer.c
===================================================================
--- linux-test.git.orig/kernel/trace/ring_buffer.c
+++ linux-test.git/kernel/trace/ring_buffer.c
@@ -2683,7 +2683,7 @@ unsigned long ring_buffer_oldest_event_t
 	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;
@@ -2698,7 +2698,8 @@ unsigned long ring_buffer_oldest_event_t
 		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;
@@ -3005,6 +3006,8 @@ rb_get_reader_page(struct ring_buffer_pe
 	 * 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;
 

Re: [v3.2-v3.4 stable version][PATCH 1/2] ring-buffer: Fix NULL pointer if rb_set_head_page() fails

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

On Fri, 2012-11-30 at 11:18 -0500, Steven Rostedt wrote:
> 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: stable@vger.kernel.org # 3.0+
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
[...]

Added to the queue for 3.2, thanks.

Ben.

-- 
Ben Hutchings
The world is coming to an end.	Please log off.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/2] ring-buffer: Fix NULL pointer if rb_set_head_page() fails

[Steven Rostedt]

On Fri, 2012-11-30 at 11:12 -0500, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> 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: stable@vger.kernel.org # 3.0+

Hmm, quilt didn't Cc. Grumble, I think a system update of quilt removed
my modification to not have quilt get confused by the hash symbol :-(

-- Steve

> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/trace/ring_buffer.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index b979426..ec01803 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -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;
>  

[PATCH 2/2] ring-buffer: Fix race between integrity check and readers

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

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: stable@vger.kernel.org # 3.6
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ring_buffer.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index ec01803..4cb5e51 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -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);
-- 
1.7.10.4



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 2/2] ring-buffer: Fix race between integrity check and readers

[Steven Rostedt]

On Fri, 2012-11-30 at 11:12 -0500, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> 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: stable@vger.kernel.org # 3.6

Again, quilt failed to Cc stable :-(

-- Steve

> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/trace/ring_buffer.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index ec01803..4cb5e51 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -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);

Re: [PATCH 0/2] [GIT PULL][v3.7] ring-buffer: Bug fixes

[Ingo Molnar]

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

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

Pulled, thanks Steve!

	Ingo