Re: [PATCH v2] ring-buffer: Handle a reader taking the new head in rb_handle_head_page()

[Vincent Donnefort <vdonnefort@google.com>]

On Thu, Nov 05, 2026 at 03:57:29PM -0500, Steven Rostedt wrote:
> 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 -->

How we can end-up in this situation? The cmpxch will only swap HEAD for UPDATE. 

> 
> 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
> 

-- 
Vincent