All of lore.kernel.org
 help / color / mirror / Atom feed
From: John Ogness <john.ogness@linutronix.de>
To: Daniil Tatianin <d-tatianin@yandex-team.ru>,
	Petr Mladek <pmladek@suse.com>
Cc: linux-kernel@vger.kernel.org,
	Steven Rostedt <rostedt@goodmis.org>,
	Sergey Senozhatsky <senozhatsky@chromium.org>
Subject: Re: [PATCH v2 0/2] printk_ringbuffer: don't needlessly wrap data blocks around
Date: Sun, 14 Sep 2025 11:29:48 +0206	[thread overview]
Message-ID: <84348pqtej.fsf@jogness.linutronix.de> (raw)
In-Reply-To: <20cbb02b-762f-4a3f-ba40-aae018388b3b@yandex-team.ru>

On 2025-09-13, Daniil Tatianin <d-tatianin@yandex-team.ru> wrote:
>> The problem comes from the function data_make_reusable(). The job of
>> this function is to push the data_ring tail forward, one data block at a
>> time, while setting the related descriptors to reusable.
>>
>> After pushing the tail forward, if it still has not pushed it far enough
>> for new requested reservation, it must push it further. For this it
>> _assumes the current position of the tail is a descriptor ID for the
>> next data block_. But what if the tail was pushed all the way to the
>> head? Then there is no next data block and it will read in garbage,
>> thinking it is the next descriptor ID to set reusable. And from there it
>> just goes crazy because it is reading garbage to determine how big the
>> data block is so that it can continue pushing the tail (beyond the
>> head!).
>>
>> Example: Assume the 96 byte ringbuffer has a single message of 64
>> bytes. Then we try to reserve space for a 72-byte
>> message. data_make_reusable() will first set the descriptor of the
>> 64-byte message to reusable and push the tail forward to index 64. But
>> the new message needs 72 bytes, so data_make_reusable() will keep going
>> and read the descriptor ID at index 64, but there is only random garbage
>> at that position. 64 is the head and there is nothing valid after it.
>
> Good catch, although I'm not sure i understand why this produces the 
> error Petr is seeing.
>
> Why would it see garbage in a zeroed .bss buffer? Is this because of
> the previous test runs dirtying the same data ring or something?

Correct. The explosions don't start happening until after about 5-6
wraps. So the data ring is full of somewhat random data that will then
be randomly interpretted once it attempts to push the tail beyond the
head.

> Either way, I guess after your patch is accepted i'm going to resend
> mine to only strip the trailing id, but the records must still be less
> than half of the data ring size so that data_make_reusable never
> invalidates past the current head.

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.

John

  reply	other threads:[~2025-09-14  9:23 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-09-05 14:41 [PATCH v2 0/2] printk_ringbuffer: don't needlessly wrap data blocks around Daniil Tatianin
2025-09-05 14:41 ` [PATCH v2 1/2] " Daniil Tatianin
2025-09-05 15:27   ` John Ogness
2025-09-05 15:29     ` Daniil Tatianin
2025-09-05 16:10       ` John Ogness
2025-09-11  8:34         ` Daniil Tatianin
2025-09-11 15:33           ` Petr Mladek
2025-09-05 15:30     ` John Ogness
2025-09-26 14:35   ` Daniil Tatianin
2025-09-26 14:44   ` Petr Mladek
2025-09-26 14:53     ` Daniil Tatianin
2025-10-22 12:46   ` Petr Mladek
2025-09-05 14:41 ` [PATCH v2 2/2] printk_ringbuffer: allow one data block to occupy the entire data ring Daniil Tatianin
2025-09-05 15:27   ` John Ogness
2025-09-11 15:30 ` [PATCH v2 0/2] printk_ringbuffer: don't needlessly wrap data blocks around Petr Mladek
2025-09-11 15:55   ` Petr Mladek
2025-09-11 16:11     ` Petr Mladek
2025-09-11 15:58   ` Petr Mladek
2025-09-11 16:12     ` John Ogness
2025-09-12  9:25       ` Petr Mladek
2025-09-12  9:54         ` Petr Mladek
2025-09-12 14:49           ` Petr Mladek
2025-09-12 15:15             ` Petr Mladek
2025-09-12 18:43             ` John Ogness
2025-09-13 17:38               ` Daniil Tatianin
2025-09-14  9:23                 ` John Ogness [this message]
2025-09-14  9:56                   ` Daniil Tatianin
2025-09-15 15:07                     ` John Ogness
2025-09-15 16:00                       ` Petr Mladek
2025-09-15 16:07                       ` Daniil Tatianin
2025-09-15 15:08               ` Petr Mladek
2025-09-15 15:25                 ` John Ogness

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=84348pqtej.fsf@jogness.linutronix.de \
    --to=john.ogness@linutronix.de \
    --cc=d-tatianin@yandex-team.ru \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pmladek@suse.com \
    --cc=rostedt@goodmis.org \
    --cc=senozhatsky@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.