Re: [PATCH v1 2/4] ring-buffer: Check remote reader page commit before meta entries

[Vincent Donnefort <vdonnefort@google.com>]

On Fri, Nov 13, 2026 at 04:03:36PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Medium] ring-buffer: rb_num_of_entries() underflow when commit
>   precedes meta entries
> --
> 
> commit 1d2d0e6ae01d518c30dc9986893db113c38a4caf
> Author: Vincent Donnefort <vdonnefort@google.com>
> 
> ring-buffer: Check remote reader page commit before meta entries
> 
> This commit reorders the checks in __rb_get_reader_page_from_remote() to
> inspect the current reader page commit offset before checking if
> rb_num_of_entries() is zero, and adds memory barriers pairing page::commit
> and meta::entries between simple_ring_buffer_commit() and the remote reader.
> 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 3c7aa982d43e9..f70010e2ede77 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> 
> [ ... ]
> 
> > @@ -5825,8 +5823,13 @@ __rb_get_reader_page_from_remote(struct ring_buffer_per_cpu *cpu_buffer)
> >  {
> >  	struct buffer_page *new_reader, *prev_reader, *prev_head, *new_head, *last;
> >  
> > -	if (!rb_read_remote_meta_page(cpu_buffer))
> > -		return NULL;
> > +	rb_read_remote_meta_page(cpu_buffer);
> > +
> > +	/*
> > +	 * Order meta::entries read before page::commit, most likely paired with
> > +	 * simple_ring_buffer_commit().
> > +	 */
> > +	smp_rmb();
> >  
> >  	/* More to read on the reader page */
> >  	if (cpu_buffer->reader_page->read < rb_page_size(cpu_buffer->reader_page)) {
> > @@ -5835,6 +5838,9 @@ __rb_get_reader_page_from_remote(struct ring_buffer_per_cpu *cpu_buffer)
> >  		return cpu_buffer->reader_page;
> >  	}
> >  
> > +	if (!rb_num_of_entries(cpu_buffer))
> > +		return NULL;
> 
> [Severity: Medium]
> Can rb_num_of_entries() underflow to ULONG_MAX when an event is consumed in
> the window between the page->commit update and the meta->entries increment?
> 
> CPU0 (writer)                       CPU1 (reader)
> -------------                       -------------
> simple_ring_buffer_commit()
>   local_set(&page->commit, write);
>                                     ring_buffer_consume()
>                                       ...
>                                       __rb_get_reader_page_from_remote()
>                                         rb_read_remote_meta_page()
>                                           /* caches entries = N */
>                                         reader_page->read < rb_page_size()
>                                           return reader_page;
>                                       rb_advance_reader()
>                                         cpu_buffer->read++; /* N + 1 */
>   simple_rb_meta_inc(meta->entries, 1);
> 
> At this point cpu_buffer->read is N + 1 while cpu_buffer->entries is still N,
> so rb_num_of_entries() evaluates N - (N + 1) = ULONG_MAX:
> 
> kernel/trace/ring_buffer.c:rb_num_of_entries() {
> 	return local_read(&cpu_buffer->entries) -
> 		(local_read(&cpu_buffer->overrun) + cpu_buffer->read);
> }
> 
> Until rb_read_remote_meta_page() runs again, ring_buffer_entries() returns
> ULONG_MAX and ring_buffer_empty() returns false on an empty buffer (which
> also prevents trace_remote_try_unload() from unloading an idle buffer).  If
> __rb_get_reader_page_from_remote() is called again before CPU0 increments
> meta->entries, !rb_num_of_entries() evaluates to false and it proceeds to
> swap_reader_page() on an empty buffer.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261113152729.837429-1-vdonnefort@google.com?part=2

This underflow can happen if ring_buffer_consume() is called directly. But
trace_remote always tests ring_buffer_empty_cpu() before calling
ring_buffer_peek() or ring_buffer_iter_peek().

-- 
Vincent