[PATCH] printk: ringbuffer: fix line counting

[PATCH] printk: ringbuffer: fix line counting

[John Ogness]

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.

Restrict newline searching to the text data length.

Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 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)
-- 
2.20.1

Re: [PATCH] printk: ringbuffer: fix line counting

[Petr Mladek]

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

Re: [PATCH] printk: ringbuffer: fix line counting

[John Ogness]

On 2021-01-14, Petr Mladek <pmladek@suse.com> wrote:
>> --- 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.

Good point. The code could look like this:

        if (!buf || !buf_size) {
                data_size = len;
        } else {
                data_size = min_t(u16, buf_size, len);
                memcpy(&buf[0], data, data_size);
        }

        if (line_count)
                *line_count = count_lines(data, data_size);
                
        return true;

John Ogness

Re: [PATCH] printk: ringbuffer: fix line counting

[Sergey Senozhatsky]

On (21/01/13 15:48), 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.
> 
> Restrict newline searching to the text data length.
> 
> Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

Re: [PATCH] printk: ringbuffer: fix line counting

[Petr Mladek]

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

The patch is committed in printk/linux.git, branch printk-rework.

I plan to send it for-5.11 the following week after it spends
few days in linux-next.

Best Regards,
Petr