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

[Vincent Donnefort]

__rb_get_reader_page_from_remote() bails out when rb_num_of_entries() is
zero before verifying if there is more data committed on the current
reader_page (rb_page_size()), while simple_ring_buffer updates
page::commit before meta::entries. As a consequence, a race between the
writer and reader could lead to __rb_get_reader_page_from_remote()
returning NULL, while more data is available for reading on the ring
buffer.

  CPU0 (writer)                       CPU1 (reader)
  -------------                       -------------
  local_set(&page->commit, write);
                                      __rb_get_reader_page_from_remote()
                                        !rb_read_remote_meta_page()
                                          return NULL;
  simple_rb_meta_inc(meta->entries, 1);

This is an issue for rb_advance_reader() which always expects
rb_get_reader_page() to return a page.

Fix this issue by swapping the checks in
__rb_get_reader_page_from_remote(): first it verifies if there is more
to read in the current reader page, then it verifies if there's anything
at all in the ring buffer.

Also, if the reader observes the meta::entries update before
page::commit while the writer moves tail_page, it could swap reader_page
out and drop unread events. Order page::commit before meta::entries on
the writer and rb_read_remote_meta_page() before rb_page_size() on the
reader size.

Cc: stable@xxxxxxxxxxxxxxx
Fixes: 2e67fabd8b77 ("ring-buffer: Introduce ring-buffer remotes")
Fixes: 34e5b958bdad ("tracing: Introduce simple_ring_buffer")
Signed-off-by: Vincent Donnefort <vdonnefort@xxxxxxxxxx>
---
 kernel/trace/ring_buffer.c        | 19 +++++++++++++------
 kernel/trace/simple_ring_buffer.c |  6 ++++++
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 3c7aa982d43e..f70010e2ede7 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -5615,14 +5615,12 @@ unsigned long ring_buffer_overruns(struct trace_buffer *buffer)
 }
 EXPORT_SYMBOL_GPL(ring_buffer_overruns);
 
-static bool rb_read_remote_meta_page(struct ring_buffer_per_cpu *cpu_buffer)
+static void rb_read_remote_meta_page(struct ring_buffer_per_cpu *cpu_buffer)
 {
 	local_set(&cpu_buffer->entries, READ_ONCE(cpu_buffer->meta_page->entries));
 	local_set(&cpu_buffer->overrun, READ_ONCE(cpu_buffer->meta_page->overrun));
 	local_set(&cpu_buffer->pages_touched, READ_ONCE(cpu_buffer->meta_page->pages_touched));
 	local_set(&cpu_buffer->pages_lost, READ_ONCE(cpu_buffer->meta_page->pages_lost));
-
-	return rb_num_of_entries(cpu_buffer);
 }
 
 static void rb_update_remote_head(struct ring_buffer_per_cpu *cpu_buffer)
@@ -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;
+
 	prev_reader = cpu_buffer->subbuf_ids[cpu_buffer->meta_page->reader.id];
 
 	if (cpu_buffer->remote->swap_reader_page(cpu_buffer->cpu,
@@ -6909,7 +6915,8 @@ int ring_buffer_poll_remote(struct trace_buffer *buffer, int cpu)
 		cpu_buffer = buffer->buffers[cpu];
 
 		guard(raw_spinlock)(&cpu_buffer->reader_lock);
-		if (rb_read_remote_meta_page(cpu_buffer))
+		rb_read_remote_meta_page(cpu_buffer);
+		if (rb_num_of_entries(cpu_buffer))
 			rb_wakeups(buffer, cpu_buffer);
 
 		return 0;
diff --git a/kernel/trace/simple_ring_buffer.c b/kernel/trace/simple_ring_buffer.c
index d5c84af01925..5f14ba152fba 100644
--- a/kernel/trace/simple_ring_buffer.c
+++ b/kernel/trace/simple_ring_buffer.c
@@ -309,6 +309,12 @@ void simple_ring_buffer_commit(struct simple_rb_per_cpu *cpu_buffer)
 {
 	local_set(&cpu_buffer->tail_page->page->commit,
 		  cpu_buffer->tail_page->write);
+
+	/*
+	 * Order page::commit before meta::entries. Paired with
+	 * __rb_get_reader_page_from_remote().
+	 */
+	smp_wmb();
 	simple_rb_meta_inc(cpu_buffer->meta->entries, 1);
 
 	/*
-- 
2.56.0.611.g1030f0b940-goog