Re: [PATCH] printk: Fix discarding of records

[Linus Torvalds]

On Sun, Feb 16, 2014 at 4:19 PM, Banerjee, Debabrata
<dbanerje@xxxxxxxxxx> wrote:
>
> No that can't be right, the prev value after every loop is the msg->flags
> from the *last* line in the list, which has no relation to the *first*, so
> reusing it for the top of the next loop is nonsense.

Please, Debabrata, humor me, and just try the patch.

And try reading the source code. Because your statement is BS.

The third loop does *not* start again from the first line! It
*continues* from where the second loop ended. Which is exactly why
clearing "prev" is *wrong*. Because the *last* line that the second
loop processes is indeed the previous line before the *first* line
that the third loop starts processing.

No, I haven't tested my patch, and maybe it's broken for some subtle
reason I'm missing too. But neither of your patches really make sense,
although I can believe that your second patch happens to get the
buffer size right almost by accident. Why? It's by virtue of the
"prefix" for a line generally being the same length, so when you
discount the prefix of the last line that you *don't* print, you by
accident get as much room as the - extraneous - prefix of the *next*
line that then actually gets copied. See?

(Btw, just to clarify: kmsg_dump_get_buffer() has the exact same logic
and the exact same bug, so as with your patches, you should remove the
"prev = 0" from before the third loop from that function too. Because
exactly as with syslog_print_all(), the third loop *continues* where
the second loop stops, and thus clearing "prev" is actually wrong).

That extended patch attached, now without whitespace damage. But still
completely and utterly untested.

                         Linus
 kernel/printk/printk.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b1d255f04135..4dae9cbe9259 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;
@@ -2788,7 +2787,6 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper, bool syslog,
 	next_idx = idx;
 
 	l = 0;
-	prev = 0;
 	while (seq < dumper->next_seq) {
 		struct printk_log *msg = log_from_idx(idx);