Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 14 Jan 2021 14:11:28 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] printk: ringbuffer: fix line counting
Message-ID: <YABDAPYB4URXtpJD@alley>
References: <20210113144234.6545-1-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210113144234.6545-1-john.ogness@linutronix.de>
Precedence: bulk

On Wed 2021-01-13 15:48:34, John Ogness wrote:
> Counting text lines in a record simply involves counting the number
> of newline characters (+1). However, it is searching the full data
> block for newline characters, even though the text data can be (and
> often is) a subset of that area. Since the extra area in the data
> block was never initialized, the result is that extra newlines may
> be seen and counted.

Great catch!

> Restrict newline searching to the text data length.
> 
> Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

There is a note below.

> ---
>  kernel/printk/printk_ringbuffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> index 6704f06e0417..8a7b7362c0dd 100644
> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -1718,7 +1718,7 @@ static bool copy_data(struct prb_data_ring *data_ring,
>  
>  	/* Caller interested in the line count? */
>  	if (line_count)
> -		*line_count = count_lines(data, data_size);
> +		*line_count = count_lines(data, len);
>  
>  	/* Caller interested in the data content? */
>  	if (!buf || !buf_size)

Another question is what line count should be returned when
the data are copied into the buffer. In this case, the text
might get shrunken even more.

Well, this case is not supported by the API at the moment.
@line_count is defined only in prb_read_valid_info() where
the buffer is always NULL.

But we might add a WARN_ONCE() or a comment there to prevent
similar mistakes in the future.

Best Regards,
Petr