Messages in this thread | | | Date | Sun, 16 Feb 2014 15:59:51 -0800 | Subject | Re: [PATCH] printk: Fix discarding of records | From | Linus Torvalds <> |
| |
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
| |