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

[sashiko-bot@kernel.org]

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