Re: [PATCH] printk: Fix discarding of records

[Linus Torvalds]

On Sun, Feb 16, 2014 at 3:23 PM, Banerjee, Debabrata
<dbanerje@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/