linux-kernel - Re: [PATCH v2 0/2] printk_ringbuffer: don't needlessly wrap data blocks around

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <84wm5z7o14.fsf@jogness.linutronix.de>
Date: Mon, 15 Sep 2025 17:13:03 +0206
From: John Ogness <john.ogness@...utronix.de>
To: Daniil Tatianin <d-tatianin@...dex-team.ru>, Petr Mladek <pmladek@...e.com>
Cc: 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

On 2025-09-14, Daniil Tatianin <d-tatianin@...dex-team.ru> wrote:
>> After applying your patch, can you provide an example where a maximum
>> size of exactly half causes the tail to be pushed beyond the head? Keep
>> in mind that data_check_size() accounts for the meta-data. It only
>> doesn't account for the extra ID on wrapping data blocks.
>
> Sorry, I think exactly half is fine, basically we can keep it half, but 
> only remove the tailing id check with my patch.

I have been investigating this further. Even _without_ your patches, I
cannot find (either by using my brain or through testing) a problem with
limiting it to exactly half:

diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index bc811de18316b..9d47c1b94b71f 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -398,8 +398,6 @@ static unsigned int to_blk_size(unsigned int size)
  */
 static bool data_check_size(struct prb_data_ring *data_ring, unsigned int size)
 {
-	struct prb_data_block *db = NULL;
-
 	if (size == 0)
 		return true;

@@ -408,11 +406,7 @@ static bool data_check_size(struct prb_data_ring *data_ring, unsigned int size)
 	 * array. The largest possible data block must still leave room for
 	 * at least the ID of the next block.
 	 */
-	size = to_blk_size(size);
-	if (size > DATA_SIZE(data_ring) - sizeof(db->id))
-		return false;
-
-	return true;
+	return (to_blk_size(size) <= (DATA_SIZE(data_ring) / 2));
 }

 /* Query the state of a descriptor. */

When invalidating a data block (pushing the tail) it only must be
certain that the newly created space is large enough to fit the new data
block.

With a maximum of half, a new non-wrapping data block will always
fit. If it is a wrapping data block the worst case is if it is maximally
sized and ends exactly at the end of the array. In the case, it is
placed at index 0. But there it will only free up until the head
value. (If the head value was less, the data block would not have
wrapped.)

Your series handles the "ends exactly at the end of the array" case by
avoiding the need to wrap and thus invalidate up to half the
ringbuffer. But your series does not affect the maximum record size.

I will submit an official patch that also improves the comments to
clarify exactly why the limit exists.

@Petr: I am fine with you keeping our 1/4 limit in printk.c. But I would
like the ringbuffer code to be exactly proper here.

John