linux-kernel - Re: [PATCH] ring-buffer: fix uninitialized read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1340801169.16702.9.camel@gandalf.stny.rr.com>
Date:	Wed, 27 Jun 2012 08:46:09 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	David Sharp <dhsharp@...gle.com>
Cc:	linux-kernel@...r.kernel.org, vnagarnaik@...gle.com
Subject: Re: [PATCH] ring-buffer: fix uninitialized read_stamp

On Tue, 2012-06-26 at 17:35 -0700, David Sharp wrote:
> On Fri, Jun 22, 2012 at 6:27 PM, David Sharp <dhsharp@...gle.com> wrote:
> > On Fri, Jun 22, 2012 at 4:31 PM, Steven Rostedt <rostedt@...dmis.org> wrote:
> >>
> >> Now I think you may understand my patch.
> >
> > Yeah, mostly. At least enough that I think it's worth testing. But Monday.
> 
> I got around to testing your patch today, and it fixes the issue. No
> bad-looking timestamps in 40 runs, whereas could reproduce within 3
> runs before. Do you want me to send you a fresh patch, or just use the
> one you have?

I rather use this one for two reasons.

1) it limits the places where read_stamp is updated. And I rather not
add an update because "it fixes an anomaly".

2) I think it is wrong to force the writer on the reader page when no
write has been made. There's some side effects that this causes. One is
that if you do a read with no write, and then do nothing, it forces the
writer on that page. Now if a lot of writes happen (function tracing),
the writes that were on the reader page are never overwritten when the
buffer is full. Then you get a page of very old data, followed by a
buffer full of new data.

-- Steve

> 
> >>> On Thu, Jun 21, 2012 at 8:56 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
> >>> > Does something like this work for you. Note, this is totally untested!
> >>> >
> >>> > -- Steve
> >>> >
> >>> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> >>> > index ad0239b..5943044 100644
> >>> > --- a/kernel/trace/ring_buffer.c
> >>> > +++ b/kernel/trace/ring_buffer.c
> >>> > @@ -3246,6 +3246,10 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
> >>> >        if (cpu_buffer->commit_page == cpu_buffer->reader_page)
> >>> >                goto out;
> >>> >
> >>> > +       /* Don't bother swapping if the ring buffer is empty */
> >>> > +       if (rb_num_of_entries(cpu_buffer) == 0)
> >>> > +               goto out;
> >>> > +
> >>> >        /*
> >>> >         * Reset the reader page to size zero.
> >>> >         */


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/