Re: [PATCH] ring-buffer: Do not allow reader to see unfinished moved head

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ring-buffer: nested irq in rb_handle_head_page() moves
  head_page past UPDATE
- [Medium] ring-buffer: race on head_page between rb_handle_head_page()
  and reader
--

commit f326a3a8a8353c768b90c13ee6f1ad10ec362a2f
Author: Steven Rostedt <rostedt@goodmis.org>

ring-buffer: Do not allow reader to see unfinished moved head

This commit updates cpu_buffer->head_page in rb_handle_head_page() with
smp_store_release() and reads it in rb_set_head_page() with
smp_load_acquire() so a reader does not start scanning for the head page
between a sub-buffer being updated and the newly moved head sub-buffer.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index e760d2fe386fb..cf9e0dc10c88f 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -1494,7 +1494,13 @@ rb_set_head_page(struct ring_buffer_per_cpu *cpu_buffer)
>  	if (RB_WARN_ON(cpu_buffer, rb_list_head(list->prev->next) != list))
>  		return NULL;
>  
> -	page = head = cpu_buffer->head_page;
> +	/*
> +	 * Make sure that the start of the search of the head_page does not
> +	 * start between the writer adding the UPDATE and the new HEAD that it
> +	 * moves forward.
> +	 */
> +	/* Sync with the writer moving the HEAD in rb_handle_head_page() */
> +	page = head = smp_load_acquire(&cpu_buffer->head_page);

[Severity: Medium]
Can the reader's non-atomic update of cpu_buffer->head_page in
__rb_get_reader_page() race with the writer's new store in
rb_handle_head_page() and advance cpu_buffer->head_page past the sub-buffer
marked for update?

Before this patch, only the reader modified cpu_buffer->head_page.  In
__rb_get_reader_page(), after rb_head_page_replace() splices reader_page into
the ring buffer, the reader advances cpu_buffer->head_page assuming it still
points to the replaced reader sub-buffer:

kernel/trace/ring_buffer.c:__rb_get_reader_page() {
    ...
	ret = rb_head_page_replace(reader, cpu_buffer->reader_page);
	if (!ret)
		goto spin;
    ...
	rb_list_head(reader->list.next)->prev = &cpu_buffer->reader_page->list;
	rb_inc_page(&cpu_buffer->head_page);
    ...
	goto again;
}

If the writer on the traced CPU moves onto the newly spliced reader_page (R),
fills it, and enters rb_handle_head_page() to overwrite the next sub-buffer
(P2) before the reader runs rb_inc_page(), both sides update
cpu_buffer->head_page without synchronization:

CPU0 (reader)                     CPU1 (writer)
__rb_get_reader_page()
  rb_set_head_page()
    head_page = P1
  rb_head_page_replace(P1, R)
  P2->list.prev = &R->list
                                  rb_handle_head_page(..., R, P2)
                                    rb_head_page_set_update(..., P2, R, ...)
                                    smp_store_release(&head_page, P2)
                                    rb_head_page_set_head(..., P3, P2, ...)
  rb_inc_page(&head_page)
    head_page = P3
  goto again
  rb_set_head_page()
    page = head_page (P3)  <-- skips P2 update, swaps P3

When __rb_get_reader_page() loops back to "again", rb_set_head_page() starts
its scan at P3 instead of P2, misses the in-progress update on P2, and swaps
out P3 while the writer is still moving the head page, corrupting the ring
buffer.

> @@ -3719,6 +3725,15 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
>  		local_sub(rb_page_commit(next_page), &cpu_buffer->entries_bytes);
>  		local_inc(&cpu_buffer->pages_lost);
>  
> +		/*
> +		 * To prevent a stale head_page from being between
> +		 * the UPDATE and new HEAD pages, set it here to have
> +		 * the reader check always find the update before
> +		 * finding the HEAD page.
> +		 */
> +		/* Sync with the reader setting the head page */
> +		smp_store_release(&cpu_buffer->head_page, next_page);

[Severity: Medium]
Can a nested interrupt that moves the head page across multiple sub-buffers
overwrite cpu_buffer->head_page and still place it past the outer writer's
sub-buffer under update?

When an outer writer in rb_handle_head_page() changes P0->list.next (P1) to
the update state, it gets type == RB_PAGE_HEAD and sets cpu_buffer->head_page
to P1.  If an interrupt then preempts the writer before it restores
P0->list.next to normal and writes enough events to wrap across two
sub-buffers (P1 and P2):

1. On the interrupt's first call to rb_handle_head_page(tail=P0, next=P1),
   rb_head_page_set_update() sees the outer update on P0->list.next and
   returns RB_PAGE_UPDATE.  The interrupt sets P1->list.next (P2) as the new
   head page and advances tail_page to P1, leaving P0->list.next in the
   update state.

2. When the interrupt fills P1 and calls rb_handle_head_page(tail=P1,
   next=P2), P1->list.next has the head flag set, so
   rb_head_page_set_update() succeeds and returns RB_PAGE_HEAD to the
   interrupt:

rb_handle_head_page(..., P0, P1)
  rb_head_page_set_update(..., P1, P0, ...)
  smp_store_release(&head_page, P1)
  <hardirq>
    rb_handle_head_page(..., P0, P1)
      rb_head_page_set_update() -> RB_PAGE_UPDATE
      rb_head_page_set_head(..., P2, P1, ...)
    ...
    rb_handle_head_page(..., P1, P2)
      rb_head_page_set_update() -> RB_PAGE_HEAD
      smp_store_release(&head_page, P2)  <-- moves head_page past P1
      rb_head_page_set_head(..., P3, P2, ...)
      rb_head_page_set_normal(..., P2, P1, ...)

At that point P1 is still in the update state from the outer writer, P3 is
the new head page, and cpu_buffer->head_page has been advanced to P2 (between
P1 and P3).  Furthermore, when the outer writer resumes at
rb_head_page_set_head(..., P2, P1, ...), it transiently sets the head flag on
P2 before resetting it to normal.  A concurrent reader on another CPU calling
rb_set_head_page() starts its scan at P2, never sees P1's update flag, and
can swap out P2 (which is cpu_buffer->tail_page) or P3 while the outer writer
is still in progress.

Also, if the interrupt arrives between rb_head_page_set_update() and
smp_store_release(&cpu_buffer->head_page, next_page), the interrupt sees
RB_PAGE_UPDATE and sets the new head page before cpu_buffer->head_page has
been updated from its stale value.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109032241.55e57d26@fedora?part=1