[for-next][PATCH 0/2] ring-buffer: Fixes for v6.12

[for-next][PATCH 0/2] ring-buffer: Fixes for v6.12

[Steven Rostedt]

ring-buffer: Fixes for v6.12

- Fix ref counter of buffers assigned at boot up

  A tracing instance can be created from the kernel command line.
  If it maps to memory, it is considered permanent and should not
  be deleted, or bad things can happen. If it is not mapped to memory,
  then the user is fine to delete it via rmdir from the instances
  directory. But the ref counts assumed 0 was free to remove and
  greater than zero was not. But this was not the case. When an
  instance is created, it should have the reference of 1, and if
  it should not be removed, it must be greater than 1. The boot up
  code set normal instances with a ref count of 0, which could get
  removed if something accessed it and then released it. And memory
  mapped instances had a ref count of 1 which meant it could be deleted,
  and bad things happen. Keep normal instances ref count as 1, and
  set memory mapped instances ref count to 2.

- Protect sub buffer size (order) updates from other modifications

  When a ring buffer is changing the size of its sub-buffers, no other
  operations should be performed on the ring buffer. That includes
  reading it. But the locking only grabbed the buffer->mutex that
  keeps some operations from touching the ring buffer. It also must
  hold the cpu_buffer->reader_lock as well when updates happen as
  other paths use that to do some operations on the ring buffer.

  git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
ring-buffer/urgent

Head SHA1: 09661f75e75cb6c1d2d8326a70c311d46729235f


Petr Pavlu (1):
      ring-buffer: Fix reader locking when changing the sub buffer order

Steven Rostedt (1):
      ring-buffer: Fix refcount setting of boot mapped buffers

----
 kernel/trace/ring_buffer.c | 44 ++++++++++++++++++++++++++------------------
 kernel/trace/trace.c       |  6 +++---
 2 files changed, 29 insertions(+), 21 deletions(-)

[for-next][PATCH 1/2] ring-buffer: Fix refcount setting of boot mapped buffers

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

A ring buffer which has its buffered mapped at boot up to fixed memory
should not be freed. Other buffers can be. The ref counting setup was
wrong for both. It made the not mapped buffers ref count have zero, and the
boot mapped buffer a ref count of 1. But an normally allocated buffer
should be 1, where it can be removed.

Keep the ref count of a normal boot buffer with its setup ref count (do
not decrement it), and increment the fixed memory boot mapped buffer's ref
count.

Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Link: https://lore.kernel.org/20241011165224.33dd2624@gandalf.local.home
Fixes: e645535a954ad ("tracing: Add option to use memmapped memory for trace boot instance")
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 1c69ca1f1088..a8f52b6527ca 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -10621,10 +10621,10 @@ __init static void enable_instances(void)
 		 * cannot be deleted by user space, so keep the reference
 		 * to it.
 		 */
-		if (start)
+		if (start) {
 			tr->flags |= TRACE_ARRAY_FL_BOOT;
-		else
-			trace_array_put(tr);
+			tr->ref++;
+		}
 
 		while ((tok = strsep(&curr_str, ","))) {
 			early_enable_events(tr, tok, true);
-- 
2.45.2

[for-next][PATCH 2/2] ring-buffer: Fix reader locking when changing the sub buffer order

[Steven Rostedt]

From: Petr Pavlu <petr.pavlu@suse.com>

The function ring_buffer_subbuf_order_set() updates each
ring_buffer_per_cpu and installs new sub buffers that match the requested
page order. This operation may be invoked concurrently with readers that
rely on some of the modified data, such as the head bit (RB_PAGE_HEAD), or
the ring_buffer_per_cpu.pages and reader_page pointers. However, no
exclusive access is acquired by ring_buffer_subbuf_order_set(). Modifying
the mentioned data while a reader also operates on them can then result in
incorrect memory access and various crashes.

Fix the problem by taking the reader_lock when updating a specific
ring_buffer_per_cpu in ring_buffer_subbuf_order_set().

Link: https://lore.kernel.org/linux-trace-kernel/20240715145141.5528-1-petr.pavlu@suse.com/
Link: https://lore.kernel.org/linux-trace-kernel/20241010195849.2f77cc3f@gandalf.local.home/
Link: https://lore.kernel.org/linux-trace-kernel/20241011112850.17212b25@gandalf.local.home/

Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Link: https://lore.kernel.org/20241015112440.26987-1-petr.pavlu@suse.com
Fixes: 8e7b58c27b3c ("ring-buffer: Just update the subbuffers when changing their allocation order")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ring_buffer.c | 44 ++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index fb04445f92c3..3ea4f7bb1837 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -6728,39 +6728,38 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
 	}
 
 	for_each_buffer_cpu(buffer, cpu) {
+		struct buffer_data_page *old_free_data_page;
+		struct list_head old_pages;
+		unsigned long flags;
 
 		if (!cpumask_test_cpu(cpu, buffer->cpumask))
 			continue;
 
 		cpu_buffer = buffer->buffers[cpu];
 
+		raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
+
 		/* Clear the head bit to make the link list normal to read */
 		rb_head_page_deactivate(cpu_buffer);
 
-		/* Now walk the list and free all the old sub buffers */
-		list_for_each_entry_safe(bpage, tmp, cpu_buffer->pages, list) {
-			list_del_init(&bpage->list);
-			free_buffer_page(bpage);
-		}
-		/* The above loop stopped an the last page needing to be freed */
-		bpage = list_entry(cpu_buffer->pages, struct buffer_page, list);
-		free_buffer_page(bpage);
-
-		/* Free the current reader page */
-		free_buffer_page(cpu_buffer->reader_page);
+		/*
+		 * Collect buffers from the cpu_buffer pages list and the
+		 * reader_page on old_pages, so they can be freed later when not
+		 * under a spinlock. The pages list is a linked list with no
+		 * head, adding old_pages turns it into a regular list with
+		 * old_pages being the head.
+		 */
+		list_add(&old_pages, cpu_buffer->pages);
+		list_add(&cpu_buffer->reader_page->list, &old_pages);
 
 		/* One page was allocated for the reader page */
 		cpu_buffer->reader_page = list_entry(cpu_buffer->new_pages.next,
 						     struct buffer_page, list);
 		list_del_init(&cpu_buffer->reader_page->list);
 
-		/* The cpu_buffer pages are a link list with no head */
+		/* Install the new pages, remove the head from the list */
 		cpu_buffer->pages = cpu_buffer->new_pages.next;
-		cpu_buffer->new_pages.next->prev = cpu_buffer->new_pages.prev;
-		cpu_buffer->new_pages.prev->next = cpu_buffer->new_pages.next;
-
-		/* Clear the new_pages list */
-		INIT_LIST_HEAD(&cpu_buffer->new_pages);
+		list_del_init(&cpu_buffer->new_pages);
 
 		cpu_buffer->head_page
 			= list_entry(cpu_buffer->pages, struct buffer_page, list);
@@ -6769,11 +6768,20 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
 		cpu_buffer->nr_pages = cpu_buffer->nr_pages_to_update;
 		cpu_buffer->nr_pages_to_update = 0;
 
-		free_pages((unsigned long)cpu_buffer->free_page, old_order);
+		old_free_data_page = cpu_buffer->free_page;
 		cpu_buffer->free_page = NULL;
 
 		rb_head_page_activate(cpu_buffer);
 
+		raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
+
+		/* Free old sub buffers */
+		list_for_each_entry_safe(bpage, tmp, &old_pages, list) {
+			list_del_init(&bpage->list);
+			free_buffer_page(bpage);
+		}
+		free_pages((unsigned long)old_free_data_page, old_order);
+
 		rb_check_pages(cpu_buffer);
 	}
 
-- 
2.45.2

Re: [for-next][PATCH 0/2] ring-buffer: Fixes for v6.12

[Steven Rostedt]

On Tue, 15 Oct 2024 11:31:05 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> ring-buffer: Fixes for v6.12
> 
> - Fix ref counter of buffers assigned at boot up
> 
>   A tracing instance can be created from the kernel command line.
>   If it maps to memory, it is considered permanent and should not
>   be deleted, or bad things can happen. If it is not mapped to memory,
>   then the user is fine to delete it via rmdir from the instances
>   directory. But the ref counts assumed 0 was free to remove and
>   greater than zero was not. But this was not the case. When an
>   instance is created, it should have the reference of 1, and if
>   it should not be removed, it must be greater than 1. The boot up
>   code set normal instances with a ref count of 0, which could get
>   removed if something accessed it and then released it. And memory
>   mapped instances had a ref count of 1 which meant it could be deleted,
>   and bad things happen. Keep normal instances ref count as 1, and
>   set memory mapped instances ref count to 2.
> 
> - Protect sub buffer size (order) updates from other modifications
> 
>   When a ring buffer is changing the size of its sub-buffers, no other
>   operations should be performed on the ring buffer. That includes
>   reading it. But the locking only grabbed the buffer->mutex that
>   keeps some operations from touching the ring buffer. It also must
>   hold the cpu_buffer->reader_lock as well when updates happen as
>   other paths use that to do some operations on the ring buffer.

Oops, used the wrong script. This was supposed to be "for-linus" not "for-next".

Oh well.

-- Steve