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] [day] [month] [year] [list]

Message-ID: <ca90223f-ca25-466c-a8c1-94f4f39b26cf@yandex-team.ru>
Date: Mon, 15 Sep 2025 19:07:46 +0300
From: Daniil Tatianin <d-tatianin@...dex-team.ru>
To: John Ogness <john.ogness@...utronix.de>, 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 9/15/25 6:07 PM, John Ogness wrote:
> 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.

Yeah, I think you're right. I guess the second patch can be dropped now, 
which is even better.

>
> 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