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

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

[Breno Leitao]

I am occasionally seeing the following  KASAN warning when running
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
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;
+		}
+		return 1;
 	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>

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

[sashiko-bot]

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 06572f261acc9f999487d8e50d79b22b59309351
Author: Breno Leitao <leitao@debian.org>

ring-buffer: Handle a reader taking the new head in rb_handle_head_page()

This patch handles RB_PAGE_MOVED returned from rb_head_page_set_head() in
rb_handle_head_page() when a nested interrupt writer advances the head page
and a concurrent reader swaps out new_head before the outer writer completes
its second cmpxchg.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 1fd28a72e6ee1..7a60fa175e2cc 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;
> +		}
> +		return 1;

[Severity: High]
Can the same race with a nested writer and concurrent reader leave two pages
in the ring buffer with RB_PAGE_HEAD set simultaneously if the reader swap
happens before new_head is loaded, or when an interrupting writer sees
type == RB_PAGE_UPDATE?

In kernel/trace/ring_buffer.c:rb_handle_head_page(), the outer writer sets
tail_page->list.next to RB_PAGE_UPDATE before computing new_head:

	type = rb_head_page_set_update(cpu_buffer, next_page, tail_page,
				       RB_PAGE_HEAD);

	switch (type) {
	case RB_PAGE_HEAD:
		local_add(entries, &cpu_buffer->overrun);
		local_sub(rb_page_commit(next_page), &cpu_buffer->entries_bytes);
		local_inc(&cpu_buffer->pages_lost);

		if (cpu_buffer->ring_meta)
			rb_update_meta_head(cpu_buffer, next_page);
		break;
	...
	}

	new_head = next_page;
	rb_inc_page(&new_head);

	ret = rb_head_page_set_head(cpu_buffer, new_head, next_page,
				    RB_PAGE_NORMAL);

Once rb_head_page_set_update() sets tail_page->list.next to RB_PAGE_UPDATE,
if the nested interrupt writer and concurrent reader swap happen before
rb_inc_page(&new_head) reads next_page->list.next:

1. The nested interrupt writer sees RB_PAGE_UPDATE, sets next_page->list.next
   (pointing to the old new_head) to RB_PAGE_HEAD, and advances
   cpu_buffer->tail_page from tail_page to next_page in rb_move_tail() ->
   rb_tail_page_update().

2. The concurrent reader in rb_get_reader_page() -> __rb_get_reader_page()
   swaps the old new_head out for its empty reader_page, setting
   reader_page->list.next to the following page with RB_PAGE_HEAD and setting
   next_page->list.next to &reader_page->list with RB_PAGE_NORMAL.

3. The outer writer resumes and executes rb_inc_page(&new_head), which loads
   the newly spliced reader_page from next_page->list.next.

4. rb_head_page_set_head() then matches &reader_page->list | RB_PAGE_NORMAL,
   sets next_page->list.next to &reader_page->list | RB_PAGE_HEAD, and
   returns RB_PAGE_NORMAL instead of RB_PAGE_MOVED.

5. The subsequent tail check does not clear the extra RB_PAGE_HEAD flag
   because the single nested interrupt only moved cpu_buffer->tail_page to
   next_page:

	if (ret == RB_PAGE_NORMAL) {
		struct buffer_page *buffer_tail_page;

		buffer_tail_page = READ_ONCE(cpu_buffer->tail_page);
		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);
	}

A similar sequence also appears possible when the outer writer sets
next_page->list.next to RB_PAGE_HEAD, the reader swaps that page out for
reader_page, and an interrupting writer arrives before the outer writer
updates cpu_buffer->tail_page. The interrupting writer enters
rb_handle_head_page() with type == RB_PAGE_UPDATE (or hits the new
RB_PAGE_MOVED path and returns 1 without advancing cpu_buffer->tail_page,
immediately retrying on the same tail_page), loads new_head as the spliced
reader_page, and marks next_page->list.next as RB_PAGE_HEAD while
buffer_tail_page is still tail_page.

In both cases, wouldn't both next_page->list.next and reader_page->list.next
be left marked with RB_PAGE_HEAD, corrupting the ring buffer list state and
triggering RB_WARN_ON() in rb_set_head_page() or __rb_get_reader_page()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-ring-buffer-head-moved-v1-1-165973790be4@debian.org?part=1