[PATCH v1 0/9] ring-buffer: Enable Clang context analysis

[PATCH v1 0/9] ring-buffer: Enable Clang context analysis

[Vincent Donnefort]

trace_buffer::subbuf_order has caused several recent resize races from
unguarded reads outside buffer->mutex. To prevent further regressions at
build time, enable Clang context analysis for the ring buffer and guard
subbuf_order with __guarded_by(&mutex).

  - Patches 1-3: Fix cpumask/hotplug races around buffer->buffers and
    trace_rb_cpu_prepare().
  - Patch 4: Fix a trace_marker resize race.
  - Patches 5-7: Remove subbuf_order reads from the write path.
  - Patches 8-9: Enable context analysis on ring_buffer.o and annotate
    subbuf_order with __guarded_by(&mutex).

Note: Patch 5 conflicts with the series removing absolute timestamps
[1], though both are independent on v7.4-rc1.

[1] https://lore.kernel.org/20261105202814.1350312-1-vdonnefort@google.com/

Vincent Donnefort (9):
  ring-buffer: Always read buffer->buffers after testing the cpumask
  ring-buffer: Drop cpus_read_lock() from ring_buffer_poll_remote()
  ring-buffer: Hold buffer->mutex in trace_rb_cpu_prepare()
  tracing: Cache ring_buffer_max_event_size() for trace_marker
  ring-buffer: Validate event length against rb_page_capacity()
  ring-buffer: Introduce rb_start_write() and rb_end_write()
  ring-buffer: Avoid reading buffer::subbuf_order on the write path
  ring-buffer: Enable context analysis
  ring-buffer: Annotate subbuf_order with __guarded_by(&mutex)

 kernel/trace/Makefile      |   2 +
 kernel/trace/ring_buffer.c | 350 ++++++++++++++++++++-----------------
 kernel/trace/trace.c       |   9 +-
 3 files changed, 193 insertions(+), 168 deletions(-)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.56.0.567.gdb80ae8cd5-goog

[PATCH v1 1/9] ring-buffer: Always read buffer->buffers after testing the cpumask

[Vincent Donnefort]

ring_buffer_reset_cpu() reads buffer->buffers[cpu] before testing the
cpumask. If trace_rb_cpu_prepare() sets the bit in between, the cpumask
check passes while cpu_buffer contains a stale value. Read
buffer->buffers[cpu] after the cpumask test passes.

While ring_buffer_peek() and ring_buffer_read_page() cannot hit this
race because callers already check the cpumask, apply the same pattern
for consistency.

Fixes: 554f786e284a ("ring-buffer: only allocate buffers for online cpus")
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
---
 kernel/trace/ring_buffer.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 1fd28a72e6ee..bd8126f6353b 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -6340,7 +6340,7 @@ struct ring_buffer_event *
 ring_buffer_peek(struct trace_buffer *buffer, int cpu, u64 *ts,
 		 unsigned long *lost_events)
 {
-	struct ring_buffer_per_cpu *cpu_buffer = buffer->buffers[cpu];
+	struct ring_buffer_per_cpu *cpu_buffer;
 	struct ring_buffer_event *event;
 	unsigned long flags;
 	bool dolock;
@@ -6348,6 +6348,8 @@ ring_buffer_peek(struct trace_buffer *buffer, int cpu, u64 *ts,
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
 		return NULL;
 
+	cpu_buffer = buffer->buffers[cpu];
+
  again:
 	local_irq_save(flags);
 	dolock = rb_reader_lock(cpu_buffer);
@@ -6740,11 +6742,13 @@ static void reset_disabled_cpu_buffer(struct ring_buffer_per_cpu *cpu_buffer)
  */
 void ring_buffer_reset_cpu(struct trace_buffer *buffer, int cpu)
 {
-	struct ring_buffer_per_cpu *cpu_buffer = buffer->buffers[cpu];
+	struct ring_buffer_per_cpu *cpu_buffer;
 
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
 		return;
 
+	cpu_buffer = buffer->buffers[cpu];
+
 	/* prevent another thread from changing buffer sizes */
 	mutex_lock(&buffer->mutex);
 
@@ -7198,7 +7202,7 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
 			  struct buffer_data_read_page *data_page,
 			  size_t len, int cpu, int full)
 {
-	struct ring_buffer_per_cpu *cpu_buffer = buffer->buffers[cpu];
+	struct ring_buffer_per_cpu *cpu_buffer;
 	struct ring_buffer_event *event;
 	struct buffer_data_page *dpage;
 	struct buffer_page *reader;
@@ -7211,6 +7215,8 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
 		return -1;
 
+	cpu_buffer = buffer->buffers[cpu];
+
 	/*
 	 * If len is not big enough to hold the page header, then
 	 * we can not copy anything.
-- 
2.56.0.567.gdb80ae8cd5-goog

[PATCH v1 2/9] ring-buffer: Drop cpus_read_lock() from ring_buffer_poll_remote()

[Vincent Donnefort]

ring_buffer_poll_remote() holds cpus_read_lock() while walking
buffer->cpumask. Ring buffers are never freed on CPU hotunplug, and
trace_rb_cpu_prepare() already orders buffer->buffers with the cpumask
update (with rb_cpu_sync()). Drop cpus_read_lock() as it is
unnecessary.

Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
---
 kernel/trace/ring_buffer.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index bd8126f6353b..32d3c47dbdf0 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -6918,8 +6918,6 @@ int ring_buffer_poll_remote(struct trace_buffer *buffer, int cpu)
 		return 0;
 	}
 
-	guard(cpus_read_lock)();
-
 	/*
 	 * Make sure all the ring buffers are up to date before we start reading
 	 * them.
-- 
2.56.0.567.gdb80ae8cd5-goog

[PATCH v1 3/9] ring-buffer: Hold buffer->mutex in trace_rb_cpu_prepare()

[Vincent Donnefort]

ring_buffer_reset() walks buffer->cpumask twice across synchronize_rcu()
under buffer->mutex. If trace_rb_cpu_prepare() adds a CPU in between,
the second loop resets an undisabled buffer.

Hold buffer->mutex in trace_rb_cpu_prepare() to synchronise it with the
ring buffer reset instead of locking the entire hotplug path. As a
consequence, the cpus_read_lock() in ring_buffer_subbuf_order_set() can
be dropped. The one in ring_buffer_resize() still stands because it
schedules insertion or deletion of ring buffer pages on each CPU.

Fixes: b23d7a5f4a07 ("ring-buffer: speed up buffer resets by avoiding synchronize_rcu for each CPU")
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
---
 kernel/trace/ring_buffer.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 32d3c47dbdf0..80f500ffd7ac 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -2824,10 +2824,10 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
 	buffer->clock = trace_clock_local;
 	buffer->reader_lock_key = key;
 
+	mutex_init(&buffer->mutex);
 	init_irq_work(&buffer->irq_work.work, rb_wake_up_waiters);
 	init_waitqueue_head(&buffer->irq_work.waiters);
 
-
 	bsize = sizeof(void *) * nr_cpu_ids;
 	buffer->buffers = kzalloc(ALIGN(bsize, cache_line_size()),
 				  GFP_KERNEL);
@@ -2923,8 +2923,6 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
 	if (ret < 0)
 		goto fail_free_buffers;
 
-	mutex_init(&buffer->mutex);
-
 	/* Persistent ring buffer needs to flush cache before reboot. */
 	if (start && end) {
 		buffer->flush_nb.notifier_call = rb_flush_buffer_cb;
@@ -3330,8 +3328,9 @@ int ring_buffer_resize(struct trace_buffer *buffer, unsigned long size,
 		return 0;
 
 	/*
-	 * Keep CPUs from coming online while resizing to synchronize
-	 * with new per CPU buffers being created.
+	 * Insertion or removal of pages in a ring buffer is scheduled on its
+	 * own CPU if online, so cpu_online_mask must stay stable. Must be taken
+	 * before buffer->mutex.
 	 */
 	guard(cpus_read_lock)();
 
@@ -7475,8 +7474,6 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
 	if (psize > RB_WRITE_MASK + 1)
 		return -EINVAL;
 
-	guard(cpus_read_lock)();
-
 	/* prevent another thread from changing buffer sizes */
 	guard(mutex)(&buffer->mutex);
 
@@ -8077,6 +8074,8 @@ int trace_rb_cpu_prepare(unsigned int cpu, struct hlist_node *node)
 	if (cpumask_test_cpu(cpu, buffer->cpumask))
 		return 0;
 
+	guard(mutex)(&buffer->mutex);
+
 	nr_pages = 0;
 	nr_pages_same = 1;
 	/* check if all cpu sizes are same */
-- 
2.56.0.567.gdb80ae8cd5-goog

[PATCH v1 4/9] tracing: Cache ring_buffer_max_event_size() for trace_marker

[Vincent Donnefort]

write_marker_to_buffer() calls ring_buffer_max_event_size() twice
without synchronisation against ring_buffer_subbuf_order_set(). If
subbuf_order grows between the two calls, cnt may expand past
TRACE_MARKER_MAX_SIZE. Read the maximum event size once so cnt can only
shrink, and drop the now-unreachable WARN_ON_ONCE().

Fixes: 2808e31ec12e ("ring-buffer: Add interface for configuring trace sub buffer size")
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
---
 kernel/trace/trace.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index e8c4efa8107b..f44bde7c51b7 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -5962,15 +5962,14 @@ static ssize_t write_marker_to_buffer(struct trace_array *tr, const char *buf,
 	event = __trace_buffer_lock_reserve(buffer, TRACE_PRINT, size,
 					    tracing_gen_ctx());
 	if (unlikely(!event)) {
+		size_t max_event_size = ring_buffer_max_event_size(buffer);
+
 		/*
 		 * If the size was greater than what was allowed, then
 		 * make it smaller and try again.
 		 */
-		if (size > ring_buffer_max_event_size(buffer)) {
-			cnt = ring_buffer_max_event_size(buffer) - meta_size;
-			/* The above should only happen once */
-			if (WARN_ON_ONCE(cnt + meta_size == size))
-				return -EBADF;
+		if (size > max_event_size) {
+			cnt = max_event_size - meta_size;
 			goto again;
 		}
 
-- 
2.56.0.567.gdb80ae8cd5-goog

[PATCH v1 5/9] ring-buffer: Validate event length against rb_page_capacity()

[Vincent Donnefort]

In rb_reserve_next_event(), the absolute timestamp path compares length
against rb_subbuf_max_data_size(). The latter subtracts the event
header while rb_calculate_event_length() already accounts for it, which
incorrectly rejects valid reservations of ring_buffer_max_event_size().

Compare against commit_page capacity instead so event headers are
accounted for consistently on both sides. This check also applies to
normal timestamp mode, so share it across both paths.

In rb_reserve_next_event(), commit_page is stable and avoids accessing
the mutex-protected buffer::subbuf_order, rendering the early calls to
rb_subbuf_max_data_size() in ring_buffer_lock_reserve() and
ring_buffer_write() unnecessary. Drop them.

Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
---
 kernel/trace/ring_buffer.c | 44 ++++++++++++++------------------------
 1 file changed, 16 insertions(+), 28 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 80f500ffd7ac..9834f9dae440 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -625,23 +625,6 @@ static __always_inline unsigned int rb_subbuf_capacity(struct trace_buffer *buff
 	return rb_subbuf_size(buffer) - BUF_PAGE_HDR_SIZE;
 }
 
-/**
- * rb_subbuf_max_data_size - Get the maximum payload size of a single event
- * @buffer:	A trace buffer
- *
- * Return: The maximum data payload size that can be stored in a single event.
- */
-static __always_inline unsigned int rb_subbuf_max_data_size(struct trace_buffer *buffer)
-{
-	struct ring_buffer_event *event;
-
-	/*
-	 * surely rb_subbuf_capacity() is bigger than
-	 * RINGBUF_TYPE_DATA_TYPE_LEN_MAX (see ring_buffer_event_length).
-	 */
-	return rb_subbuf_capacity(buffer) - RB_EVNT_HDR_SIZE - sizeof(event->array[0]);
-}
-
 static unsigned long rb_calculate_nr_pages(struct trace_buffer *buffer, unsigned long size)
 {
 	return max(DIV_ROUND_UP(size, rb_subbuf_capacity(buffer)), RING_BUFFER_MIN_NR_PAGES);
@@ -4143,7 +4126,7 @@ rb_update_event(struct ring_buffer_per_cpu *cpu_buffer,
 		event->type_len = DIV_ROUND_UP(length, RB_ALIGNMENT);
 }
 
-static unsigned rb_calculate_event_length(unsigned length)
+static unsigned long rb_calculate_event_length(unsigned long length)
 {
 	struct ring_buffer_event event; /* Used only for sizeof array */
 
@@ -4986,12 +4969,13 @@ rb_reserve_next_event(struct trace_buffer *buffer,
 	if (ring_buffer_time_stamp_abs(cpu_buffer->buffer)) {
 		add_ts_default = RB_ADD_STAMP_ABSOLUTE;
 		info.length += RB_LEN_TIME_EXTEND;
-		if (info.length > rb_subbuf_max_data_size(cpu_buffer->buffer))
-			goto out_fail;
 	} else {
 		add_ts_default = RB_ADD_STAMP_NONE;
 	}
 
+	if (unlikely(info.length > rb_page_capacity(cpu_buffer->commit_page)))
+		goto out_fail;
+
  again:
 	info.add_timestamp = add_ts_default;
 	info.delta = 0;
@@ -5061,9 +5045,6 @@ ring_buffer_lock_reserve(struct trace_buffer *buffer, unsigned long length)
 	if (unlikely(atomic_read(&cpu_buffer->record_disabled)))
 		goto out;
 
-	if (unlikely(length > rb_subbuf_max_data_size(buffer)))
-		goto out;
-
 	if (unlikely(trace_recursive_lock(cpu_buffer)))
 		goto out;
 
@@ -5208,9 +5189,6 @@ int ring_buffer_write(struct trace_buffer *buffer,
 	if (atomic_read(&cpu_buffer->record_disabled))
 		return -EBUSY;
 
-	if (length > rb_subbuf_max_data_size(buffer))
-		return -EBUSY;
-
 	if (unlikely(trace_recursive_lock(cpu_buffer)))
 		return -EBUSY;
 
@@ -6575,11 +6553,21 @@ EXPORT_SYMBOL_GPL(ring_buffer_capacity);
  */
 unsigned long ring_buffer_max_event_size(struct trace_buffer *buffer)
 {
+	struct ring_buffer_event *event;
+	unsigned long max_data_size;
+
+	/*
+	 * surely rb_subbuf_capacity() is bigger than
+	 * RINGBUF_TYPE_DATA_TYPE_LEN_MAX (see ring_buffer_event_length).
+	 */
+	max_data_size = rb_subbuf_capacity(buffer) - RB_EVNT_HDR_SIZE -
+			sizeof(event->array[0]);
+
 	/* If abs timestamp is requested, events have a timestamp too */
 	if (ring_buffer_time_stamp_abs(buffer))
-		return rb_subbuf_max_data_size(buffer) - RB_LEN_TIME_EXTEND;
+		return max_data_size - RB_LEN_TIME_EXTEND;
 
-	return rb_subbuf_max_data_size(buffer);
+	return max_data_size;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_max_event_size);
 
-- 
2.56.0.567.gdb80ae8cd5-goog

[PATCH v1 6/9] ring-buffer: Introduce rb_start_write() and rb_end_write()

[Vincent Donnefort]

Factor the common write-context entry (preemption disabling, disabled
checks, cpumask testing, recursive locking) and exit paths shared by
ring_buffer_lock_reserve() and ring_buffer_write() into a pair of
functions: rb_start_write() and rb_end_write().

No functional change intended.

Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
---
 kernel/trace/ring_buffer.c | 106 ++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 55 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 9834f9dae440..02d3cc193d1e 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -4495,6 +4495,45 @@ trace_recursive_unlock(struct ring_buffer_per_cpu *cpu_buffer)
 		cpu_buffer->current_context - (1 << cpu_buffer->nest);
 }
 
+static __always_inline struct ring_buffer_per_cpu *
+rb_start_write(struct trace_buffer *buffer)
+{
+	struct ring_buffer_per_cpu *cpu_buffer;
+	int cpu;
+
+	/* If we are tracing schedule, we don't want to recurse */
+	preempt_disable_notrace();
+
+	if (unlikely(atomic_read(&buffer->record_disabled)))
+		goto out;
+
+	cpu = raw_smp_processor_id();
+
+	if (unlikely(!cpumask_test_cpu(cpu, buffer->cpumask)))
+		goto out;
+
+	cpu_buffer = buffer->buffers[cpu];
+
+	if (unlikely(atomic_read(&cpu_buffer->record_disabled)))
+		goto out;
+
+	if (unlikely(trace_recursive_lock(cpu_buffer)))
+		goto out;
+
+	return cpu_buffer;
+
+out:
+	preempt_enable_notrace();
+
+	return NULL;
+}
+
+static __always_inline void rb_end_write(struct ring_buffer_per_cpu *cpu_buffer)
+{
+	trace_recursive_unlock(cpu_buffer);
+	preempt_enable_notrace();
+}
+
 /* The recursive locking above uses 5 bits */
 #define NESTED_BITS 5
 
@@ -4563,9 +4602,7 @@ int ring_buffer_unlock_commit(struct trace_buffer *buffer)
 
 	rb_wakeups(buffer, cpu_buffer);
 
-	trace_recursive_unlock(cpu_buffer);
-
-	preempt_enable_notrace();
+	rb_end_write(cpu_buffer);
 
 	return 0;
 }
@@ -5027,38 +5064,16 @@ ring_buffer_lock_reserve(struct trace_buffer *buffer, unsigned long length)
 {
 	struct ring_buffer_per_cpu *cpu_buffer;
 	struct ring_buffer_event *event;
-	int cpu;
 
-	/* If we are tracing schedule, we don't want to recurse */
-	preempt_disable_notrace();
-
-	if (unlikely(atomic_read(&buffer->record_disabled)))
-		goto out;
-
-	cpu = raw_smp_processor_id();
-
-	if (unlikely(!cpumask_test_cpu(cpu, buffer->cpumask)))
-		goto out;
-
-	cpu_buffer = buffer->buffers[cpu];
-
-	if (unlikely(atomic_read(&cpu_buffer->record_disabled)))
-		goto out;
-
-	if (unlikely(trace_recursive_lock(cpu_buffer)))
-		goto out;
+	cpu_buffer = rb_start_write(buffer);
+	if (!cpu_buffer)
+		return NULL;
 
 	event = rb_reserve_next_event(buffer, cpu_buffer, length);
 	if (!event)
-		goto out_unlock;
+		rb_end_write(cpu_buffer);
 
 	return event;
-
- out_unlock:
-	trace_recursive_unlock(cpu_buffer);
- out:
-	preempt_enable_notrace();
-	return NULL;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_lock_reserve);
 
@@ -5143,11 +5158,7 @@ void ring_buffer_discard_commit(struct trace_buffer *buffer,
 	rb_decrement_entry(cpu_buffer, event);
 	rb_try_to_discard(cpu_buffer, event);
 	rb_end_commit(cpu_buffer);
-
-	trace_recursive_unlock(cpu_buffer);
-
-	preempt_enable_notrace();
-
+	rb_end_write(cpu_buffer);
 }
 EXPORT_SYMBOL_GPL(ring_buffer_discard_commit);
 
@@ -5170,31 +5181,16 @@ int ring_buffer_write(struct trace_buffer *buffer,
 {
 	struct ring_buffer_per_cpu *cpu_buffer;
 	struct ring_buffer_event *event;
-	void *body;
 	int ret = -EBUSY;
-	int cpu;
+	void *body;
 
-	guard(preempt_notrace)();
-
-	if (atomic_read(&buffer->record_disabled))
-		return -EBUSY;
-
-	cpu = raw_smp_processor_id();
-
-	if (!cpumask_test_cpu(cpu, buffer->cpumask))
-		return -EBUSY;
-
-	cpu_buffer = buffer->buffers[cpu];
-
-	if (atomic_read(&cpu_buffer->record_disabled))
-		return -EBUSY;
-
-	if (unlikely(trace_recursive_lock(cpu_buffer)))
+	cpu_buffer = rb_start_write(buffer);
+	if (!cpu_buffer)
 		return -EBUSY;
 
 	event = rb_reserve_next_event(buffer, cpu_buffer, length);
 	if (!event)
-		goto out_unlock;
+		goto out;
 
 	body = rb_event_data(event);
 
@@ -5206,8 +5202,8 @@ int ring_buffer_write(struct trace_buffer *buffer,
 
 	ret = 0;
 
- out_unlock:
-	trace_recursive_unlock(cpu_buffer);
+out:
+	rb_end_write(cpu_buffer);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_write);
-- 
2.56.0.567.gdb80ae8cd5-goog

[PATCH v1 7/9] ring-buffer: Avoid reading buffer::subbuf_order on the write path

[Vincent Donnefort]

buffer::subbuf_order is protected by buffer::mutex, which cannot be
held on the write path. While reading it is safe because writers are
disabled during order changes, reading the order from commit_page (which
is pinned for the duration of the commit) avoids confusing context
analysis.

Because rb_subbuf_size() and rb_subbuf_capacity() are no longer on the
write hot path, drop their __always_inline annotations.

Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
---
 kernel/trace/ring_buffer.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 02d3cc193d1e..82f6e3df4627 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -606,7 +606,7 @@ struct trace_buffer {
 	unsigned int			subbuf_order;
 };
 
-static __always_inline unsigned int rb_subbuf_size(struct trace_buffer *buffer)
+static unsigned int rb_subbuf_size(struct trace_buffer *buffer)
 {
 	return PAGE_SIZE << buffer->subbuf_order;
 }
@@ -620,7 +620,7 @@ static __always_inline unsigned int rb_subbuf_size(struct trace_buffer *buffer)
  *
  * Return: The maximum size available for events in a trace buffer subbuffer.
  */
-static __always_inline unsigned int rb_subbuf_capacity(struct trace_buffer *buffer)
+static unsigned int rb_subbuf_capacity(struct trace_buffer *buffer)
 {
 	return rb_subbuf_size(buffer) - BUF_PAGE_HDR_SIZE;
 }
@@ -632,15 +632,15 @@ static unsigned long rb_calculate_nr_pages(struct trace_buffer *buffer, unsigned
 
 /**
  * rb_subbuf_start - Get the start address of a subbuffer
- * @buffer:	A trace buffer
+ * @cpu_buffer:	A per-CPU ring buffer
  * @addr:	An address of an event on a subbuffer
  *
  * Return: The start of the subbuffer for where @addr sits
  */
 static __always_inline
-unsigned long rb_subbuf_start(struct trace_buffer *buffer, unsigned long addr)
+unsigned long rb_subbuf_start(struct ring_buffer_per_cpu *cpu_buffer, unsigned long addr)
 {
-	return addr & ~((unsigned long)(rb_subbuf_size(buffer) - 1));
+	return addr & ~((unsigned long)(PAGE_SIZE << cpu_buffer->commit_page->order) - 1);
 }
 
 static bool rb_is_static(struct ring_buffer_per_cpu *cpu_buffer)
@@ -3582,7 +3582,7 @@ rb_event_index(struct ring_buffer_per_cpu *cpu_buffer, struct ring_buffer_event
 {
 	unsigned long addr = (unsigned long)event;
 
-	addr &= (unsigned long)rb_subbuf_size(cpu_buffer->buffer) - 1;
+	addr &= (unsigned long)(PAGE_SIZE << cpu_buffer->commit_page->order) - 1;
 
 	return addr - BUF_PAGE_HDR_SIZE;
 }
@@ -4168,7 +4168,7 @@ rb_try_to_discard(struct ring_buffer_per_cpu *cpu_buffer,
 
 	new_index = rb_event_index(cpu_buffer, event);
 	old_index = new_index + rb_event_ts_length(event);
-	addr = rb_subbuf_start(cpu_buffer->buffer, (unsigned long)event);
+	addr = rb_subbuf_start(cpu_buffer, (unsigned long)event);
 
 	bpage = READ_ONCE(cpu_buffer->tail_page);
 
@@ -5091,7 +5091,7 @@ rb_decrement_entry(struct ring_buffer_per_cpu *cpu_buffer,
 	struct buffer_page *bpage = cpu_buffer->commit_page;
 	struct buffer_page *start;
 
-	addr = rb_subbuf_start(cpu_buffer->buffer, addr);
+	addr = rb_subbuf_start(cpu_buffer, addr);
 
 	/* Do the likely case first */
 	if (likely(bpage->page == (void *)addr)) {
-- 
2.56.0.567.gdb80ae8cd5-goog

[PATCH v1 8/9] ring-buffer: Enable context analysis

[Vincent Donnefort]

Locking of some variables has proven to be fiddly recently. To mitigate
further issues, enable context analysis for the ring buffer.

This comes at the expense of a few annotations and the removal of the
helpers rb_get_mapped_buffer() and rb_put_mapped_buffer(), used in a
single place anyway.

No functional change intended.

Cc: Marco Elver <elver@google.com>
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
---
 kernel/trace/Makefile      |  2 ++
 kernel/trace/ring_buffer.c | 50 ++++++++++++++++----------------------
 2 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index b0c44f438999..165593ec4c03 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -53,6 +53,8 @@ endif
 KCOV_INSTRUMENT_trace_preemptirq.o := n
 KCOV_INSTRUMENT_trace_irqsoff.o := n
 
+CONTEXT_ANALYSIS_ring_buffer.o := y
+
 CFLAGS_bpf_trace.o := -I$(src)
 
 CFLAGS_trace_benchmark.o := -I$(src)
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 82f6e3df4627..99ade8b41b53 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -6269,6 +6269,7 @@ rb_iter_peek(struct ring_buffer_iter *iter, u64 *ts)
 EXPORT_SYMBOL_GPL(ring_buffer_iter_peek);
 
 static inline bool rb_reader_lock(struct ring_buffer_per_cpu *cpu_buffer)
+	__acquires(&cpu_buffer->reader_lock)
 {
 	if (likely(!in_nmi())) {
 		raw_spin_lock(&cpu_buffer->reader_lock);
@@ -6289,14 +6290,18 @@ static inline bool rb_reader_lock(struct ring_buffer_per_cpu *cpu_buffer)
 
 	/* Continue without locking, but disable the ring buffer */
 	atomic_inc(&cpu_buffer->record_disabled);
+	__acquire(&cpu_buffer->reader_lock);
 	return false;
 }
 
 static inline void
 rb_reader_unlock(struct ring_buffer_per_cpu *cpu_buffer, bool locked)
+	__releases(&cpu_buffer->reader_lock)
 {
 	if (likely(locked))
 		raw_spin_unlock(&cpu_buffer->reader_lock);
+	else
+		__release(&cpu_buffer->reader_lock);
 }
 
 /**
@@ -6458,11 +6463,15 @@ ring_buffer_read_start(struct trace_buffer *buffer, int cpu, gfp_t flags)
 	 */
 	if (gfpflags_allow_blocking(flags))
 		mutex_lock(&buffer->mutex);
+	else
+		__acquire(&buffer->mutex);
 
 	atomic_inc(&cpu_buffer->resize_disabled);
 
 	if (gfpflags_allow_blocking(flags))
 		mutex_unlock(&buffer->mutex);
+	else
+		__release(&buffer->mutex);
 
 	/* Holds the entire event: data and meta data. */
 	iter->event_size = rb_page_capacity(READ_ONCE(cpu_buffer->reader_page));
@@ -7648,31 +7657,6 @@ static void rb_setup_ids_meta_page(struct ring_buffer_per_cpu *cpu_buffer,
 	rb_update_meta_page(cpu_buffer);
 }
 
-static struct ring_buffer_per_cpu *
-rb_get_mapped_buffer(struct trace_buffer *buffer, int cpu)
-{
-	struct ring_buffer_per_cpu *cpu_buffer;
-
-	if (!cpumask_test_cpu(cpu, buffer->cpumask))
-		return ERR_PTR(-EINVAL);
-
-	cpu_buffer = buffer->buffers[cpu];
-
-	mutex_lock(&cpu_buffer->mapping_lock);
-
-	if (!cpu_buffer->user_mapped) {
-		mutex_unlock(&cpu_buffer->mapping_lock);
-		return ERR_PTR(-ENODEV);
-	}
-
-	return cpu_buffer;
-}
-
-static void rb_put_mapped_buffer(struct ring_buffer_per_cpu *cpu_buffer)
-{
-	mutex_unlock(&cpu_buffer->mapping_lock);
-}
-
 /*
  * Fast-path for rb_buffer_(un)map(). Called whenever the meta-page doesn't need
  * to be set-up or torn-down.
@@ -7948,9 +7932,17 @@ int ring_buffer_map_get_reader(struct trace_buffer *buffer, int cpu)
 	unsigned long reader_size;
 	unsigned long flags;
 
-	cpu_buffer = rb_get_mapped_buffer(buffer, cpu);
-	if (IS_ERR(cpu_buffer))
-		return (int)PTR_ERR(cpu_buffer);
+	if (!cpumask_test_cpu(cpu, buffer->cpumask))
+		return -EINVAL;
+
+	cpu_buffer = buffer->buffers[cpu];
+
+	mutex_lock(&cpu_buffer->mapping_lock);
+
+	if (!cpu_buffer->user_mapped) {
+		mutex_unlock(&cpu_buffer->mapping_lock);
+		return -ENODEV;
+	}
 
 	raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
 
@@ -8031,7 +8023,7 @@ int ring_buffer_map_get_reader(struct trace_buffer *buffer, int cpu)
 	rb_update_meta_page(cpu_buffer);
 
 	raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
-	rb_put_mapped_buffer(cpu_buffer);
+	mutex_unlock(&cpu_buffer->mapping_lock);
 
 	return 0;
 }
-- 
2.56.0.567.gdb80ae8cd5-goog

[PATCH v1 9/9] ring-buffer: Annotate subbuf_order with __guarded_by(&mutex)

[Vincent Donnefort]

trace_buffer::subbuf_order caused a few locking issues recently. To
mitigate any coming problems, annotate it with __guarded_by() to
enforce its proper locking.

The API functions are still allowed to access it locklessly: they must
externally harden the races. For each, add a note about possible
concurrency to the kerneldoc. To that end, add a set of accessors:

   rb_subbuf_order_unsafe(), rb_subbuf_size_unsafe(),
   rb_subbuf_capacity_unsafe().

Not only do they use the unsafe annotation to silence context analysis,
but they also use a READ_ONCE() which was missing in some places.

Cc: Marco Elver <elver@google.com>
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>
---
 kernel/trace/ring_buffer.c | 115 ++++++++++++++++++++++++++-----------
 1 file changed, 80 insertions(+), 35 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 99ade8b41b53..6ced0ba4fcad 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -603,10 +603,30 @@ struct trace_buffer {
 
 	struct ring_buffer_meta		*meta;
 
-	unsigned int			subbuf_order;
+	unsigned int			subbuf_order __guarded_by(&mutex);
 };
 
+static unsigned int rb_subbuf_order_unsafe(struct trace_buffer *buffer)
+{
+	/*
+	 * Unsafe read of subbuf_order, restricted to API paths that either
+	 * synchronize it externally or tolerate a race.
+	 */
+	return context_unsafe(READ_ONCE(buffer->subbuf_order));
+}
+
+static unsigned int rb_subbuf_size_unsafe(struct trace_buffer *buffer)
+{
+	return PAGE_SIZE << rb_subbuf_order_unsafe(buffer);
+}
+
+static unsigned int rb_subbuf_capacity_unsafe(struct trace_buffer *buffer)
+{
+	return rb_subbuf_size_unsafe(buffer) - BUF_PAGE_HDR_SIZE;
+}
+
 static unsigned int rb_subbuf_size(struct trace_buffer *buffer)
+	__must_hold(&buffer->mutex)
 {
 	return PAGE_SIZE << buffer->subbuf_order;
 }
@@ -621,11 +641,13 @@ static unsigned int rb_subbuf_size(struct trace_buffer *buffer)
  * Return: The maximum size available for events in a trace buffer subbuffer.
  */
 static unsigned int rb_subbuf_capacity(struct trace_buffer *buffer)
+	__must_hold(&buffer->mutex)
 {
 	return rb_subbuf_size(buffer) - BUF_PAGE_HDR_SIZE;
 }
 
 static unsigned long rb_calculate_nr_pages(struct trace_buffer *buffer, unsigned long size)
+	__must_hold(&buffer->mutex)
 {
 	return max(DIV_ROUND_UP(size, rb_subbuf_capacity(buffer)), RING_BUFFER_MIN_NR_PAGES);
 }
@@ -696,8 +718,9 @@ int ring_buffer_print_page_header(struct trace_buffer *buffer, struct trace_seq
 	trace_seq_printf(s, "\tfield: char data;\t"
 			 "offset:%u;\tsize:%u;\tsigned:%u;\n",
 			 (unsigned int)offsetof(typeof(field), data),
-			 (unsigned int)(buffer ? rb_subbuf_capacity(buffer) :
-						 PAGE_SIZE - BUF_PAGE_HDR_SIZE),
+			 (unsigned int)(buffer ?
+					rb_subbuf_capacity_unsafe(buffer) :
+					PAGE_SIZE - BUF_PAGE_HDR_SIZE),
 			 (unsigned int)is_signed_type(char));
 
 	return !trace_seq_has_overflowed(s);
@@ -1685,15 +1708,17 @@ rb_range_align_subbuf(unsigned long addr, unsigned int subbuf_size, unsigned lon
  */
 static void *rb_range_meta(struct trace_buffer *buffer, unsigned long nr_pages, int cpu)
 {
-	unsigned int subbuf_size = rb_subbuf_size(buffer);
 	struct ring_buffer_cpu_meta *meta;
 	struct ring_buffer_meta *bmeta;
 	unsigned long ptr, nr_subbufs;
+	unsigned int subbuf_size;
 
 	bmeta = buffer->meta;
 	if (!bmeta)
 		return NULL;
 
+	/* persistent buffers are static and cannot be resized */
+	subbuf_size = context_unsafe(rb_subbuf_size(buffer));
 	ptr = (unsigned long)bmeta + bmeta->buffers_offset;
 	meta = (struct ring_buffer_cpu_meta *)ptr;
 
@@ -2427,10 +2452,11 @@ static void *ring_buffer_desc_page(struct ring_buffer_desc *desc, unsigned int p
 	return page_id >= desc->nr_page_va ? NULL : (void *)desc->page_va[page_id];
 }
 
-static int __rb_allocate_pages(struct ring_buffer_per_cpu *cpu_buffer,
+static int __rb_allocate_pages(struct trace_buffer *buffer,
+			       struct ring_buffer_per_cpu *cpu_buffer,
 			       unsigned long nr_pages, struct list_head *pages)
+	__must_hold(&buffer->mutex)
 {
-	struct trace_buffer *buffer = cpu_buffer->buffer;
 	unsigned int subbuf_order = buffer->subbuf_order;
 	struct ring_buffer_cpu_meta *meta = NULL;
 	struct buffer_page *bpage, *tmp;
@@ -2531,14 +2557,16 @@ static int __rb_allocate_pages(struct ring_buffer_per_cpu *cpu_buffer,
 	return -ENOMEM;
 }
 
-static int rb_allocate_pages(struct ring_buffer_per_cpu *cpu_buffer,
+static int rb_allocate_pages(struct trace_buffer *buffer,
+			     struct ring_buffer_per_cpu *cpu_buffer,
 			     unsigned long nr_pages)
+	__must_hold(&buffer->mutex)
 {
 	LIST_HEAD(pages);
 
 	WARN_ON(!nr_pages);
 
-	if (__rb_allocate_pages(cpu_buffer, nr_pages, &pages))
+	if (__rb_allocate_pages(buffer, cpu_buffer, nr_pages, &pages))
 		return -ENOMEM;
 
 	/*
@@ -2558,6 +2586,7 @@ static int rb_allocate_pages(struct ring_buffer_per_cpu *cpu_buffer,
 
 static struct ring_buffer_per_cpu *
 rb_allocate_cpu_buffer(struct trace_buffer *buffer, unsigned long nr_pages, int cpu)
+	__must_hold(&buffer->mutex)
 {
 	struct ring_buffer_per_cpu *cpu_buffer __free(kfree) =
 		alloc_cpu_buffer(cpu);
@@ -2636,7 +2665,7 @@ rb_allocate_cpu_buffer(struct trace_buffer *buffer, unsigned long nr_pages, int
 	INIT_LIST_HEAD(&cpu_buffer->reader_page->list);
 	INIT_LIST_HEAD(&cpu_buffer->new_pages);
 
-	ret = rb_allocate_pages(cpu_buffer, nr_pages);
+	ret = rb_allocate_pages(buffer, cpu_buffer, nr_pages);
 	if (ret < 0)
 		goto fail_free_reader;
 
@@ -2783,6 +2812,7 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
 					 unsigned long scratch_size,
 					 struct lock_class_key *key,
 					 struct ring_buffer_remote *remote)
+	__context_unsafe(init)
 {
 	struct trace_buffer *buffer __free(kfree) = NULL;
 	unsigned int subbuf_size;
@@ -2800,6 +2830,7 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
 	if (!zalloc_cpumask_var(&buffer->cpumask, GFP_KERNEL))
 		return NULL;
 
+	mutex_init(&buffer->mutex);
 	buffer->subbuf_order = order;
 	subbuf_size = (PAGE_SIZE << order);
 
@@ -2807,7 +2838,6 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
 	buffer->clock = trace_clock_local;
 	buffer->reader_lock_key = key;
 
-	mutex_init(&buffer->mutex);
 	init_irq_work(&buffer->irq_work.work, rb_wake_up_waiters);
 	init_waitqueue_head(&buffer->irq_work.waiters);
 
@@ -3350,7 +3380,8 @@ int ring_buffer_resize(struct trace_buffer *buffer, unsigned long size,
 			 * allocated without receiving ENOMEM
 			 */
 			INIT_LIST_HEAD(&cpu_buffer->new_pages);
-			if (__rb_allocate_pages(cpu_buffer, cpu_buffer->nr_pages_to_update,
+			if (__rb_allocate_pages(buffer, cpu_buffer,
+						cpu_buffer->nr_pages_to_update,
 						&cpu_buffer->new_pages)) {
 				/* not enough memory for new pages */
 				err = -ENOMEM;
@@ -3410,8 +3441,9 @@ int ring_buffer_resize(struct trace_buffer *buffer, unsigned long size,
 
 		INIT_LIST_HEAD(&cpu_buffer->new_pages);
 		if (cpu_buffer->nr_pages_to_update > 0 &&
-			__rb_allocate_pages(cpu_buffer, cpu_buffer->nr_pages_to_update,
-					    &cpu_buffer->new_pages)) {
+		    __rb_allocate_pages(buffer, cpu_buffer,
+					cpu_buffer->nr_pages_to_update,
+					&cpu_buffer->new_pages)) {
 			err = -ENOMEM;
 			goto out_err;
 		}
@@ -6536,8 +6568,9 @@ EXPORT_SYMBOL_GPL(ring_buffer_iter_advance);
  * @buffer: The ring buffer.
  * @cpu: The CPU to get ring buffer capacity from.
  *
- * Capacity is the total memory available for recording event data in the
- * ring buffer. It excludes subbuffer headers.
+ * Capacity is the total memory available for recording event data in the ring
+ * buffer. It excludes subbuffer headers. No synchronization with either
+ * ring_buffer_subbuf_order_set() or ring_buffer_resize() is provided.
  *
  * Returns the capacity in bytes, or 0 if the CPU is not set in the buffer mask.
  */
@@ -6546,7 +6579,7 @@ unsigned long ring_buffer_capacity(struct trace_buffer *buffer, int cpu)
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
 		return 0;
 
-	return rb_subbuf_capacity(buffer) * buffer->buffers[cpu]->nr_pages;
+	return rb_subbuf_capacity_unsafe(buffer) * buffer->buffers[cpu]->nr_pages;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_capacity);
 
@@ -6554,6 +6587,8 @@ EXPORT_SYMBOL_GPL(ring_buffer_capacity);
  * ring_buffer_max_event_size - return the max data size of an event
  * @buffer: The ring buffer.
  *
+ * No synchronization with ring_buffer_subbuf_order_set() is provided.
+ *
  * Returns the maximum size an event can be.
  */
 unsigned long ring_buffer_max_event_size(struct trace_buffer *buffer)
@@ -6562,11 +6597,11 @@ unsigned long ring_buffer_max_event_size(struct trace_buffer *buffer)
 	unsigned long max_data_size;
 
 	/*
-	 * surely rb_subbuf_capacity() is bigger than
+	 * surely rb_subbuf_capacity_unsafe() is bigger than
 	 * RINGBUF_TYPE_DATA_TYPE_LEN_MAX (see ring_buffer_event_length).
 	 */
-	max_data_size = rb_subbuf_capacity(buffer) - RB_EVNT_HDR_SIZE -
-			sizeof(event->array[0]);
+	max_data_size = rb_subbuf_capacity_unsafe(buffer) -
+			RB_EVNT_HDR_SIZE - sizeof(event->array[0]);
 
 	/* If abs timestamp is requested, events have a timestamp too */
 	if (ring_buffer_time_stamp_abs(buffer))
@@ -6941,7 +6976,8 @@ int ring_buffer_poll_remote(struct trace_buffer *buffer, int cpu)
  * This function is useful for tracers that want to take a "snapshot"
  * of a CPU buffer and has another back up buffer lying around.
  * it is expected that the tracer handles the cpu buffer not being
- * used at the moment.
+ * used at the moment. No synchronization with
+ * ring_buffer_subbuf_order_set() is provided.
  */
 int ring_buffer_swap_cpu(struct trace_buffer *buffer_a,
 			 struct trace_buffer *buffer_b, int cpu)
@@ -6965,7 +7001,8 @@ int ring_buffer_swap_cpu(struct trace_buffer *buffer_a,
 	if (cpu_buffer_a->nr_pages != cpu_buffer_b->nr_pages)
 		return -EINVAL;
 
-	if (buffer_a->subbuf_order != buffer_b->subbuf_order)
+	/* Up to the callers to synchronize with a subbuf order change */
+	if (rb_subbuf_order_unsafe(buffer_a) != rb_subbuf_order_unsafe(buffer_b))
 		return -EINVAL;
 
 	if (atomic_read(&buffer_a->record_disabled))
@@ -7037,7 +7074,8 @@ EXPORT_SYMBOL_GPL(ring_buffer_swap_cpu);
  *
  * If @rpage is provided, and it has a different order than the current
  * subbuffer order, its payload will be freed and re-allocated. If it
- * already matches the order, it is simply returned.
+ * already matches the order, it is simply returned. No synchronization
+ * with ring_buffer_subbuf_order_set() is provided.
  *
  * Returns:
  *  0 on success, < 0 on error
@@ -7055,7 +7093,7 @@ int ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu,
 	if (!rpage)
 		return -EINVAL;
 
-	order = READ_ONCE(buffer->subbuf_order);
+	order = rb_subbuf_order_unsafe(buffer);
 
 	if (*rpage) {
 		if ((*rpage)->order == order)
@@ -7134,7 +7172,8 @@ void ring_buffer_free_read_page(struct trace_buffer *buffer, int cpu,
 	 * is different from the subbuffer order of the buffer -
 	 * we can't reuse it
 	 */
-	if (page_ref_count(page) > 1 || rpage->order != READ_ONCE(buffer->subbuf_order))
+	if (page_ref_count(page) > 1 ||
+	    rpage->order != rb_subbuf_order_unsafe(buffer))
 		goto out;
 
 	local_irq_save(flags);
@@ -7400,11 +7439,13 @@ EXPORT_SYMBOL_GPL(ring_buffer_read_page_size);
  * ring_buffer_subbuf_size_get - get size of the sub buffer.
  * @buffer: the buffer to get the sub buffer size from
  *
+ * No synchronization with ring_buffer_subbuf_order_set() is provided.
+ *
  * Returns size of the sub buffer, in bytes.
  */
 int ring_buffer_subbuf_size_get(struct trace_buffer *buffer)
 {
-	return rb_subbuf_size(buffer);
+	return rb_subbuf_size_unsafe(buffer);
 }
 EXPORT_SYMBOL_GPL(ring_buffer_subbuf_size_get);
 
@@ -7412,9 +7453,10 @@ EXPORT_SYMBOL_GPL(ring_buffer_subbuf_size_get);
  * ring_buffer_subbuf_order_get - get order of system sub pages in one buffer page.
  * @buffer: The ring_buffer to get the system sub page order from
  *
- * By default, one ring buffer sub page equals to one system page. This parameter
- * is configurable, per ring buffer. The size of the ring buffer sub page can be
- * extended, but must be an order of system page size.
+ * By default, one ring buffer sub page equals to one system page. This
+ * parameter is configurable, per ring buffer. The size of the ring buffer sub
+ * page can be extended, but must be an order of system page size. No
+ * synchronization with ring_buffer_subbuf_order_set() is provided.
  *
  * Returns the order of buffer sub page size, in system pages:
  * 0 means the sub buffer size is 1 system page and so forth.
@@ -7425,7 +7467,7 @@ int ring_buffer_subbuf_order_get(struct trace_buffer *buffer)
 	if (!buffer)
 		return -EINVAL;
 
-	return buffer->subbuf_order;
+	return rb_subbuf_order_unsafe(buffer);
 }
 EXPORT_SYMBOL_GPL(ring_buffer_subbuf_order_get);
 
@@ -7502,7 +7544,7 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
 
 		/* Allocate the new size buffer */
 		INIT_LIST_HEAD(&cpu_buffer->new_pages);
-		if (__rb_allocate_pages(cpu_buffer, nr_pages,
+		if (__rb_allocate_pages(buffer, cpu_buffer, nr_pages,
 					&cpu_buffer->new_pages)) {
 			/* not enough memory for new pages */
 			err = -ENOMEM;
@@ -7617,8 +7659,10 @@ static void rb_free_meta_page(struct ring_buffer_per_cpu *cpu_buffer)
 	cpu_buffer->meta_page = NULL;
 }
 
-static void rb_setup_ids_meta_page(struct ring_buffer_per_cpu *cpu_buffer,
+static void rb_setup_ids_meta_page(struct trace_buffer *buffer,
+				   struct ring_buffer_per_cpu *cpu_buffer,
 				   struct buffer_page **subbuf_ids)
+	__must_hold(&buffer->mutex)
 {
 	struct trace_buffer_meta *meta = cpu_buffer->meta_page;
 	unsigned long nr_subbufs = cpu_buffer->nr_pages + 1;
@@ -7651,7 +7695,7 @@ static void rb_setup_ids_meta_page(struct ring_buffer_per_cpu *cpu_buffer,
 
 	meta->meta_struct_len = sizeof(*meta);
 	meta->nr_subbufs = nr_subbufs;
-	meta->subbuf_size = rb_subbuf_size(cpu_buffer->buffer);
+	meta->subbuf_size = rb_subbuf_size(buffer);
 	meta->meta_page_size = meta->subbuf_size;
 
 	rb_update_meta_page(cpu_buffer);
@@ -7715,7 +7759,8 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
 	    !(vma->vm_flags & VM_MAYSHARE))
 		return -EPERM;
 
-	subbuf_order = cpu_buffer->buffer->subbuf_order;
+	/* We either have the buffer mutex or user_mapped is set */
+	subbuf_order = context_unsafe(cpu_buffer->buffer->subbuf_order);
 	subbuf_pages = 1 << subbuf_order;
 
 	if (subbuf_order && pgoff % subbuf_pages)
@@ -7848,7 +7893,7 @@ int ring_buffer_map(struct trace_buffer *buffer, int cpu,
 	 * assigned.
 	 */
 	raw_spin_lock_irqsave(&cpu_buffer->reader_lock, flags);
-	rb_setup_ids_meta_page(cpu_buffer, subbuf_ids);
+	rb_setup_ids_meta_page(buffer, cpu_buffer, subbuf_ids);
 
 	raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
 
@@ -8018,7 +8063,7 @@ int ring_buffer_map_get_reader(struct trace_buffer *buffer, int cpu)
 out:
 	/* Some archs do not have data cache coherency between kernel and user-space */
 	flush_kernel_vmap_range(cpu_buffer->reader_page->page,
-				rb_subbuf_size(buffer));
+				PAGE_SIZE << cpu_buffer->reader_page->order);
 
 	rb_update_meta_page(cpu_buffer);
 
-- 
2.56.0.567.gdb80ae8cd5-goog

Re: [PATCH v1 1/9] ring-buffer: Always read buffer->buffers after testing the cpumask

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] ring-buffer: atomic counter underflow in ring_buffer_reset()
  [1]
--

commit fbf1784e6b7f3202f4d6898704fb8180140b6da5
Author: Vincent Donnefort <vdonnefort@google.com>

ring-buffer: Always read buffer->buffers after testing the cpumask

This commit moves the load of buffer->buffers[cpu] after the
cpumask_test_cpu() check in ring_buffer_peek(), ring_buffer_reset_cpu(),
and ring_buffer_read_page() to avoid reading a stale per-CPU buffer
pointer when racing with trace_rb_cpu_prepare().

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 1fd28a72e6ee1..bd8126f6353b7 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c

[ ... ]

> @@ -6740,11 +6742,13 @@ static void reset_disabled_cpu_buffer(struct ring_buffer_per_cpu *cpu_buffer)
>   */
>  void ring_buffer_reset_cpu(struct trace_buffer *buffer, int cpu)
>  {
> -	struct ring_buffer_per_cpu *cpu_buffer = buffer->buffers[cpu];
> +	struct ring_buffer_per_cpu *cpu_buffer;
>  
>  	if (!cpumask_test_cpu(cpu, buffer->cpumask))
>  		return;
>  
> +	cpu_buffer = buffer->buffers[cpu];
> +
>  	/* prevent another thread from changing buffer sizes */
>  	mutex_lock(&buffer->mutex);

[Severity: High]
This problem wasn't introduced by this patch, but can a concurrent
trace_rb_cpu_prepare() race with ring_buffer_reset() [1] and
underflow cpu_buffer->record_disabled and cpu_buffer->resize_disabled?

Unlike ring_buffer_reset_online_cpus(), which marks each disabled
cpu_buffer with RESET_BIT before sleeping in synchronize_rcu(),
ring_buffer_reset() iterates over buffer->cpumask twice without tracking
which CPUs were disabled in the first loop or holding cpus_read_lock():

CPU0                                  CPU1
ring_buffer_reset()
  for_each_buffer_cpu(buffer, cpu)
    atomic_inc(&cpu_buffer->resize_disabled);
    atomic_inc(&cpu_buffer->record_disabled);
  synchronize_rcu();
                                      trace_rb_cpu_prepare()
                                        rb_allocate_cpu_buffer()
                                        cpumask_set_cpu(cpu, buffer->cpumask);
  for_each_buffer_cpu(buffer, cpu)
    reset_disabled_cpu_buffer(cpu_buffer);
    atomic_dec(&cpu_buffer->record_disabled);  <-- underflows to -1
    atomic_dec(&cpu_buffer->resize_disabled);  <-- underflows to -1

For the newly prepared CPU, reset_disabled_cpu_buffer() runs while
recording is still enabled and both atomic counters underflow from 0 to
-1, corrupting the per-CPU ring buffer state.

[1] https://sashiko.dev/#/bug/linux-2557ac4d-25af-4832-a2c9-c12af32c7a04

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112141440.3050124-1-vdonnefort@google.com?part=1

Re: [PATCH v1 4/9] tracing: Cache ring_buffer_max_event_size() for trace_marker

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] tracing: inaccurate Fixes trailer in write_marker_to_buffer()
  commit
--

commit fc0aa729f88ae3461e4964c8c56c6c6eee40f687
Author: Vincent Donnefort <vdonnefort@google.com>

> tracing: Cache ring_buffer_max_event_size() for trace_marker
>
> write_marker_to_buffer() calls ring_buffer_max_event_size() twice
> without synchronisation against ring_buffer_subbuf_order_set(). If
> subbuf_order grows between the two calls, cnt may expand past
> TRACE_MARKER_MAX_SIZE. Read the maximum event size once so cnt can only
> shrink, and drop the now-unreachable WARN_ON_ONCE().
>
> Fixes: 2808e31ec12e ("ring-buffer: Add interface for configuring trace sub buffer size")
> Signed-off-by: Vincent Donnefort <vdonnefort@google.com>

[Severity: Low]
In 2808e31ec12e ("ring-buffer: Add interface for configuring trace sub
buffer size"), ring_buffer_max_event_size() still read
buffer->max_data_size, which was initialized once at buffer allocation
and not updated by ring_buffer_subbuf_order_set().  It only started
computing the size dynamically from buffer->subbuf_order in 472ac478b14d
("ring-buffer: Dynamically calculate max_data_size").  Should the Fixes:
tag be:

  Fixes: 472ac478b14d ("ring-buffer: Dynamically calculate max_data_size")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112141440.3050124-1-vdonnefort@google.com?part=4

Re: [PATCH v1 8/9] ring-buffer: Enable context analysis

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] ring-buffer: overwritten lost events count in
  __rb_get_reader_page() [1]
--

commit 07222abe396def21c907e79ee32f4784cd985857
Author: Vincent Donnefort <vdonnefort@google.com>

ring-buffer: Enable context analysis

This commit enables compiler context analysis for kernel/trace/ring_buffer.c,
adding lock annotations and inlining rb_get_mapped_buffer() and
rb_put_mapped_buffer() into ring_buffer_map_get_reader().

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 82f6e3df46276..99ade8b41b536 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c

[ ... ]

> @@ -8031,7 +8023,7 @@ int ring_buffer_map_get_reader(struct trace_buffer *buffer, int cpu)
>  	rb_update_meta_page(cpu_buffer);
>  
>  	raw_spin_unlock_irqrestore(&cpu_buffer->reader_lock, flags);
> -	rb_put_mapped_buffer(cpu_buffer);
> +	mutex_unlock(&cpu_buffer->mapping_lock);
>  
>  	return 0;
>  }

[Severity: Medium]
This problem wasn't introduced by this patch, but can __rb_get_reader_page()
overwrite an unconsumed cpu_buffer->lost_events count [1]?

When __rb_get_reader_page() swaps in a new reader page after an overrun, it
assigns the delta directly to cpu_buffer->lost_events and loops back to
"again:":

kernel/trace/ring_buffer.c:__rb_get_reader_page() {
	...
	if (overwrite != cpu_buffer->last_overrun) {
		cpu_buffer->lost_events = overwrite - cpu_buffer->last_overrun;
		cpu_buffer->last_overrun = overwrite;
	}

	goto again;
}

If the swapped page is empty (or the reader catches up to commit_page on the
next iteration), __rb_get_reader_page() returns NULL with
cpu_buffer->lost_events still non-zero. Callers such as
ring_buffer_map_get_reader() and ring_buffer_read_page() then jump to their
"out:" paths without consuming or clearing cpu_buffer->lost_events. If
another overrun happens before the next page swap, the direct assignment in
__rb_get_reader_page() overwrites the earlier unconsumed count instead of
accumulating it with +=, under-reporting dropped events to trace readers and
user-space meta-page mappings.

[1] https://sashiko.dev/#/bug/linux-6817998d-f7d9-427c-82e8-fac5ea6dfa8c

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112141440.3050124-1-vdonnefort@google.com?part=8