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: <aMg4MJU9JZ0QPYTn@pathway.suse.cz>
Date: Mon, 15 Sep 2025 18:00:48 +0200
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
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

On Mon 2025-09-15 17:13:03, 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.)

I could confirm this by my findings. By other words, a record which
needs half of the ring buffer will always fit into the ring buffer
even when it is wrapped and occupies also some unused space from
the previous wrap.

In the worst case, the unused space might be half of the ring buffer.
In this case, it would be the perfectly fitting record which would
be wrapped now. But it would be stored in the 2nd half of the ring
buffer with fix by the 1st patch from this patchset.

By other words, it would be enough to invalidate all existing
entries. data_alloc()/data_realloc() would never ask
data_push_tail() to push tail_lpos in front of head_lpos.

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

Fair enough.

To make it complete. I wondered about the reader API. But it seems
to be safe as well.

The reader API seems to be safe as long as there is at least
one descriptor in reserved state (empty ring buffer is handled
special way).

And prb_reserve() _does reserve_ a descriptor before allocating
the data. It means that a descriptor is in a reserved state
before other descriptors are made reusable to free the space.

It means that at least one descriptor is in a reserved state
even when others are reusable. And it means that _prb_read_valid()
exits in this descriptor because prb_read() returns -EINVAL...

Best Regards,
Petr