lkml.org 
[lkml]   [2014]   [Feb]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] printk: Fix discarding of records
From
On Sun, Feb 16, 2014 at 3:23 PM, Banerjee, Debabrata
<dbanerje@akamai.com> wrote:
>
> The explanation is: the loops look identical but they are not. When a
> record is printed first, its size can expand due to adding the prefix and
> timestamp. The second loop is calculating len with the first line printed
> possibly changing every iteration.

That still makes zero sense.

The size should damn well not change, because we *should* be calling
it with the exact same flags. If the size changes, something is wrong.
The logic is:

- first traverse all log indexes to find out the total length

- then traverse *again* all the indexes, until you have traversed
enough that the remainder fits in the given buffer size.

For this to make sense, that second pass absolutely *has* to use the
exact same lengths as the first pass did. Otherwise the whole logic is
totally broken.

Now, *once* you have found the right record (so that the rest should
fit), the problem is that the *third* loop (that traverses that "rest"
part) is now done with a "prev" value that does not match the original
"let's figure out the size".

So my suspicion is that the *real* bug is that

prev = 0;

before that *third* loop, because it means that the first time through
that loop (when we did *not* reset "seq/idx" to the beginning, we use
the wrong "prev" value.

So my (totally and utterly untested, and obviously whitespace-damaged)
suggested patch would be something along the lines of this:

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b1d255f04135..053c3c1a4061 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1076,7 +1076,6 @@ static int syslog_print_all(char __user
*buf, int size, bool clear)
next_seq = log_next_seq;

len = 0;
- prev = 0;
while (len >= 0 && seq < next_seq) {
struct printk_log *msg = log_from_idx(idx);
int textlen;

because at least this makes sense. It means that "prev" is only zero
when we start from "clear_idx", at all other times it's actually the
msg->flags of the previous message.

But maybe I'm missing something. That code really is messy. But
clearing "prev" there in the middle really looks wrong to me.

Linus


\
 
 \ /
  Last update: 2014-02-17 01:41    [W:0.055 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site