Message-ID: <84a52zy0iu.fsf@jogness.linutronix.de>
Date: Fri, 12 Sep 2025 20:49:37 +0206
From: John Ogness <john.ogness@...utronix.de>
To: Petr Mladek <pmladek@...e.com>
Cc: Daniil Tatianin <d-tatianin@...dex-team.ru>,
 linux-kernel@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>, Sergey
 Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [PATCH v2 0/2] printk_ringbuffer: don't needlessly wrap data
 blocks around

Hi Petr,

Summary: printk() is not in danger but we should correct a loose bounds
check.

On 2025-09-12, Petr Mladek <pmladek@...e.com> wrote:
> Honestly, I would really like to limit the maximal record size to
> 1/4 of the buffer size. I do not want to make the design more
> complicated just to be able to fill just one record, definitely.

So I was able to track this down. Your usage of

DEFINE_PRINTKRB(test_rb, 4, 4);

actually made it relatively easy because there are only 16
descriptors. All I needed to do was dump the descriptors before each
reserve, between reserve and commit, after commit, and when reserve
fails. This allowed me to easily see exactly how the ringbuffer is
behaving.

The problem can be reproduced with a single writer, no reader
needed. Using

#define MAX_RBDATA_TEXT_SIZE (0x256 - sizeof(struct prbtest_rbdata))

provides a wild range of attempts that trigger the problem within about
20 write cycles.

The problem comes from the function data_make_reusable(). The job of
this function is to push the data_ring tail forward, one data block at a
time, while setting the related descriptors to reusable.

After pushing the tail forward, if it still has not pushed it far enough
for new requested reservation, it must push it further. For this it
_assumes the current position of the tail is a descriptor ID for the
next data block_. But what if the tail was pushed all the way to the
head? Then there is no next data block and it will read in garbage,
thinking it is the next descriptor ID to set reusable. And from there it
just goes crazy because it is reading garbage to determine how big the
data block is so that it can continue pushing the tail (beyond the
head!).

Example: Assume the 96 byte ringbuffer has a single message of 64
bytes. Then we try to reserve space for a 72-byte
message. data_make_reusable() will first set the descriptor of the
64-byte message to reusable and push the tail forward to index 64. But
the new message needs 72 bytes, so data_make_reusable() will keep going
and read the descriptor ID at index 64, but there is only random garbage
at that position. 64 is the head and there is nothing valid after it.

This situation can never happen for printk because of your 1/4 limit
(MAX_LOG_TAKE_PART), although it is over-conservative. It is enough to
limit messages to 1/2 of the data ring (with Daniil's series). Otherwise
the limit must be "1/2 - sizeof(long)" to also leave room for the
trailing ID of a wrapping data block.

I am still positive about Daniil's series. And we should fix
data_check_size() to be provide a proper limit as well as describe the
critical relationship between data_check_size() and
data_make_reusable().

I prefer not modify data_make_reusable() to handle this case. Currently
data_make_reusable() does nothing with the head, so it would introduce
new memory barriers. Also, the "push tail beyond head" scenario is a bit
odd to handle. It is better just to document the assumption and put in
the correct bounds checks.

I will put together a patch without Daniil's series that improves the
comments and puts a proper bound on data_check_size().

John Ogness

Powered by blists - more mailing lists