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

[Vincent Donnefort <vdonnefort@google.com>]

On Mon, Nov 02, 2026 at 06:30:11AM -0800, Breno Leitao wrote:
> I am occasionally seeing the following  KASAN warning when running

a KASAN warning?

> some debug kernel at 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)
> 
> I've understood that rb_handle_head_page() moves the head page forward

Drop "I've understood". 

> 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
>     </interrupt>
>                                         rb_get_reader_page()
>                                           swaps new_head out for the
>                                           reader page
>     cmpxchg(new_head|NORMAL ->
>             new_head|HEAD) -> MOVED
>     RB_WARN_ON()
> 
> RB_WARN_ON() disables the whole ring buffer, and the function returns
> without clearing its UPDATE flag, so tracing stays off until the buffer
> is reset.
> 
> Do not treat RB_PAGE_MOVED as fatal, retry after clearing UPDATE. The
> head already advanced past both pages, so the caller can simply try
> again without losing tracing.
> 
> Fixes: 77ae365eca89 ("ring-buffer: make lockless")
> Cc: stable@vger.kernel.org
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> 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.
> - Without the patch the warning fired after about 12 minutes. With a
>   debug-only 50us udelay() between computing new_head and the cmpxchg
>   it fired within a second, and a debug print showed ret ==
>   RB_PAGE_MOVED, type == RB_PAGE_HEAD and new_head being the reader's
>   page.
> - With the patch: no warning in a 5 minute run with the udelay() (the
>   RB_PAGE_MOVED case ran about 1000 times) or in a 25 minute run
>   without it (it ran once), and the buffer kept recording in both.
> - checkpatch --strict is clean, and kernel/trace/ring_buffer.o builds
>   without warnings at W=1 on v7.4-rc1.
> 
> Not tested: the fix has not run on the arm64 Grace host where the
> original WARN was seen.
> 
> The original WARN came from a next-20261030 KASAN kernel on an NVIDIA
> Grace host under stress-ng, after an interrupted ftracetest run had left
> that tracing setup in place.
> 
> The analysis, reproducer and patch were developed with an AI coding
> assistant (Claude Code). It triaged the WARN from the netconsole logs,
> wrote the virtme-ng reproducer and the debug instrumentation, and
> drafted the fix and changelog.
> ---
>  kernel/trace/ring_buffer.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 1fd28a72e6ee13..7a60fa175e2cc6 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -3789,6 +3789,15 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
>  	case RB_PAGE_NORMAL:
>  		/* OK */
>  		break;
> +	case RB_PAGE_MOVED:
> +		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;
> +		}

We should avoid duplicating this.

> +		return 1;

Besides, I am not sure about returning 1. If the nested writer interrupts
between rb_head_page_set_head() and rb_head_page_set_normal(), it can also get
RB_PAGE_MOVED and skipping rb_tail_page_update() means the retry will observe
the old tail.

All in all, I wonder if it shouldn't simply be: 

  case RB_PAGE_MOVED:
     break;

Additionally, the comment with "Valid returns" must be amended.

>  	default:
>  		RB_WARN_ON(cpu_buffer, 1);
>  		return -1;
> 
> ---
> base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
> change-id: 20261102-ring-buffer-head-moved-f9caf5da9444
> 
> Best regards,
> --  
> Breno Leitao <leitao@debian.org>
> 

-- 
Vincent