[GIT PULL] ring-buffer: Fix infinite spin in reading buffer

[GIT PULL] ring-buffer: Fix infinite spin in reading buffer

[Steven Rostedt]

Linus,

While testing some new changes for 3.18, I kept hitting a bug every so
often in the ring buffer. At first I thought it had to do with some
of the changes I was working on, but then testing something else I
realized that the bug was in 3.17 itself. I ran several bisects as the
bug was not very reproducible, and finally came up with the commit
that I could reproduce easily within a few minutes, and without the change
I could run the tests over an hour without issue. The change fit the
bug and I figured out a fix. That bad commit was:

Commit 651e22f2701b "ring-buffer: Always reset iterator to reader page"

This commit fixed a bug, but in the process created another one. It used
the wrong value as the cached value that is used to see if things changed
while an iterator was in use. This made it look like a change always
happened, and could cause the iterator to go into an infinite loop.


Greg (and stable et al),

This fixes a commit that was marked for stable as far back as 2.6.28.
This patch needs to be added to all stable trees that included the
first fix. Obviously after Linus applies it.

Please pull the latest trace-fixes-v3.17-rc7 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-fixes-v3.17-rc7

Tag SHA1: 0c08f2a68c694e7d95dcf2109dc08772056b4746
Head SHA1: 24607f114fd14f2f37e3e0cb3d47bce96e81e848


Steven Rostedt (Red Hat) (1):
      ring-buffer: Fix infinite spin in reading buffer

----
 kernel/trace/ring_buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
---------------------------
commit 24607f114fd14f2f37e3e0cb3d47bce96e81e848
Author: Steven Rostedt (Red Hat) <rostedt@goodmis.org>
Date:   Thu Oct 2 16:51:18 2014 -0400

    ring-buffer: Fix infinite spin in reading buffer
    
    Commit 651e22f2701b "ring-buffer: Always reset iterator to reader page"
    fixed one bug but in the process caused another one. The reset is to
    update the header page, but that fix also changed the way the cached
    reads were updated. The cache reads are used to test if an iterator
    needs to be updated or not.
    
    A ring buffer iterator, when created, disables writes to the ring buffer
    but does not stop other readers or consuming reads from happening.
    Although all readers are synchronized via a lock, they are only
    synchronized when in the ring buffer functions. Those functions may
    be called by any number of readers. The iterator continues down when
    its not interrupted by a consuming reader. If a consuming read
    occurs, the iterator starts from the beginning of the buffer.
    
    The way the iterator sees that a consuming read has happened since
    its last read is by checking the reader "cache". The cache holds the
    last counts of the read and the reader page itself.
    
    Commit 651e22f2701b changed what was saved by the cache_read when
    the rb_iter_reset() occurred, making the iterator never match the cache.
    Then if the iterator calls rb_iter_reset(), it will go into an
    infinite loop by checking if the cache doesn't match, doing the reset
    and retrying, just to see that the cache still doesn't match! Which
    should never happen as the reset is suppose to set the cache to the
    current value and there's locks that keep a consuming reader from
    having access to the data.
    
    Fixes: 651e22f2701b "ring-buffer: Always reset iterator to reader page"
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index b38fb2b9e237..2d75c94ae87d 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -3359,7 +3359,7 @@ static void rb_iter_reset(struct ring_buffer_iter *iter)
 	iter->head = cpu_buffer->reader_page->read;
 
 	iter->cache_reader_page = iter->head_page;
-	iter->cache_read = iter->head;
+	iter->cache_read = cpu_buffer->read;
 
 	if (iter->head)
 		iter->read_stamp = cpu_buffer->read_stamp;

Re: [GIT PULL] ring-buffer: Fix infinite spin in reading buffer

[Greg Kroah-Hartman]

On Fri, Oct 03, 2014 at 04:20:43PM -0400, Steven Rostedt wrote:
> 
> Linus,
> 
> While testing some new changes for 3.18, I kept hitting a bug every so
> often in the ring buffer. At first I thought it had to do with some
> of the changes I was working on, but then testing something else I
> realized that the bug was in 3.17 itself. I ran several bisects as the
> bug was not very reproducible, and finally came up with the commit
> that I could reproduce easily within a few minutes, and without the change
> I could run the tests over an hour without issue. The change fit the
> bug and I figured out a fix. That bad commit was:
> 
> Commit 651e22f2701b "ring-buffer: Always reset iterator to reader page"
> 
> This commit fixed a bug, but in the process created another one. It used
> the wrong value as the cached value that is used to see if things changed
> while an iterator was in use. This made it look like a change always
> happened, and could cause the iterator to go into an infinite loop.
> 
> 
> Greg (and stable et al),
> 
> This fixes a commit that was marked for stable as far back as 2.6.28.
> This patch needs to be added to all stable trees that included the
> first fix. Obviously after Linus applies it.
> 
> Please pull the latest trace-fixes-v3.17-rc7 tree, which can be found at:
> 
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> trace-fixes-v3.17-rc7
> 
> Tag SHA1: 0c08f2a68c694e7d95dcf2109dc08772056b4746
> Head SHA1: 24607f114fd14f2f37e3e0cb3d47bce96e81e848
> 
> 
> Steven Rostedt (Red Hat) (1):
>       ring-buffer: Fix infinite spin in reading buffer
> 
> ----
>  kernel/trace/ring_buffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> ---------------------------
> commit 24607f114fd14f2f37e3e0cb3d47bce96e81e848
> Author: Steven Rostedt (Red Hat) <rostedt@goodmis.org>
> Date:   Thu Oct 2 16:51:18 2014 -0400
> 
>     ring-buffer: Fix infinite spin in reading buffer
>     
>     Commit 651e22f2701b "ring-buffer: Always reset iterator to reader page"
>     fixed one bug but in the process caused another one. The reset is to
>     update the header page, but that fix also changed the way the cached
>     reads were updated. The cache reads are used to test if an iterator
>     needs to be updated or not.
>     
>     A ring buffer iterator, when created, disables writes to the ring buffer
>     but does not stop other readers or consuming reads from happening.
>     Although all readers are synchronized via a lock, they are only
>     synchronized when in the ring buffer functions. Those functions may
>     be called by any number of readers. The iterator continues down when
>     its not interrupted by a consuming reader. If a consuming read
>     occurs, the iterator starts from the beginning of the buffer.
>     
>     The way the iterator sees that a consuming read has happened since
>     its last read is by checking the reader "cache". The cache holds the
>     last counts of the read and the reader page itself.
>     
>     Commit 651e22f2701b changed what was saved by the cache_read when
>     the rb_iter_reset() occurred, making the iterator never match the cache.
>     Then if the iterator calls rb_iter_reset(), it will go into an
>     infinite loop by checking if the cache doesn't match, doing the reset
>     and retrying, just to see that the cache still doesn't match! Which
>     should never happen as the reset is suppose to set the cache to the
>     current value and there's locks that keep a consuming reader from
>     having access to the data.
>     
>     Fixes: 651e22f2701b "ring-buffer: Always reset iterator to reader page"
>     Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Next time, please also add a Cc: stable...  here so that my tools pick
it up automatically.

greg k-h

Re: [GIT PULL] ring-buffer: Fix infinite spin in reading buffer

[Steven Rostedt]

On Sun, 5 Oct 2014 16:49:43 -0700
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

  
> >     Fixes: 651e22f2701b "ring-buffer: Always reset iterator to reader page"
> >     Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Next time, please also add a Cc: stable...  here so that my tools pick
> it up automatically.

Sure. I was a bit confused by exactly how to do that as it was a fix to
something that went to stable but wasn't in the original release.

But thinking about it now, I guess a Cc: stable without a version would
have sufficed. I'm too much in the habit of always adding a version and
didn't know what to add for this change as it was for 3.17-rcX. I guess
I just thought too hard about what to do.

-- Steve

Re: [GIT PULL] ring-buffer: Fix infinite spin in reading buffer

[Chuck Ebbert]

On Sun, 5 Oct 2014 16:49:43 -0700
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Fri, Oct 03, 2014 at 04:20:43PM -0400, Steven Rostedt wrote:

...

> >     Fixes: 651e22f2701b "ring-buffer: Always reset iterator to reader page"
> >     Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Next time, please also add a Cc: stable...  here so that my tools pick
> it up automatically.
> 

Can you add "Fixes:" to the list of keywords your tools pick up, and
determine if the patch is needed in -stable by looking at the commit ID
that's being fixed? Some authors might not remember if the thing being
fixed has made it into an older release via -stable.

Re: [GIT PULL] ring-buffer: Fix infinite spin in reading buffer

[Greg Kroah-Hartman]

On Tue, Oct 07, 2014 at 09:40:11PM -0500, Chuck Ebbert wrote:
> On Sun, 5 Oct 2014 16:49:43 -0700
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Fri, Oct 03, 2014 at 04:20:43PM -0400, Steven Rostedt wrote:
> 
> ...
> 
> > >     Fixes: 651e22f2701b "ring-buffer: Always reset iterator to reader page"
> > >     Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > 
> > Next time, please also add a Cc: stable...  here so that my tools pick
> > it up automatically.
> > 
> 
> Can you add "Fixes:" to the list of keywords your tools pick up, and
> determine if the patch is needed in -stable by looking at the commit ID
> that's being fixed? Some authors might not remember if the thing being
> fixed has made it into an older release via -stable.

I do that as well, but I don't advertise it as much, as it's easier for
me with the cc: stable marking :)

thanks,

greg k-h