linux-kernel - Re: [PATCH v1] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <84ikhxfejl.fsf@jogness.linutronix.de>
Date: Fri, 05 Sep 2025 11:19:02 +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 v1] printk_ringbuffer: don't needlessly wrap data blocks
 around

On 2025-09-04, Petr Mladek <pmladek@...e.com> wrote:
> On Thu 2025-09-04 16:04:30, John Ogness wrote:
>> > +static bool same_lpos_wraps(struct prb_data_ring *data_ring,
>> > +			     unsigned long begin_lpos, unsigned long next_lpos)
>> 
>> We need a better name here since it is not actually using the value of
>> @next_lpos to check the wrap count. Perhaps inverting the return value
>> and naming it blk_lpos_wraps(). So it would be identifying if the given
>> blk_lpos values lead to a wrapping data block.
>
> Or a combination of my and this proposal: is_blk_wrapped().

Sounds good to me.

>> The rest looked fine to me and also passed various private
>> tests. However, we should also update data_check_size(), since now data
>> blocks are allowed to occupy the entire data ring. Something like this:
>> 
>> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
>> index d9fb053cff67d..e6bdfb8237a3d 100644
>> --- a/kernel/printk/printk_ringbuffer.c
>> +++ b/kernel/printk/printk_ringbuffer.c
>> @@ -397,21 +397,14 @@ 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;
>>  
>>  	/*
>>  	 * Ensure the alignment padded size could possibly fit in the data
>> -	 * array. The largest possible data block must still leave room for
>> -	 * at least the ID of the next block.
>> +	 * array.
>>  	 */
>> -	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));
>>  }
>
> I hope that we would never reach this limit. A buffer for one
> message does not look practical. I originally suggested to avoid
> messages bigger than 1/4 of the buffer size ;-)
>
> That said, strictly speaking, the above change looks correct.
> I would just do it in a separate patch. The use of the full
> buffer and the limit of the maximal message are related
> but they are not the same things. Also separate patch might
> help with bisection in case of problems.

FWIW, Aside from inspecting all the related code carefully, I also
performed various size and boundary tests using small buffers (like 1KB
and 2KB). I am confortable with these changes.

John