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