* 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 20:57 ` Steven Rostedt
0 siblings, 0 replies; 2+ 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 [flat|nested] 2+ messages in thread