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

[sashiko-bot@kernel.org]

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