* Re: [PATCH v2] ring-buffer: Handle a reader taking the new head in rb_handle_head_page()
2026-11-05 11:12 [PATCH v2] ring-buffer: Handle a reader taking the new head in rb_handle_head_page() Breno Leitao
@ 2026-11-05 11:31 ` sashiko-bot
2026-11-05 20:57 ` Steven Rostedt
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-11-05 11:31 UTC (permalink / raw)
To: Breno Leitao; +Cc: linux-trace-kernel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ring-buffer: duplicate RB_PAGE_HEAD race in rb_handle_head_page()
--
commit 0a0249d6b3f6922c5b67d590d4639ec1b62531b2
Author: Breno Leitao <leitao@debian.org>
ring-buffer: Handle a reader taking the new head in rb_handle_head_page()
This commit updates rb_handle_head_page() to treat RB_PAGE_MOVED returned
by rb_head_page_set_head() the same as RB_PAGE_HEAD and RB_PAGE_NORMAL
when a concurrent reader swaps out new_head after a nested writer already
marked it as the head page.
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 1fd28a72e6ee1..44782b7166472 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -3783,10 +3783,13 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
> * 1) We really set it.
> * 2) A bunch of interrupts came in and moved
> * the page forward again.
> + * MOVED - another writer set it, then a reader on
> + * another CPU swapped it for its reader page.
> */
> switch (ret) {
> case RB_PAGE_HEAD:
> case RB_PAGE_NORMAL:
> + case RB_PAGE_MOVED:
> /* OK */
> break;
> default:
[Severity: High]
As reported by Sashiko on v1 [1], does handling RB_PAGE_MOVED here cover
the case where the reader swaps out the new head page before the writer
loads new_head via rb_inc_page(&new_head)?
In rb_handle_head_page(), new_head is loaded right before calling
rb_head_page_set_head():
new_head = next_page;
rb_inc_page(&new_head);
ret = rb_head_page_set_head(cpu_buffer, new_head, next_page,
RB_PAGE_NORMAL);
rb_head_page_set_head() only returns RB_PAGE_MOVED if the reader swaps
new_head out in the narrow window between rb_inc_page(&new_head) and the
cmpxchg in rb_head_page_set_head().
If the reader swaps the page out in __rb_get_reader_page() before
rb_inc_page(&new_head) executes (either while the outer writer is
interrupted after rb_head_page_set_update(), or before a nested interrupt
writer reaches rb_inc_page(&new_head)), __rb_get_reader_page() sets
RB_PAGE_HEAD on reader_page->list.next and replaces next_page->list.next
with &reader_page->list | RB_PAGE_NORMAL:
rb_set_list_to_head(&cpu_buffer->reader_page->list);
...
ret = rb_head_page_replace(reader, cpu_buffer->reader_page);
When the writer then executes rb_inc_page(&new_head), it loads the newly
spliced reader_page as new_head. rb_head_page_set_head() then matches
&reader_page->list | RB_PAGE_NORMAL, sets next_page->list.next to
RB_PAGE_HEAD, and returns RB_PAGE_NORMAL instead of RB_PAGE_MOVED.
In the recovery check that follows:
if (ret == RB_PAGE_NORMAL) {
struct buffer_page *buffer_tail_page;
buffer_tail_page = READ_ONCE(cpu_buffer->tail_page);
/*
* If the tail had moved passed next, then we need
* to reset the pointer.
*/
if (buffer_tail_page != tail_page &&
buffer_tail_page != next_page)
rb_head_page_set_normal(cpu_buffer, new_head,
next_page,
RB_PAGE_HEAD);
}
cpu_buffer->tail_page is still either next_page (if the nested writer
moved the tail) or tail_page (if the nested writer interrupted after the
outer writer's second cmpxchg), so rb_head_page_set_normal() is not
called. Wouldn't that leave RB_PAGE_HEAD set on both next_page->list.next
and reader_page->list.next simultaneously?
[1] https://lore.kernel.org/r/20261102-ring-buffer-head-moved-v1-1-165973790be4@debian.org
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-ring-buffer-head-moved-v2-1-05da40cebccc@debian.org?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] ring-buffer: Handle a reader taking the new head in rb_handle_head_page()
2026-11-05 11:12 [PATCH v2] ring-buffer: Handle a reader taking the new head in rb_handle_head_page() Breno Leitao
2026-11-05 11:31 ` sashiko-bot
@ 2026-11-05 20:57 ` Steven Rostedt
1 sibling, 0 replies; 3+ messages in thread
From: Steven Rostedt @ 2026-11-05 20:57 UTC (permalink / raw)
To: Breno Leitao
Cc: Vincent Donnefort, Masami Hiramatsu, Mathieu Desnoyers,
linux-kernel, linux-trace-kernel, Steven Rostedt, kernel-team,
stable
On Thu, 05 Nov 2026 03:12:42 -0800
Breno Leitao <leitao@debian.org> wrote:
> The following warning occasionally fires on a KASAN-enabled debug
> kernel in Meta's fleet:
>
> WARNING: kernel/trace/ring_buffer.c:3793 at rb_move_tail+0xf78/0x1050
> Call trace:
> rb_move_tail+0xf78/0x1050 (P)
> __rb_reserve_next+0x648/0xac0
> ring_buffer_lock_reserve+0xd00/0xf00
> trace_buffer_lock_reserve+0x28/0x188
> trace_graph_return+0x220/0x3e8
> ftrace_return_to_handler+0x2bc/0x5a8
> return_to_handler+0x20/0x48
> __arm64_sys_sched_yield+0x18/0x30 (F)
>
> rb_handle_head_page() moves the head page forward in two steps: it marks
> the pointer to the old head page UPDATE, then marks the pointer to the
> page after it (new_head) HEAD. The second cmpxchg is assumed to return
> only HEAD or NORMAL, but it can also return RB_PAGE_MOVED:
>
> CPU 0 (writer) CPU 1 (reader)
> ------------------------------------ --------------------------------
> rb_handle_head_page()
> old head ptr: HEAD -> UPDATE
> new_head = page after old head
> <interrupt: nested write>
> sees UPDATE, marks new_head
> HEAD and moves the tail page
Wait? The interrupt should never change UPDATE to HEAD.
Something else is broken here. Because only the one that set HEAD to update
is allowed to set it back to NORMAL.
> </interrupt>
> rb_get_reader_page()
> swaps new_head out for the
> reader page
This should not happen. The entire point of
setting the pointer to UPDATE is to prevent
swaps from happening when any commit is in
progress.
The rb_handle_head_page() even states this:
/*
* If this was the outer most commit (the one that
* changed the original pointer from HEAD to UPDATE),
* then it is up to us to reset it to NORMAL.
*/
if (type == RB_PAGE_HEAD) {
ret = rb_head_page_set_normal(cpu_buffer, next_page,
tail_page,
RB_PAGE_UPDATE);
if (RB_WARN_ON(cpu_buffer,
ret != RB_PAGE_UPDATE))
return -1;
}
> cmpxchg(new_head|NORMAL ->
> new_head|HEAD) -> MOVED
> RB_WARN_ON()
>
> A nested writer that interrupts the outer one after the second cmpxchg
> still sees the old head pointer UPDATE and repeats the second step. If
> the reader swaps new_head out just before that cmpxchg, it returns
> RB_PAGE_MOVED too.
>
> RB_WARN_ON() permanently disables recording on the whole ring buffer,
> and the outer writer returns without clearing its UPDATE flag.
>
> In both cases another writer has already moved the head off the old
> head page, as when the cmpxchg returns HEAD. Treat RB_PAGE_MOVED the
> same way and fall through: the outer writer still clears its UPDATE
> flag, and the caller updates the tail page as usual.
>
> Fixes: 77ae365eca89 ("ring-buffer: make lockless")
> Cc: stable@vger.kernel.org
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> Changes in v2:
> - Fall through on RB_PAGE_MOVED instead of returning 1, so a nested
> writer still moves the tail page (Vincent)
> - Drop the duplicated UPDATE clear (Vincent)
> - Document RB_PAGE_MOVED in the "Valid returns" comment (Vincent)
> - Commit message improvements
> - Link to v1: https://patch.msgid.link/20261102-ring-buffer-head-moved-v1-1-165973790be4@debian.org
From your v1 comment:
> A nested writer and a consuming reader can race so that the second
> cmpxchg of a head page move in rb_handle_head_page() returns
> RB_PAGE_MOVED. That trips RB_WARN_ON(), which disables the ring buffer
> and leaves the old head pointer flagged UPDATE. The patch handles the
> return like the RB_PAGE_MOVED case of the first cmpxchg: clear the
> UPDATE flag if this writer set it, and retry.
>
> Testing, on arm64 KVM guests under virtme-ng (32 vCPUs, KASAN config,
> next-20261030; kernel/trace/ring_buffer.c is identical in v7.4-rc1):
> - Setup: function_graph tracer filtered to "*sched*", buffer_size_kb=7
> (two pages per CPU), overwrite on, a trace_pipe_raw reader on every
> CPU plus a trace_pipe reader, and stress-ng --yield/--timer/--futex.
OK, I think I see the issue here. I believe the reader is finding the wrong
head page. Making the buffer really small can make this scenario happen
more often.
The problem is where the reader thinks the "head_page" is. When the reader
swaps out the page, it first looks for the "head_page". The head page
always has flags set in the pointer to it, so before the swap, the reader
will search for the head page.
It starts with the cpu_buffer->head_page pointer which is where it found it
last. It then searches for the head page and when it finds it, it tries the
swap. The swap will only succeed if the pointer is HEAD.
The issue I think you hit was that we have two HEAD pages, when interrupts
come in. That is, we can get a state of:
--> subbuf --UPDATE--> subbuf --> subbuf --HEAD--> subbuf -->
With the expectation that the reader will never be between those two
pointers and it will always find the UPDATE before it sees the new HEAD. But
since only the reader moves the cpu_buffer->head_page around and the writer
does not, we can have a stale pointer of this:
--> subbuf --UPDATE--> subbuf --> subbuf --HEAD--> subbuf -->
^
|
cpu_buffer->head_page
And when the reader looks for the new head, it will find this:
--> subbuf --UPDATE--> subbuf --> subbuf --HEAD--> subbuf -->
^
|
cpu_buffer->head_page
And possibly swap out the head page before it is allowed to.
To prevent this, I believe having the writer move this pointer will keep it
from being stale and ending up between the two.
Can you try this?
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index c64e001c3fc0..f0bb2d23d818 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -3721,6 +3721,14 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
local_sub(rb_page_commit(next_page), &cpu_buffer->entries_bytes);
local_inc(&cpu_buffer->pages_lost);
+ /*
+ * To prevent a stale head_page from being between
+ * the UPDATE and new HEAD pages, set it here to have
+ * the reader check always find the update before
+ * finding the HEAD page.
+ */
+ cpu_buffer->head_page = next_page;
+
if (cpu_buffer->ring_meta)
rb_update_meta_head(cpu_buffer, next_page);
/*
We may need to add memory barriers here, but for now, I think this could
fix it. As it should prevent the search from ever starting in between the
two heads.
-- Steve
^ permalink raw reply related [flat|nested] 3+ messages in thread