| 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
| ||
|
Message-ID: <20141119165149.GF2332@dhcp128.suse.cz>
Date: Wed, 19 Nov 2014 17:51:49 +0100
From: Petr Mladek <pmladek@...e.cz>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Jiri Kosina <jkosina@...e.cz>
Subject: Re: [PATCH 1/2] tracing: Clean up tracing_fill_pipe_page()
On Wed 2014-11-19 11:17:18, Steven Rostedt wrote:
> On Tue, 18 Nov 2014 17:15:46 +0100
> Petr Mladek <pmladek@...e.cz> wrote:
>
> > On Mon 2014-11-17 14:11:08, Steven Rostedt wrote:
> > > >
> > > > I don't like the fact that I did a code structure change with this
> > > > patch. This patch should be just a simple conversion of len to
> > > > seq_buf_used(). I'm going to strip this change out and put it before
> > > > this patch.
> > >
> > >
> > > The function tracing_fill_pipe_page() logic is a little confusing with the
> > > use of count saving the seq.len and reusing it.
> > >
> > > Instead of subtracting a number that is calculated from the saved
> > > value of the seq.len from seq.len, just save the seq.len at the start
> > > and if we need to reset it, just assign it again.
> > >
> > > When the seq_buf overflow is len == size + 1, the current logic will
> > > break. Changing it to use a saved length for resetting back to the
> > > original value is more robust and will work when we change the way
> > > seq_buf sets the overflow.
> > >
> > > Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
> > > ---
> > > kernel/trace/trace.c | 26 ++++++++++++++++++++------
> > > 1 file changed, 20 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > > index 7d7a07e9b9e9..2dbc18e5f929 100644
> > > --- a/kernel/trace/trace.c
> > > +++ b/kernel/trace/trace.c
> > > @@ -4575,23 +4575,37 @@ static size_t
> > > tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)
> > > {
> > > size_t count;
> > > + int save_len;
> > > int ret;
> > >
> > > /* Seq buffer is page-sized, exactly what we need. */
> > > for (;;) {
> > > - count = iter->seq.seq.len;
> > > + save_len = iter->seq.seq.len;
> > > ret = print_trace_line(iter);
> > > - count = iter->seq.seq.len - count;
> > > - if (rem < count) {
> > > - rem = 0;
> > > - iter->seq.seq.len -= count;
> > > +
> > > + if (trace_seq_has_overflowed(&iter->seq)) {
> > > + iter->seq.seq.len = save_len;
> > > break;
> > > }
> > > +
> > > + /*
> > > + * This should not be hit, because it should only
> > > + * be set if the iter->seq overflowed. But check it
> > > + * anyway to be safe.
> > > + */
> > > if (ret == TRACE_TYPE_PARTIAL_LINE) {
> > > - iter->seq.seq.len -= count;
> > > + iter->seq.seq.len = save_len;
> > > break;
> > > }
> >
> > The two ifs has the same body. Small optimization would be to do:
> >
> > /*
> > * The two checks should be equivalent but rather be
> > * on the safe side.
> > */
> > if (trace_seq_has_overflowed(&iter->seq) ||
> > ret == TRACE_TYPE_PARTIAL_LINE) {
> > iter->seq.seq.len = save_len;
> > break;
> > }
>
> Yeah, I originally had something like that, but I wanted to remove that
> second check. I left it separate to make it stand out as something that
> might be removed in the future. Just a preference I guess.
Fair enough.
> > To be honest, the code seems to be a bit twisted. This function
> > is called from tracing_splice_read_pipe(). It copies the
> > trace_seq buffer into spd.page and call trace_seq_init()
> > in a for cycle.
>
> Yeah, that splice code confused me too. I'll start looking at it some
> more and see if it can be fixed up.
>
> >
> > Therefore if the overflow happens, trace_find_next_entry_inc()
> > is not called in tracing_fill_pipe_page() and we work with the same
> > iterator instance next time. It means that the overflow happens most
> > likely again and we fill all remaining spd.pages with no data and
> > are stacked on the same iterator instance.
>
> Luckily, overflows never happen. But if they do, things might break.
I thought so. :-)
> >
> > BTW: The trace_seq_to_buffer() in tracing_splice_read_pipe()
> > is suspicious as well. It passes trace_seq_used(&iter->seq)
> > as the "cnt" parameter. I guess that it should pass the
> > size of the "spd.page" instead.
>
> Wow, I should have looked harder at that code when I accepted it. It
> just "worked" and I was happy. Oh well, another thing to fix up.
>
> >
> > Also we should somehow handle the situation when some data are not
> > copied. Well, it seems the spd.page has the page size, so it is
> > the same size as the trace_seq buffer.
> >
> >
> > Well, this patch does not change the behavior. We could solve the
> > above problem later if it is there. Maybe I got it wrong.
>
> No, that code doesn't look too good. That's some old stuff that got in
> when we were still learning, and if it worked, we added it ;-)
>
> That needs to be cleaned up. I'll put it on my ever growing todo
> list.
I believe. I am a bit scared to put it on my todo list because these
kind of working things tend to just fall down.
> Of course if you want to clean it up, feel free to send some patches on
> top of this. That is, if we get the OK from Linus or Andrew.
OK, I'll put it on my todo list. Let's see who is faster ;-) And I
keep my fingers crossed about the OK from Linus and Andrew.
Best Regards,
Petr
--
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/
Powered by blists - more mailing lists