Re: [PATCH] ring-buffer: Let the reader spin on head page update

[Vincent Donnefort <vdonnefort@google.com>]

On Wed, Nov 11, 2026 at 04:55:54PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Medium] ring-buffer: stale list.prev dereference in
>   rb_handle_head_page() writer loop
> --
> 
> commit 42f758b28dc946f5aa06647c36c046a615ee80cf
> Author: Vincent Donnefort <vdonnefort@google.com>
> 
> ring-buffer: Let the reader spin on head page update
> 
> This commit introduces the RB_PAGE_HEAD_UPDATE flag to mark newly moved head
> page links while a writer update is in progress, converting the final link
> to RB_PAGE_HEAD only when the outer writer finishes rb_handle_head_page().
> 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 1fd28a72e6ee1..73f31e6719f98 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> 
> [ ... ]
> 
> > @@ -3833,6 +3849,24 @@ rb_handle_head_page(struct ring_buffer_per_cpu *cpu_buffer,
> >  			return -1;
> >  	}
> >  
> > +	if (type == RB_PAGE_HEAD) {
> > +		struct buffer_page *prev_page;
> > +
> > +		do {
> > +			/* Interrupts might have moved the head further  */
> > +			while (!rb_is_head_page(new_head, new_head->list.prev))
> > +				rb_inc_page(&new_head);
> > +
> > +			prev_page = new_head;
> > +			rb_dec_page(&prev_page);
> > +
> > +			/* Try to release the reader */
> > +			ret = rb_head_page_set_head(cpu_buffer, new_head, prev_page,
> > +						    RB_PAGE_HEAD_UPDATE);
> > +
> > +		} while (ret != (RB_PAGE_HEAD_UPDATE));
> > +	}
> > +
> >  	return 0;
> >  }
> 
> [Severity: Medium]
> Can reading new_head->list.prev and rb_dec_page(&prev_page) here inspect a
> stale predecessor page and spin in rb_handle_head_page()?
> 
> Lockless writers cannot rely on the head page's list.prev pointer because
> __rb_get_reader_page() and rb_insert_pages() update the forward list.next
> pointer via cmpxchg before updating the following page's list.prev pointer:
> 
> 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;
> 	...
> }
> 
> If a writer enters rb_handle_head_page() in the window after a reader swaps
> the head page (or after rb_insert_pages() splices in new pages) but before
> list.prev is updated, new_head->list.prev still points to the old predecessor:
> 
> CPU0 (reader)                     CPU1 (writer)
> __rb_get_reader_page()
>   rb_head_page_replace(reader,
>     cpu_buffer->reader_page)
>                                   rb_handle_head_page()
>                                     /* tail_page is reader_page->prev,
>                                      * next_page is reader_page,
>                                      * new_head is reader->next
>                                      */
>                                     rb_head_page_set_head_update(...,
>                                       new_head, next_page, ...)
>                                     rb_head_page_set_normal(...,
>                                       next_page, tail_page, ...)
>                                     rb_is_head_page(new_head,
>                                       new_head->list.prev)
>                                       /* inspects reader->list.next
>                                        * (RB_PAGE_NORMAL) instead of
>                                        * reader_page->list.next
>                                        * (RB_PAGE_HEAD_UPDATE)
>                                        */
>                                     rb_inc_page(&new_head)  <-- misses head
>   rb_list_head(reader->list.next)->prev =
>     &cpu_buffer->reader_page->list;
> 
> Because every other link in the ring has RB_PAGE_NORMAL, the inner while loop
> wraps around the ring buffer until CPU0 updates list.prev, or deadlocks if the
> writer is an NMI that interrupted __rb_get_reader_page() or rb_insert_pages()
> on the same CPU.
> 
> Since new_head starts as the page right after next_page, tracking prev_page
> forward from next_page as new_head advances via rb_inc_page() would avoid
> touching list.prev on the writer path.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261111160624.1294946-1-vdonnefort@google.com?part=1

Right, using new_head backpointer might be too risky. The prev_page suggestion
looks just better and doesn't invalidate this approach.

-- 
Vincent