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

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

[Breno Leitao]

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
    </interrupt>
                                        rb_get_reader_page()
                                          swaps new_head out for the
                                          reader page
    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
---
 kernel/trace/ring_buffer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 1fd28a72e6ee13..44782b71664722 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:

---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261102-ring-buffer-head-moved-f9caf5da9444

Best regards,
--  
Breno Leitao <leitao@debian.org>

Re: [PATCH v2] 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 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