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

[Petr Mladek <pmladek@suse.com>]

On Thu 2025-09-11 17:55:49, Petr Mladek wrote:
> On Thu 2025-09-11 17:30:36, Petr Mladek wrote:
> > On Fri 2025-09-05 17:41:50, Daniil Tatianin wrote:
> > > This series fixes the issue where data blocks would wrap in cases where the last
> > > data block perfectly fits the ring. This caused whatever was at the beginning of
> > > the ring to get discarded in this case, and the data block would get put there
> > > even though it could be put at the end of the data ring just fine without
> > > discarding anything.
> > > 
> > > Fixing this issue also allows to simplify the check in data_check_size,
> > > previously it would ensure there's space for a trailing id, which we
> > > don't need anymore.
> > > 
> > > v0->v1:
> > > - Fix severely broken code alignment
> > > 
> > > v1->v2:
> > > - Rename & invert get_next_lpos -> is_blk_wrapped
> > > - Add a new commit for changing the logic in data_check_size
> > 
> > The patchset looks good to me. But I wanted to do some tests
> > and it failed. I did the following:
> > 
> > 1. Applied this patchset on top of printk/linux.git, branch for-next,
> >    https://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git/log/?h=for-next
> > 
> >    I this branch because it contains a new KUnit test for the printk
> >    ring buffer.
> 
> The KUnit test fails even without this patchset, see below.
> 
> > 2. I applied the following patch:
> > 
> >        + It reduces the size of the data ring. If I count it correctly
> > 	 it should be 256 (2 << 8).
> > 
> >        + It increases the maximal size of the text so that the maximal
> > 	 record size is 256.
> > 
> > 3. I built it with Kasan enabled:
> > 
> > 	$> grep KASAN .config
> > 	CONFIG_KASAN_SHADOW_OFFSET=0xdffffc0000000000
> > 	CONFIG_HAVE_ARCH_KASAN=y
> > 	CONFIG_HAVE_ARCH_KASAN_VMALLOC=y
> > 	CONFIG_CC_HAS_KASAN_GENERIC=y
> > 	CONFIG_CC_HAS_KASAN_SW_TAGS=y
> > 	CONFIG_KASAN=y
> > 	CONFIG_CC_HAS_KASAN_MEMINTRINSIC_PREFIX=y
> > 	CONFIG_KASAN_GENERIC=y
> > 	# CONFIG_KASAN_OUTLINE is not set
> > 	CONFIG_KASAN_INLINE=y
> > 	CONFIG_KASAN_STACK=y
> > 	CONFIG_KASAN_VMALLOC=y
> > 	# CONFIG_KASAN_KUNIT_TEST is not set
> > 	# CONFIG_KASAN_EXTRA_INFO is not set
> > 
> > 
> > 4. I loaded the test module:
> > 
> > 	# depmod
> > 	# modprobe printk_ringbuffer_kunit_test
> > 
> > I am not sure if it is caused by this patchset or
> 
> Hmm, the KUnit test fails even after reverting this patchset.

I do not longer [*] see the problem with the following change in the
original code:

diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index bc811de18316..ff93c4a079f7 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -409,7 +409,7 @@ static bool data_check_size(struct prb_data_ring *data_ring, unsigned int size)
 	 * at least the ID of the next block.
 	 */
 	size = to_blk_size(size);
-	if (size > DATA_SIZE(data_ring) - sizeof(db->id))
+	if (size > (DATA_SIZE(data_ring) / 4))
 		return false;
 
 	return true;

[*] I did 10 runs on the Knuit test. The problem was always
    reproducible before.

I hope that the race happens only when one record uses more than
half of the data ring. Or something like this. So that printk()
could not meet it in practice.

Sigh.

Best Regards,
Petr