* [PATCH] ring-buffer: Let the reader spin on head page update
@ 2026-11-11 16:06 Vincent Donnefort
2026-11-11 16:55 ` sashiko-bot
0 siblings, 1 reply; 3+ messages in thread
From: Vincent Donnefort @ 2026-11-11 16:06 UTC (permalink / raw)
To: rostedt, mhiramat, linux-trace-kernel
Cc: mathieu.desnoyers, kernel-team, linux-kernel, Vincent Donnefort,
Breno Leitao, stable
When a writer hits the head page, it needs to move it. The update
happens in two steps: first it swaps the HEAD link for UPDATE and then
swaps the following from NORMAL to HEAD. However, if a stale
cpu_buffer->head_page sits past the UPDATE link, rb_set_head_page() will
find HEAD directly and allow swapping.
This is where things gets hairy. If a outer update is interrupted before
setting HEAD, this nested writer finishes the update first. Another CPU
running trace_pipe can swap that newly installed head page with its
reader page. Now back to the outer writer. It resumes and tries to swap
the following link from NORMAL to HEAD. It sees PAGE_MOVED which is
fatal for the ring buffer.
To prevent the race, mark the newly moved head links with HEAD_UPDATE
(HEAD | UPDATE) and only convert the final link to PAGE_HEAD when the
outer writer exists rb_handle_head_page(). This effectively keeps the
reader spinning.
Fixes: 77ae365eca89 ("ring-buffer: make lockless")
Reported-by: Breno Leitao <leitao@debian.org>
Closes: https://lore.kernel.org/all/20261105-ring-buffer-head-moved-v2-1-05da40cebccc@debian.org/
Cc: stable@vger.kernel.org
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
---
kernel/trace/ring_buffer.c | 44 +++++++++++++++++++++++++++++++++-----
1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 1fd28a72e6ee..73f31e6719f9 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -1306,6 +1306,7 @@ EXPORT_SYMBOL_GPL(ring_buffer_normalize_time_stamp);
#define RB_PAGE_NORMAL 0UL
#define RB_PAGE_HEAD 1UL
#define RB_PAGE_UPDATE 2UL
+#define RB_PAGE_HEAD_UPDATE (RB_PAGE_HEAD | RB_PAGE_UPDATE)
#define RB_FLAG_MASK 3UL
@@ -1455,6 +1456,15 @@ static int rb_head_page_set_head(struct ring_buffer_per_cpu *cpu_buffer,
old_flag, RB_PAGE_HEAD);
}
+static int rb_head_page_set_head_update(struct ring_buffer_per_cpu *cpu_buffer,
+ struct buffer_page *head,
+ struct buffer_page *prev,
+ int old_flag)
+{
+ return rb_head_page_set(cpu_buffer, head, prev,
+ old_flag, RB_PAGE_HEAD_UPDATE);
+}
+
static int rb_head_page_set_normal(struct ring_buffer_per_cpu *cpu_buffer,
struct buffer_page *head,
struct buffer_page *prev,
@@ -3699,6 +3709,11 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
type = rb_head_page_set_update(cpu_buffer, next_page, tail_page,
RB_PAGE_HEAD);
+ /* We have interrupted an update */
+ if (type == RB_PAGE_HEAD_UPDATE)
+ type = rb_head_page_set_update(cpu_buffer, next_page, tail_page,
+ RB_PAGE_HEAD_UPDATE);
+
/*
* type can be one of four:
* NORMAL - an interrupt already moved it for us
@@ -3711,6 +3726,7 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
*/
switch (type) {
+ case RB_PAGE_HEAD_UPDATE:
case RB_PAGE_HEAD:
/*
* We changed the head to UPDATE, thus
@@ -3773,8 +3789,8 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
new_head = next_page;
rb_inc_page(&new_head);
- ret = rb_head_page_set_head(cpu_buffer, new_head, next_page,
- RB_PAGE_NORMAL);
+ ret = rb_head_page_set_head_update(cpu_buffer, new_head, next_page,
+ RB_PAGE_NORMAL);
/*
* Valid returns are:
@@ -3785,7 +3801,7 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
* the page forward again.
*/
switch (ret) {
- case RB_PAGE_HEAD:
+ case RB_PAGE_HEAD_UPDATE:
case RB_PAGE_NORMAL:
/* OK */
break;
@@ -3816,7 +3832,7 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
buffer_tail_page != next_page)
rb_head_page_set_normal(cpu_buffer, new_head,
next_page,
- RB_PAGE_HEAD);
+ RB_PAGE_HEAD_UPDATE);
}
/*
@@ -3824,7 +3840,7 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
* changed the original pointer from HEAD to UPDATE),
* then it is up to us to reset it to NORMAL.
*/
- if (type == RB_PAGE_HEAD) {
+ if (type == RB_PAGE_HEAD || type == RB_PAGE_HEAD_UPDATE) {
ret = rb_head_page_set_normal(cpu_buffer, next_page,
tail_page,
RB_PAGE_UPDATE);
@@ -3833,6 +3849,24 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
return -1;
}
+ if (type == RB_PAGE_HEAD) {
+ struct buffer_page *prev_page;
+
+ do {
+ /* Interrupts might have moved the head further */
+ while (!rb_is_head_page(new_head, new_head->list.prev))
+ rb_inc_page(&new_head);
+
+ prev_page = new_head;
+ rb_dec_page(&prev_page);
+
+ /* Try to release the reader */
+ ret = rb_head_page_set_head(cpu_buffer, new_head, prev_page,
+ RB_PAGE_HEAD_UPDATE);
+
+ } while (ret != (RB_PAGE_HEAD_UPDATE));
+ }
+
return 0;
}
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
--
2.56.0.542.g3ef89b7020-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ring-buffer: Let the reader spin on head page update
2026-11-11 16:06 [PATCH] ring-buffer: Let the reader spin on head page update Vincent Donnefort
@ 2026-11-11 16:55 ` sashiko-bot
2026-11-12 13:01 ` Vincent Donnefort
0 siblings, 1 reply; 3+ messages in thread
From: sashiko-bot @ 2026-11-11 16:55 UTC (permalink / raw)
To: Vincent Donnefort, linux-trace-kernel
Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ring-buffer: stale list.prev dereference in
rb_handle_head_page() writer loop
--
commit 42f758b28dc946f5aa06647c36c046a615ee80cf
Author: Vincent Donnefort <vdonnefort@google.com>
ring-buffer: Let the reader spin on head page update
This commit introduces the RB_PAGE_HEAD_UPDATE flag to mark newly moved head
page links while a writer update is in progress, converting the final link
to RB_PAGE_HEAD only when the outer writer finishes rb_handle_head_page().
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 1fd28a72e6ee1..73f31e6719f98 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
[ ... ]
> @@ -3833,6 +3849,24 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
> return -1;
> }
>
> + if (type == RB_PAGE_HEAD) {
> + struct buffer_page *prev_page;
> +
> + do {
> + /* Interrupts might have moved the head further */
> + while (!rb_is_head_page(new_head, new_head->list.prev))
> + rb_inc_page(&new_head);
> +
> + prev_page = new_head;
> + rb_dec_page(&prev_page);
> +
> + /* Try to release the reader */
> + ret = rb_head_page_set_head(cpu_buffer, new_head, prev_page,
> + RB_PAGE_HEAD_UPDATE);
> +
> + } while (ret != (RB_PAGE_HEAD_UPDATE));
> + }
> +
> return 0;
> }
[Severity: Medium]
Can reading new_head->list.prev and rb_dec_page(&prev_page) here inspect a
stale predecessor page and spin in rb_handle_head_page()?
Lockless writers cannot rely on the head page's list.prev pointer because
__rb_get_reader_page() and rb_insert_pages() update the forward list.next
pointer via cmpxchg before updating the following page's list.prev pointer:
kernel/trace/ring_buffer.c:__rb_get_reader_page() {
...
ret = rb_head_page_replace(reader, cpu_buffer->reader_page);
if (!ret)
goto spin;
...
rb_list_head(reader->list.next)->prev = &cpu_buffer->reader_page->list;
...
}
If a writer enters rb_handle_head_page() in the window after a reader swaps
the head page (or after rb_insert_pages() splices in new pages) but before
list.prev is updated, new_head->list.prev still points to the old predecessor:
CPU0 (reader) CPU1 (writer)
__rb_get_reader_page()
rb_head_page_replace(reader,
cpu_buffer->reader_page)
rb_handle_head_page()
/* tail_page is reader_page->prev,
* next_page is reader_page,
* new_head is reader->next
*/
rb_head_page_set_head_update(...,
new_head, next_page, ...)
rb_head_page_set_normal(...,
next_page, tail_page, ...)
rb_is_head_page(new_head,
new_head->list.prev)
/* inspects reader->list.next
* (RB_PAGE_NORMAL) instead of
* reader_page->list.next
* (RB_PAGE_HEAD_UPDATE)
*/
rb_inc_page(&new_head) <-- misses head
rb_list_head(reader->list.next)->prev =
&cpu_buffer->reader_page->list;
Because every other link in the ring has RB_PAGE_NORMAL, the inner while loop
wraps around the ring buffer until CPU0 updates list.prev, or deadlocks if the
writer is an NMI that interrupted __rb_get_reader_page() or rb_insert_pages()
on the same CPU.
Since new_head starts as the page right after next_page, tracking prev_page
forward from next_page as new_head advances via rb_inc_page() would avoid
touching list.prev on the writer path.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261111160624.1294946-1-vdonnefort@google.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ring-buffer: Let the reader spin on head page update
2026-11-11 16:55 ` sashiko-bot
@ 2026-11-12 13:01 ` Vincent Donnefort
0 siblings, 0 replies; 3+ messages in thread
From: Vincent Donnefort @ 2026-11-12 13:01 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-trace-kernel
On Wed, Nov 11, 2026 at 04:55:54PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Medium] ring-buffer: stale list.prev dereference in
> rb_handle_head_page() writer loop
> --
>
> commit 42f758b28dc946f5aa06647c36c046a615ee80cf
> Author: Vincent Donnefort <vdonnefort@google.com>
>
> ring-buffer: Let the reader spin on head page update
>
> This commit introduces the RB_PAGE_HEAD_UPDATE flag to mark newly moved head
> page links while a writer update is in progress, converting the final link
> to RB_PAGE_HEAD only when the outer writer finishes rb_handle_head_page().
>
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 1fd28a72e6ee1..73f31e6719f98 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
>
> [ ... ]
>
> > @@ -3833,6 +3849,24 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
> > return -1;
> > }
> >
> > + if (type == RB_PAGE_HEAD) {
> > + struct buffer_page *prev_page;
> > +
> > + do {
> > + /* Interrupts might have moved the head further */
> > + while (!rb_is_head_page(new_head, new_head->list.prev))
> > + rb_inc_page(&new_head);
> > +
> > + prev_page = new_head;
> > + rb_dec_page(&prev_page);
> > +
> > + /* Try to release the reader */
> > + ret = rb_head_page_set_head(cpu_buffer, new_head, prev_page,
> > + RB_PAGE_HEAD_UPDATE);
> > +
> > + } while (ret != (RB_PAGE_HEAD_UPDATE));
> > + }
> > +
> > return 0;
> > }
>
> [Severity: Medium]
> Can reading new_head->list.prev and rb_dec_page(&prev_page) here inspect a
> stale predecessor page and spin in rb_handle_head_page()?
>
> Lockless writers cannot rely on the head page's list.prev pointer because
> __rb_get_reader_page() and rb_insert_pages() update the forward list.next
> pointer via cmpxchg before updating the following page's list.prev pointer:
>
> kernel/trace/ring_buffer.c:__rb_get_reader_page() {
> ...
> ret = rb_head_page_replace(reader, cpu_buffer->reader_page);
> if (!ret)
> goto spin;
> ...
> rb_list_head(reader->list.next)->prev = &cpu_buffer->reader_page->list;
> ...
> }
>
> If a writer enters rb_handle_head_page() in the window after a reader swaps
> the head page (or after rb_insert_pages() splices in new pages) but before
> list.prev is updated, new_head->list.prev still points to the old predecessor:
>
> CPU0 (reader) CPU1 (writer)
> __rb_get_reader_page()
> rb_head_page_replace(reader,
> cpu_buffer->reader_page)
> rb_handle_head_page()
> /* tail_page is reader_page->prev,
> * next_page is reader_page,
> * new_head is reader->next
> */
> rb_head_page_set_head_update(...,
> new_head, next_page, ...)
> rb_head_page_set_normal(...,
> next_page, tail_page, ...)
> rb_is_head_page(new_head,
> new_head->list.prev)
> /* inspects reader->list.next
> * (RB_PAGE_NORMAL) instead of
> * reader_page->list.next
> * (RB_PAGE_HEAD_UPDATE)
> */
> rb_inc_page(&new_head) <-- misses head
> rb_list_head(reader->list.next)->prev =
> &cpu_buffer->reader_page->list;
>
> Because every other link in the ring has RB_PAGE_NORMAL, the inner while loop
> wraps around the ring buffer until CPU0 updates list.prev, or deadlocks if the
> writer is an NMI that interrupted __rb_get_reader_page() or rb_insert_pages()
> on the same CPU.
>
> Since new_head starts as the page right after next_page, tracking prev_page
> forward from next_page as new_head advances via rb_inc_page() would avoid
> touching list.prev on the writer path.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261111160624.1294946-1-vdonnefort@google.com?part=1
Right, using new_head backpointer might be too risky. The prev_page suggestion
looks just better and doesn't invalidate this approach.
--
Vincent
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-11-12 13:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-11 16:06 [PATCH] ring-buffer: Let the reader spin on head page update Vincent Donnefort
2026-11-11 16:55 ` sashiko-bot
2026-11-12 13:01 ` Vincent Donnefort
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox