[PATCH v7 0/3] ring-buffer: Fixes for subbuf resizing and persistent buffers

[PATCH v7 0/3] ring-buffer: Fixes for subbuf resizing and persistent buffers

[Vincent Donnefort]

This series addresses multiple issues discovered with the dynamic ring
buffer resizing.

Changelog:

v7:

  - Match the "static" rb limit with bpage::id bitwidth
  - Cover another 32-bit truncation in rb_range_buffer (Sashiko)
  - Fix uninitialized spare_size (Sashiko)

v6 (https://lore.kernel.org/all/20260814154823.755406-1-vdonnefort@google.com/):

  - New prototype for ring_buffer_alloc_read_page() (Steven)
  - ring_buffer_read_page() to return -EAGAIN (Steven)
  - Keep nr_pages "unsigned long" (Steven)
  - Repase on ring-buffer/next (Drop most of the patches)

v5 (https://lore.kernel.org/all/20260813131152.3589632-1-vdonnefort@google.com/):

  - Reset info->spare_read only when data is in the ring-buffer (Sashiko)
  - Use `unsigned long` for subbuf_size declaration to avoid 32-bit
    truncation. (Sashiko)
  - Make cpu_buffer::free_page a buffer_read_data_page
  - Update kerneldoc for ring_buffer_alloc_read_page()

v4 (https://lore.kernel.org/all/20260812153311.2328812-1-vdonnefort@google.com/):

  - Add rb_subbuf_start() helper (Steven)
  - kerneldoc additions
  - Fix races in trace_pipe_raw readers
  - Use rb_subbuf_capacity() in ring_buffer_subbuf_order_set()
  - use rb_page_capacity() in ring_buffer_map_get_reader (Sashiko)
  - Fix 32-bit overflow in ring_buffer_subbuf_order_set() (Sashiko)
  - Hold cpu_buffer::lock when modifying cpu_buffer->free_page in
    ring_buffer_subbuf_order_set (Sashiko)

v3 (https://lore.kernel.org/all/20260810125633.3344684-1-vdonnefort@google.com/):

  - Drop first 3 patches (Rebased on 7.2-rc7)
  - Add a patch to align "nr_pages" to unsigned int
  - Add a patch to remove useless trace_buffer::cpus
  - Add unsigned long cast for rb_subbuf_size()
  - subbuf_order fix for rb_free_cpu_buffer() (Sashiko)
  - Use __always_inline just like the other accessors for the hot-path.

v2 (https://lore.kernel.org/all/20260806211306.3704194-1-vdonnefort@google.com/):

  - Prevent resizing of the persistent ring buffer
  - Add missing bpage::order init
  - Rework subbuf_size/subbuf_order (Sashiko)
  - Remove ring_buffer_per_cpu::mapped
  - Dynamically calculate trace_buffer::max_data_size

v1 (https://lore.kernel.org/all/20260805153225.2096152-1-vdonnefort@google.com/)

Vincent Donnefort (3):
  tracing: Fix subbuf resize races with trace_pipe_raw readers
  ring-buffer: Cap static ring buffer nr_pages
  ring-buffer: Prevent truncation of nr_pages / nr_subbufs

 include/linux/ring_buffer.h          |   5 +-
 kernel/trace/ring_buffer.c           | 182 ++++++++++++++++++---------
 kernel/trace/ring_buffer_benchmark.c |   6 +-
 kernel/trace/trace.c                 | 101 +++++++--------
 kernel/trace/trace.h                 |   9 +-
 5 files changed, 181 insertions(+), 122 deletions(-)


base-commit: 8b502bf6eb3da15f4b954ad3632335ff10ed746a
-- 
2.55.0.691.gc56d675ccc-goog

[PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers

[Vincent Donnefort]

Concurrent subbuffer resizes may crash trace_pipe_raw readers or leak
uninitialized memory to userspace due to stale size values.

Modify ring_buffer_alloc_read_page() to let it handle the resizing of a
previous buffer_data_read_page if necessary and add a new
ring_buffer_read_page_size() which enables ring-buffer users to avoid
using the racy ring_buffer_subbuf_size_get(). This makes the spare_size
member of ftrace_buffer_info redundant.

Safely handle cases in both readers where the subbuf order is modified
mid-read.

Fixes: bce761d75745 ("ring-buffer: Read and write to ring buffers with custom sub buffer size")
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>

diff --git a/include/linux/ring_buffer.h b/include/linux/ring_buffer.h
index 0670742b2d60..afc7daa6ee7d 100644
--- a/include/linux/ring_buffer.h
+++ b/include/linux/ring_buffer.h
@@ -218,14 +218,15 @@ bool ring_buffer_time_stamp_abs(struct trace_buffer *buffer);
 size_t ring_buffer_nr_dirty_pages(struct trace_buffer *buffer, int cpu);
 
 struct buffer_data_read_page;
-struct buffer_data_read_page *
-ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu);
+int ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu,
+				struct buffer_data_read_page **rpage);
 void ring_buffer_free_read_page(struct trace_buffer *buffer, int cpu,
 				struct buffer_data_read_page *page);
 int ring_buffer_read_page(struct trace_buffer *buffer,
 			  struct buffer_data_read_page *data_page,
 			  size_t len, int cpu, int full);
 void *ring_buffer_read_page_data(struct buffer_data_read_page *page);
+unsigned int ring_buffer_read_page_size(struct buffer_data_read_page *rpage);
 
 struct trace_seq;
 
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 5fc009edc1ec..ec13779922ff 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -6990,56 +6990,78 @@ EXPORT_SYMBOL_GPL(ring_buffer_swap_cpu);
  * ring_buffer_alloc_read_page - allocate a page to read from buffer
  * @buffer: the buffer to allocate for.
  * @cpu: the cpu buffer to allocate.
+ * @rpage: pointer to pass in an already allocated page (can be NULL)
+ *         and returns the allocated page.
  *
- * This function is used in conjunction with ring_buffer_read_page.
+ * This function is used in conjunction with ring_buffer_read_page().
  * When reading a full page from the ring buffer, these functions
  * can be used to speed up the process. The calling function should
  * allocate a few pages first with this function. Then when it
  * needs to get pages from the ring buffer, it passes the result
- * of this function into ring_buffer_read_page, which will swap
+ * of this function into ring_buffer_read_page(), which will swap
  * the page that was allocated, with the read page of the buffer.
  *
+ * 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.
+ *
  * Returns:
- *  The page allocated, or ERR_PTR
+ *  0 on success, < 0 on error
  */
-struct buffer_data_read_page *
-ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu)
+int ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu,
+				struct buffer_data_read_page **rpage)
 {
 	struct ring_buffer_per_cpu *cpu_buffer;
-	struct buffer_data_read_page *bpage = NULL;
 	unsigned long flags;
+	unsigned int order;
 
 	if (!cpumask_test_cpu(cpu, buffer->cpumask))
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 
-	bpage = kzalloc_obj(*bpage);
-	if (!bpage)
-		return ERR_PTR(-ENOMEM);
+	if (!rpage)
+		return -EINVAL;
 
-	bpage->order = buffer->subbuf_order;
+	order = buffer->subbuf_order;
+
+	if (*rpage) {
+		if ((*rpage)->order == order)
+			return 0;
+
+		/* We can reuse rpage, but we discard the payload */
+		free_pages((unsigned long)(*rpage)->data, (*rpage)->order);
+		(*rpage)->data = NULL;
+	} else {
+		*rpage = kzalloc_obj(**rpage);
+		if (!*rpage)
+			return -ENOMEM;
+	}
+
+	(*rpage)->order = order;
 	cpu_buffer = buffer->buffers[cpu];
+
 	local_irq_save(flags);
 	arch_spin_lock(&cpu_buffer->lock);
 
 	if (cpu_buffer->free_page.data) {
-		*bpage = cpu_buffer->free_page;
+		**rpage = cpu_buffer->free_page;
 		cpu_buffer->free_page.data = NULL;
 	}
 
 	arch_spin_unlock(&cpu_buffer->lock);
 	local_irq_restore(flags);
 
-	if (bpage->data) {
-		rb_init_data_page(bpage->data);
+	if ((*rpage)->data) {
+		rb_init_data_page((*rpage)->data);
 	} else {
-		bpage->data = alloc_cpu_data(cpu, bpage->order);
-		if (!bpage->data) {
-			kfree(bpage);
-			return ERR_PTR(-ENOMEM);
+		(*rpage)->data = alloc_cpu_data(cpu, (*rpage)->order);
+		if (!(*rpage)->data) {
+			kfree(*rpage);
+			*rpage = NULL;
+			return -ENOMEM;
 		}
 	}
 
-	return bpage;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_alloc_read_page);
 
@@ -7047,21 +7069,30 @@ EXPORT_SYMBOL_GPL(ring_buffer_alloc_read_page);
  * ring_buffer_free_read_page - free an allocated read page
  * @buffer: the buffer the page was allocate for
  * @cpu: the cpu buffer the page came from
- * @data_page: the page to free
+ * @rpage: the buffer_dat_read_page to free
  *
  * Free a page allocated from ring_buffer_alloc_read_page.
  */
 void ring_buffer_free_read_page(struct trace_buffer *buffer, int cpu,
-				struct buffer_data_read_page *data_page)
+				struct buffer_data_read_page *rpage)
 {
 	struct ring_buffer_per_cpu *cpu_buffer;
-	struct buffer_data_page *dpage = data_page->data;
-	struct page *page = virt_to_page(dpage);
+	struct buffer_data_page *dpage;
 	unsigned long flags;
+	struct page *page;
 
 	if (!buffer || !buffer->buffers || !buffer->buffers[cpu])
 		return;
 
+	if (!rpage)
+		return;
+
+	dpage = rpage->data;
+	if (!dpage)
+		goto out;
+
+	page = virt_to_page(dpage);
+
 	cpu_buffer = buffer->buffers[cpu];
 
 	/*
@@ -7069,14 +7100,14 @@ 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 || data_page->order != buffer->subbuf_order)
+	if (page_ref_count(page) > 1 || rpage->order != buffer->subbuf_order)
 		goto out;
 
 	local_irq_save(flags);
 	arch_spin_lock(&cpu_buffer->lock);
 
 	if (!cpu_buffer->free_page.data) {
-		cpu_buffer->free_page = *data_page;
+		cpu_buffer->free_page = *rpage;
 		dpage = NULL;
 	}
 
@@ -7084,8 +7115,8 @@ void ring_buffer_free_read_page(struct trace_buffer *buffer, int cpu,
 	local_irq_restore(flags);
 
  out:
-	free_pages((unsigned long)dpage, data_page->order);
-	kfree(data_page);
+	free_pages((unsigned long)dpage, rpage->order);
+	kfree(rpage);
 }
 EXPORT_SYMBOL_GPL(ring_buffer_free_read_page);
 
@@ -7122,6 +7153,7 @@ EXPORT_SYMBOL_GPL(ring_buffer_free_read_page);
  * Returns:
  *  >=0 if data has been transferred, returns the offset of consumed data.
  *  <0 if no data has been transferred.
+ *  -EAGAIN if the subbuf size has changed and @data_page must be reallocated.
  */
 int ring_buffer_read_page(struct trace_buffer *buffer,
 			  struct buffer_data_read_page *data_page,
@@ -7159,7 +7191,7 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
 	guard(raw_spinlock_irqsave)(&cpu_buffer->reader_lock);
 
 	if (data_page->order != cpu_buffer->reader_page->order)
-		return -1;
+		return -EAGAIN;
 
 	reader = rb_get_reader_page(cpu_buffer);
 	if (!reader)
@@ -7323,6 +7355,18 @@ void *ring_buffer_read_page_data(struct buffer_data_read_page *page)
 }
 EXPORT_SYMBOL_GPL(ring_buffer_read_page_data);
 
+/**
+ * ring_buffer_read_page_size - get size of the read page.
+ * @page:  the page to get the size from
+ *
+ * Returns size of the page in bytes.
+ */
+unsigned int ring_buffer_read_page_size(struct buffer_data_read_page *rpage)
+{
+	return PAGE_SIZE << rpage->order;
+}
+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
diff --git a/kernel/trace/ring_buffer_benchmark.c b/kernel/trace/ring_buffer_benchmark.c
index 593e3b59e42e..c3d34c0e64e2 100644
--- a/kernel/trace/ring_buffer_benchmark.c
+++ b/kernel/trace/ring_buffer_benchmark.c
@@ -104,7 +104,7 @@ static enum event_status read_event(int cpu)
 
 static enum event_status read_page(int cpu)
 {
-	struct buffer_data_read_page *bpage;
+	struct buffer_data_read_page *bpage = NULL;
 	struct ring_buffer_event *event;
 	struct rb_page *rpage;
 	unsigned long commit;
@@ -114,8 +114,8 @@ static enum event_status read_page(int cpu)
 	int inc;
 	int i;
 
-	bpage = ring_buffer_alloc_read_page(buffer, cpu);
-	if (IS_ERR(bpage))
+	ret = ring_buffer_alloc_read_page(buffer, cpu, &bpage);
+	if (ret < 0)
 		return EVENT_DROPPED;
 
 	page_size = ring_buffer_subbuf_size_get(buffer);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 395238b2b715..737922b236d4 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -7080,8 +7080,8 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
 {
 	struct ftrace_buffer_info *info = filp->private_data;
 	struct trace_iterator *iter = &info->iter;
+	unsigned int spare_size;
 	void *trace_data;
-	int page_size;
 	ssize_t ret = 0;
 	ssize_t size;
 
@@ -7091,36 +7091,24 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
 	if (iter->snapshot && tracer_uses_snapshot(iter->tr->current_trace))
 		return -EBUSY;
 
-	page_size = ring_buffer_subbuf_size_get(iter->array_buffer->buffer);
-
-	/* Make sure the spare matches the current sub buffer size */
+again:
 	if (info->spare) {
-		if (page_size != info->spare_size) {
-			ring_buffer_free_read_page(iter->array_buffer->buffer,
-						   info->spare_cpu, info->spare);
-			info->spare = NULL;
-		}
+		spare_size = ring_buffer_read_page_size(info->spare);
+		/* Do we have previous read data to read? */
+		if (info->read < spare_size)
+			goto read;
 	}
 
-	if (!info->spare) {
-		info->spare = ring_buffer_alloc_read_page(iter->array_buffer->buffer,
-							  iter->cpu_file);
-		if (IS_ERR(info->spare)) {
-			ret = PTR_ERR(info->spare);
-			info->spare = NULL;
-		} else {
-			info->spare_cpu = iter->cpu_file;
-			info->spare_size = page_size;
-		}
-	}
-	if (!info->spare)
+	/* Make sure the read page order is aligned with the current subbuf order */
+	ret = ring_buffer_alloc_read_page(iter->array_buffer->buffer, iter->cpu_file,
+					  &info->spare);
+	if (ret)
 		return ret;
 
-	/* Do we have previous read data to read? */
-	if (info->read < page_size)
-		goto read;
+	spare_size = ring_buffer_read_page_size(info->spare);
+	info->read = spare_size;
+	info->spare_cpu = iter->cpu_file;
 
- again:
 	trace_access_lock(iter->cpu_file);
 	ret = ring_buffer_read_page(iter->array_buffer->buffer,
 				    info->spare,
@@ -7128,7 +7116,9 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
 				    iter->cpu_file, 0);
 	trace_access_unlock(iter->cpu_file);
 
-	if (ret < 0) {
+	if (ret == -EAGAIN) {
+		goto again;
+	} else if (ret < 0) {
 		if (trace_empty(iter) && !iter->closed) {
 			if (update_last_data_if_empty(iter->tr))
 				return 0;
@@ -7146,8 +7136,9 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
 	}
 
 	info->read = 0;
+
  read:
-	size = page_size - info->read;
+	size = spare_size - info->read;
 	if (size > count)
 		size = count;
 	trace_data = ring_buffer_read_page_data(info->spare);
@@ -7197,17 +7188,17 @@ int tracing_buffers_release(struct inode *inode, struct file *file)
 }
 
 struct buffer_ref {
-	struct trace_buffer	*buffer;
-	void			*page;
-	int			cpu;
-	refcount_t		refcount;
+	struct trace_buffer		*buffer;
+	struct buffer_data_read_page	*rpage;
+	int				cpu;
+	refcount_t			refcount;
 };
 
 static void buffer_ref_release(struct buffer_ref *ref)
 {
 	if (!refcount_dec_and_test(&ref->refcount))
 		return;
-	ring_buffer_free_read_page(ref->buffer, ref->cpu, ref->page);
+	ring_buffer_free_read_page(ref->buffer, ref->cpu, ref->rpage);
 	kfree(ref);
 }
 
@@ -7268,23 +7259,12 @@ ssize_t tracing_buffers_splice_read(struct file *file, loff_t *ppos,
 	};
 	struct buffer_ref *ref;
 	bool woken = false;
-	int page_size;
 	int entries, i;
 	ssize_t ret = 0;
 
 	if (iter->snapshot && tracer_uses_snapshot(iter->tr->current_trace))
 		return -EBUSY;
 
-	page_size = ring_buffer_subbuf_size_get(iter->array_buffer->buffer);
-	if (*ppos & (page_size - 1))
-		return -EINVAL;
-
-	if (len & (page_size - 1)) {
-		if (len < page_size)
-			return -EINVAL;
-		len &= (~(page_size - 1));
-	}
-
 	if (splice_grow_spd(pipe, &spd))
 		return -ENOMEM;
 
@@ -7292,7 +7272,8 @@ ssize_t tracing_buffers_splice_read(struct file *file, loff_t *ppos,
 	trace_access_lock(iter->cpu_file);
 	entries = ring_buffer_entries_cpu(iter->array_buffer->buffer, iter->cpu_file);
 
-	for (i = 0; i < spd.nr_pages_max && len && entries; i++, len -= page_size) {
+	for (i = 0; i < spd.nr_pages_max && len && entries; i++) {
+		unsigned int page_size;
 		struct page *page;
 		int r;
 
@@ -7304,25 +7285,38 @@ ssize_t tracing_buffers_splice_read(struct file *file, loff_t *ppos,
 
 		refcount_set(&ref->refcount, 1);
 		ref->buffer = iter->array_buffer->buffer;
-		ref->page = ring_buffer_alloc_read_page(ref->buffer, iter->cpu_file);
-		if (IS_ERR(ref->page)) {
-			ret = PTR_ERR(ref->page);
-			ref->page = NULL;
+
+new_read_page:
+		ret = ring_buffer_alloc_read_page(ref->buffer, iter->cpu_file, &ref->rpage);
+		if (ret) {
 			kfree(ref);
 			break;
 		}
 		ref->cpu = iter->cpu_file;
 
-		r = ring_buffer_read_page(ref->buffer, ref->page,
-					  len, iter->cpu_file, 1);
+		page_size = ring_buffer_read_page_size(ref->rpage);
+
+		r = -EINVAL;
+		if (IS_ALIGNED(*ppos, page_size) && len >= page_size) {
+			r = ring_buffer_read_page(ref->buffer, ref->rpage, len, iter->cpu_file, 1);
+			if (r == -EAGAIN)
+				goto new_read_page;
+		} else if (!i) {
+			/*
+			 * If the first iteration fails this is an invalid userspace input.
+			 * Otherwise, this is because the subbuf order has been modified. Do not
+			 * report an error and finish the read.
+			 */
+			ret = -EINVAL;
+		}
+
 		if (r < 0) {
-			ring_buffer_free_read_page(ref->buffer, ref->cpu,
-						   ref->page);
+			ring_buffer_free_read_page(ref->buffer, ref->cpu, ref->rpage);
 			kfree(ref);
 			break;
 		}
 
-		page = virt_to_page(ring_buffer_read_page_data(ref->page));
+		page = virt_to_page(ring_buffer_read_page_data(ref->rpage));
 
 		spd.pages[i] = page;
 		spd.partial[i].len = page_size;
@@ -7330,6 +7324,7 @@ ssize_t tracing_buffers_splice_read(struct file *file, loff_t *ppos,
 		spd.partial[i].private = (unsigned long)ref;
 		spd.nr_pages++;
 		*ppos += page_size;
+		len -= page_size;
 
 		entries = ring_buffer_entries_cpu(iter->array_buffer->buffer, iter->cpu_file);
 	}
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index bf77331f56a4..cbf54eded447 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -745,11 +745,10 @@ static inline int tracing_get_cpu(struct inode *inode)
 void tracing_reset_cpu(struct array_buffer *buf, int cpu);
 
 struct ftrace_buffer_info {
-	struct trace_iterator	iter;
-	void			*spare;
-	unsigned int		spare_cpu;
-	unsigned int		spare_size;
-	unsigned int		read;
+	struct trace_iterator		iter;
+	struct buffer_data_read_page	*spare;
+	unsigned int			spare_cpu;
+	unsigned int			read;
 };
 
 /**
-- 
2.55.0.691.gc56d675ccc-goog

Re: [PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Infinite busy-wait loop in tracing reads (tracing_buffers_read and tracing_buffers_splice_read) causing lockups during concurrent ring buffer resizes.

Pre-existing issues:
- [Critical] Concurrent io_uring reads trigger a double free on info->spare->data due to missing VFS locks.
- [High] Use-After-Free vulnerability in VFS pipe buffer destruction path due to lifecycle mismatch with the trace array.
--

--- Patch [1]: [PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers ---
commit 3b5685c224f6977e19f6580474cf69ff34b547a9
Author: Vincent Donnefort <vdonnefort@google.com>

tracing: Fix subbuf resize races with trace_pipe_raw readers

Concurrent subbuffer resizes may crash trace_pipe_raw readers or leak
uninitialized memory to userspace due to stale size values.
Modify ring_buffer_alloc_read_page() to handle resizing and add
ring_buffer_read_page_size() to avoid racy subbuf size gets.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 5fc009edc1ec9..ec13779922ffb 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
[ ... ]
> @@ -6990,56 +6990,78 @@ EXPORT_SYMBOL_GPL(ring_buffer_swap_cpu);
> +int ring_buffer_alloc_read_page(struct trace_buffer *buffer, int cpu,
> +				struct buffer_data_read_page **rpage)
>  {
[ ... ]
> +	if (*rpage) {
> +		if ((*rpage)->order == order)
> +			return 0;
>  
> -	bpage->order = buffer->subbuf_order;
> +		/* We can reuse rpage, but we discard the payload */
> +		free_pages((unsigned long)(*rpage)->data, (*rpage)->order);
> +		(*rpage)->data = NULL;

[Severity: Critical]
This is a pre-existing issue, but concurrent io_uring reads can call vfs_read()
without holding f_pos_lock, leading to concurrent execution of
tracing_buffers_read().

Neither tracing_buffers_read() nor this function protects the shared
info->spare pointer with a lock.

If these threads race during a subbuffer resize, can both observe the order
mismatch simultaneously and execute free_pages() on the same data address,
causing a double free?

> +	} else {
> +		*rpage = kzalloc_obj(**rpage);
> +		if (!*rpage)
> +			return -ENOMEM;
> +	}
> +
> +	(*rpage)->order = order;
>  	cpu_buffer = buffer->buffers[cpu];
[ ... ]
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 395238b2b715c..737922b236d46 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
[ ... ]
> @@ -7128,7 +7116,9 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
>  				    iter->cpu_file, 0);
>  	trace_access_unlock(iter->cpu_file);
>  
> -	if (ret < 0) {
> +	if (ret == -EAGAIN) {
> +		goto again;
> +	} else if (ret < 0) {

[Severity: High]
If ring_buffer_read_page() returns -EAGAIN because the subbuffer order was
updated but cpu_buffer->reader_page->order hasn't been updated yet, this
branches back to the again label.

Since the loop does not yield the CPU or call cond_resched(), could this cause
a hard lockup if the reader and the resize thread are on the same CPU?

>  		if (trace_empty(iter) && !iter->closed) {
>  			if (update_last_data_if_empty(iter->tr))
>  				return 0;
[ ... ]
> @@ -7268,23 +7259,12 @@ ssize_t tracing_buffers_splice_read(struct file *file, loff_t *ppos,
>  	};
>  	struct buffer_ref *ref;
>  	bool woken = false;
> -	int page_size;
>  	int entries, i;
>  	ssize_t ret = 0;
[ ... ]
>  	for (i = 0; i < spd.nr_pages_max && len && entries; i++) {
> +		unsigned int page_size;
>  		struct page *page;
>  		int r;
>  
> @@ -7304,25 +7285,38 @@ ssize_t tracing_buffers_splice_read(struct file *file, loff_t *ppos,
>  
>  		refcount_set(&ref->refcount, 1);
>  		ref->buffer = iter->array_buffer->buffer;

[Severity: High]
This is a pre-existing issue, but splice_to_pipe() links kernel pages to a
pipe buffer and stashes ref->buffer as private data.

The VFS operation never acquires a reference on the trace_array. If the user
closes the trace file and deletes the trace instance, the trace_buffer is
freed.

When the pipe buffer is subsequently read or destroyed, won't
buffer_ref_release() dereference the dangling ref->buffer pointer when calling
ring_buffer_free_read_page(), causing a use-after-free?

> -		ref->page = ring_buffer_alloc_read_page(ref->buffer, iter->cpu_file);
> -		if (IS_ERR(ref->page)) {
> -			ret = PTR_ERR(ref->page);
> -			ref->page = NULL;
> +
> +new_read_page:
> +		ret = ring_buffer_alloc_read_page(ref->buffer, iter->cpu_file, &ref->rpage);
> +		if (ret) {
>  			kfree(ref);
>  			break;
>  		}
>  		ref->cpu = iter->cpu_file;
>  
> -		r = ring_buffer_read_page(ref->buffer, ref->page,
> -					  len, iter->cpu_file, 1);
> +		page_size = ring_buffer_read_page_size(ref->rpage);
> +
> +		r = -EINVAL;
> +		if (IS_ALIGNED(*ppos, page_size) && len >= page_size) {
> +			r = ring_buffer_read_page(ref->buffer, ref->rpage, len, iter->cpu_file, 1);
> +			if (r == -EAGAIN)
> +				goto new_read_page;

[Severity: High]
Similar to the read path above, this catches -EAGAIN and loops back to
new_read_page.

Because goto new_read_page executes continuously without releasing
trace_access_lock() or yielding the CPU, can this permanently deadlock the
system during a resize by preventing the resize thread from acquiring the
necessary locks?

> +		} else if (!i) {
> +			/*
> +			 * If the first iteration fails this is an invalid userspace input.
> +			 * Otherwise, this is because the subbuf order has been modified. Do not
> +			 * report an error and finish the read.
> +			 */
> +			ret = -EINVAL;
> +		}

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

Re: [PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers

[Steven Rostedt]

On Mon, 17 Aug 2026 14:08:11 +0000
sashiko-bot@kernel.org wrote:

> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 395238b2b715c..737922b236d46 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c  
> [ ... ]
> > @@ -7128,7 +7116,9 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
> >  				    iter->cpu_file, 0);
> >  	trace_access_unlock(iter->cpu_file);
> >  
> > -	if (ret < 0) {
> > +	if (ret == -EAGAIN) {
> > +		goto again;
> > +	} else if (ret < 0) {  
> 
> [Severity: High]
> If ring_buffer_read_page() returns -EAGAIN because the subbuffer order was
> updated but cpu_buffer->reader_page->order hasn't been updated yet, this
> branches back to the again label.
> 
> Since the loop does not yield the CPU or call cond_resched(), could this cause
> a hard lockup if the reader and the resize thread are on the same CPU?

Hmm, this looks legit. We should at least have a way to break out if it
loops too many times.

> 
> >  		if (trace_empty(iter) && !iter->closed) {
> >  			if (update_last_data_if_empty(iter->tr))
> >  				return 0;  
> [ ... ]

> > -		ref->page = ring_buffer_alloc_read_page(ref->buffer, iter->cpu_file);
> > -		if (IS_ERR(ref->page)) {
> > -			ret = PTR_ERR(ref->page);
> > -			ref->page = NULL;
> > +
> > +new_read_page:
> > +		ret = ring_buffer_alloc_read_page(ref->buffer, iter->cpu_file, &ref->rpage);
> > +		if (ret) {
> >  			kfree(ref);
> >  			break;
> >  		}
> >  		ref->cpu = iter->cpu_file;
> >  
> > -		r = ring_buffer_read_page(ref->buffer, ref->page,
> > -					  len, iter->cpu_file, 1);
> > +		page_size = ring_buffer_read_page_size(ref->rpage);
> > +
> > +		r = -EINVAL;
> > +		if (IS_ALIGNED(*ppos, page_size) && len >= page_size) {
> > +			r = ring_buffer_read_page(ref->buffer, ref->rpage, len, iter->cpu_file, 1);
> > +			if (r == -EAGAIN)
> > +				goto new_read_page;  
> 
> [Severity: High]
> Similar to the read path above, this catches -EAGAIN and loops back to
> new_read_page.
> 
> Because goto new_read_page executes continuously without releasing
> trace_access_lock() or yielding the CPU, can this permanently deadlock the
> system during a resize by preventing the resize thread from acquiring the
> necessary locks?

This also looks legit.

-- Steve

> 
> > +		} else if (!i) {
> > +			/*
> > +			 * If the first iteration fails this is an invalid userspace input.
> > +			 * Otherwise, this is because the subbuf order has been modified. Do not
> > +			 * report an error and finish the read.
> > +			 */
> > +			ret = -EINVAL;
> > +		}  
> 

Re: [PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers

[Vincent Donnefort]

On Sat, Aug 22, 2026 at 11:04:24AM -0400, Steven Rostedt wrote:
> On Mon, 17 Aug 2026 14:08:11 +0000
> sashiko-bot@kernel.org wrote:
> 
> > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > > index 395238b2b715c..737922b236d46 100644
> > > --- a/kernel/trace/trace.c
> > > +++ b/kernel/trace/trace.c  
> > [ ... ]
> > > @@ -7128,7 +7116,9 @@ ssize_t tracing_buffers_read(struct file *filp, char __user *ubuf,
> > >  				    iter->cpu_file, 0);
> > >  	trace_access_unlock(iter->cpu_file);
> > >  
> > > -	if (ret < 0) {
> > > +	if (ret == -EAGAIN) {
> > > +		goto again;
> > > +	} else if (ret < 0) {  
> > 
> > [Severity: High]
> > If ring_buffer_read_page() returns -EAGAIN because the subbuffer order was
> > updated but cpu_buffer->reader_page->order hasn't been updated yet, this
> > branches back to the again label.
> > 
> > Since the loop does not yield the CPU or call cond_resched(), could this cause
> > a hard lockup if the reader and the resize thread are on the same CPU?
> 
> Hmm, this looks legit. We should at least have a way to break out if it
> loops too many times.

Can this happen only on !CONFIG_PREEMPT systems?

I can add a cond_resched() here though, I believe that would make sense: if an
order modification is pending, writing is disabled anyway and we are about to
loose the stored events.

Additionally I can reduce the window where buffer::subbuf_order differs from the
reader page order by adding an "order" argument to __rb_allocate_pages() and
only set buffer::subbuf_order later in ring_buffer_subbuf_order_set().

WDYS?

-- 
Vincent

> 
> > 
> > >  		if (trace_empty(iter) && !iter->closed) {
> > >  			if (update_last_data_if_empty(iter->tr))
> > >  				return 0;  
> > [ ... ]
> 
> > > -		ref->page = ring_buffer_alloc_read_page(ref->buffer, iter->cpu_file);
> > > -		if (IS_ERR(ref->page)) {
> > > -			ret = PTR_ERR(ref->page);
> > > -			ref->page = NULL;
> > > +
> > > +new_read_page:
> > > +		ret = ring_buffer_alloc_read_page(ref->buffer, iter->cpu_file, &ref->rpage);
> > > +		if (ret) {
> > >  			kfree(ref);
> > >  			break;
> > >  		}
> > >  		ref->cpu = iter->cpu_file;
> > >  
> > > -		r = ring_buffer_read_page(ref->buffer, ref->page,
> > > -					  len, iter->cpu_file, 1);
> > > +		page_size = ring_buffer_read_page_size(ref->rpage);
> > > +
> > > +		r = -EINVAL;
> > > +		if (IS_ALIGNED(*ppos, page_size) && len >= page_size) {
> > > +			r = ring_buffer_read_page(ref->buffer, ref->rpage, len, iter->cpu_file, 1);
> > > +			if (r == -EAGAIN)
> > > +				goto new_read_page;  
> > 
> > [Severity: High]
> > Similar to the read path above, this catches -EAGAIN and loops back to
> > new_read_page.
> > 
> > Because goto new_read_page executes continuously without releasing
> > trace_access_lock() or yielding the CPU, can this permanently deadlock the
> > system during a resize by preventing the resize thread from acquiring the
> > necessary locks?
> 
> This also looks legit.
> 
> -- Steve
> 
> > 
> > > +		} else if (!i) {
> > > +			/*
> > > +			 * If the first iteration fails this is an invalid userspace input.
> > > +			 * Otherwise, this is because the subbuf order has been modified. Do not
> > > +			 * report an error and finish the read.
> > > +			 */
> > > +			ret = -EINVAL;
> > > +		}  
> > 
> 

Re: [PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers

[Steven Rostedt]

On Sun, 23 Aug 2026 21:09:05 +0100
Vincent Donnefort <vdonnefort@google.com> wrote:

> > > [Severity: High]
> > > If ring_buffer_read_page() returns -EAGAIN because the subbuffer order was
> > > updated but cpu_buffer->reader_page->order hasn't been updated yet, this
> > > branches back to the again label.
> > > 
> > > Since the loop does not yield the CPU or call cond_resched(), could this cause
> > > a hard lockup if the reader and the resize thread are on the same CPU?  
> > 
> > Hmm, this looks legit. We should at least have a way to break out if it
> > loops too many times.  
> 
> Can this happen only on !CONFIG_PREEMPT systems?

It can happen with RT tasks as they don't yield. If this task is of higher
priority than the one resizing, then it will loop forever even with a
cond_resched().

> 
> I can add a cond_resched() here though, I believe that would make sense: if an
> order modification is pending, writing is disabled anyway and we are about to
> loose the stored events.

For RT tasks, cond_resched() wouldn't be enough.

> 
> Additionally I can reduce the window where buffer::subbuf_order differs from the
> reader page order by adding an "order" argument to __rb_allocate_pages() and
> only set buffer::subbuf_order later in ring_buffer_subbuf_order_set().

I'm wondering if we even care if data_page->order != cpu_buffer->reader_page->order
and should change it to being a force copy instead.

If the reader_page is bigger than the dpage, and only copy what the dpage
can hold. It would also update the read value of the reader_page which
would allow another copy to get the rest. If the data page is bigger, then
just copy everything.

It's not like this will happen often.

-- Steve

Re: [PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers

[Vincent Donnefort]

On Mon, Aug 24, 2026 at 10:53:15AM -0400, Steven Rostedt wrote:
> On Sun, 23 Aug 2026 21:09:05 +0100
> Vincent Donnefort <vdonnefort@google.com> wrote:
> 
> > > > [Severity: High]
> > > > If ring_buffer_read_page() returns -EAGAIN because the subbuffer order was
> > > > updated but cpu_buffer->reader_page->order hasn't been updated yet, this
> > > > branches back to the again label.
> > > > 
> > > > Since the loop does not yield the CPU or call cond_resched(), could this cause
> > > > a hard lockup if the reader and the resize thread are on the same CPU?  
> > > 
> > > Hmm, this looks legit. We should at least have a way to break out if it
> > > loops too many times.  
> > 
> > Can this happen only on !CONFIG_PREEMPT systems?
> 
> It can happen with RT tasks as they don't yield. If this task is of higher
> priority than the one resizing, then it will loop forever even with a
> cond_resched().

Ha yes of course!

> 
> > 
> > I can add a cond_resched() here though, I believe that would make sense: if an
> > order modification is pending, writing is disabled anyway and we are about to
> > loose the stored events.
> 
> For RT tasks, cond_resched() wouldn't be enough.
> 
> > 
> > Additionally I can reduce the window where buffer::subbuf_order differs from the
> > reader page order by adding an "order" argument to __rb_allocate_pages() and
> > only set buffer::subbuf_order later in ring_buffer_subbuf_order_set().
> 
> I'm wondering if we even care if data_page->order != cpu_buffer->reader_page->order
> and should change it to being a force copy instead.
> 
> If the reader_page is bigger than the dpage, and only copy what the dpage
> can hold. It would also update the read value of the reader_page which
> would allow another copy to get the rest. If the data page is bigger, then
> just copy everything.
> 
> It's not like this will happen often.
> 
> -- Steve


That sounds interesting... that would get rid of the -EAGAIN. Let me try.

Do you think it is still worth to reduce the window of discrepency between
buffer->subbuf_order and buffer_page->order?

-- 
Vincent

Re: [PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers

[Steven Rostedt]

On Tue, 25 Aug 2026 09:26:07 +0100
Vincent Donnefort <vdonnefort@google.com> wrote:

> 
> Do you think it is still worth to reduce the window of discrepency between
> buffer->subbuf_order and buffer_page->order?

Can you elaborate more on what you mean?

-- Steve

Re: [PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers

[Vincent Donnefort]

On Tue, Aug 25, 2026 at 08:47:07AM -0400, Steven Rostedt wrote:
> On Tue, 25 Aug 2026 09:26:07 +0100
> Vincent Donnefort <vdonnefort@google.com> wrote:
> 
> > 
> > Do you think it is still worth to reduce the window of discrepency between
> > buffer->subbuf_order and buffer_page->order?
> 
> Can you elaborate more on what you mean?
> 
> -- Steve

Something like this.

@@ -7469,8 +7473,6 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
        /* Make sure all commits have finished */
        synchronize_rcu();
 
-       buffer->subbuf_order = order;
-
        /* Make sure all new buffers are allocated, before deleting the old ones */
        for_each_buffer_cpu(buffer, cpu) {
 
@@ -7500,6 +7502,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,
+                                       order,
                                        &cpu_buffer->new_pages)) {
                        /* not enough memory for new pages */
                        err = -ENOMEM;
@@ -7507,6 +7510,8 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
                }
        }
 
+       buffer->subbuf_order = order;
+
        for_each_buffer_cpu(buffer, cpu) {
                struct buffer_data_read_page old_free_data_page;
                struct list_head old_pages;


But now with the idea of copying the data when data_page->order !=
reader_page->order, I am less convinced there's any gain.

-- 
Vincent

Re: [PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers

[Steven Rostedt]

On Tue, 25 Aug 2026 13:52:11 +0100
Vincent Donnefort <vdonnefort@google.com> wrote:

> But now with the idea of copying the data when data_page->order !=
> reader_page->order, I am less convinced there's any gain.

Yeah, let's leave this alone for now.

-- Steve

[PATCH v7 2/3] ring-buffer: Cap static ring buffer nr_pages

[Vincent Donnefort]

Static ring buffers (i.e. persistent, user-mapped and remote) rely on
the bpage::id field. The number of pages for those ring buffers must fit
into that variable. Enforce this limit on ring buffer creation or
user-mapping.

While at it, make buffer_page::id 31 bits. This does not change the
struct buffer_page size.

Fixes: be68d63a139b ("ring-buffer: Add ring_buffer_alloc_range()")
Signed-off-by: Vincent Donnefort <vdonnefort@google.com>

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index ec13779922ff..5e26eeff588f 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -345,7 +345,7 @@ struct buffer_page {
 	local_t		 entries;	/* entries on this page */
 	unsigned long	 real_end;	/* real end of data */
 	unsigned	 order;		/* order of the page */
-	u32		 id:30;		/* ID for external mapping */
+	u32		 id:31;		/* ID for external mapping */
 	u32		 range:1;	/* Mapped via a range */
 	struct buffer_data_page *page;	/* Actual data page */
 };
@@ -652,6 +652,15 @@ static bool rb_is_static(struct ring_buffer_per_cpu *cpu_buffer)
 	return cpu_buffer->user_mapped || cpu_buffer->remote || cpu_buffer->ring_meta;
 }
 
+static unsigned long rb_static_max_pages(void)
+{
+	/*
+	 * Static ring buffers are using bpage::id and must account for the
+	 * reader page.
+	 */
+	return (1UL << 31) - 1;
+}
+
 struct ring_buffer_iter {
 	struct ring_buffer_per_cpu	*cpu_buffer;
 	unsigned long			head;
@@ -2837,6 +2846,10 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
 		 */
 		nr_pages = (size - sizeof(struct ring_buffer_cpu_meta)) /
 			(subbuf_size + sizeof(int));
+
+		if (nr_pages > rb_static_max_pages())
+			goto fail_free_buffers;
+
 		/* Need at least two pages plus the reader page */
 		if (nr_pages < 3)
 			goto fail_free_buffers;
@@ -2869,6 +2882,10 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
 		/* The writer is remote. This ring-buffer is read-only */
 		atomic_inc(&buffer->record_disabled);
 		nr_pages = desc->nr_page_va - 1;
+
+		if (nr_pages > rb_static_max_pages())
+			goto fail_free_buffers;
+
 		if (nr_pages < 2)
 			goto fail_free_buffers;
 	} else {
@@ -7834,6 +7851,9 @@ int ring_buffer_map(struct trace_buffer *buffer, int cpu,
 	/* prevent another thread from changing buffer/sub-buffer sizes */
 	guard(mutex)(&buffer->mutex);
 
+	if (cpu_buffer->nr_pages > rb_static_max_pages())
+		return -E2BIG;
+
 	err = rb_alloc_meta_page(cpu_buffer);
 	if (err)
 		return err;
-- 
2.55.0.691.gc56d675ccc-goog

Re: [PATCH v7 2/3] ring-buffer: Cap static ring buffer nr_pages

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Integer underflow silently bypasses the newly introduced rb_static_max_pages() bound check on 32-bit platforms.
- [High] Integer overflow in pointer arithmetic when ring buffer is large causes out-of-bounds memory mapping.
- [Medium] Mixing goto-based error handling with scope-based cleanup (__free and guard) violates subsystem guidelines.
--

--- Patch [2]: [PATCH v7 2/3] ring-buffer: Cap static ring buffer nr_pages ---
commit 5ef3e045e7449785cd305ffb3c9bd4573685ad34
Author: Vincent Donnefort <vdonnefort@google.com>

ring-buffer: Cap static ring buffer nr_pages
    
This commit introduces a maximum page limit for static ring buffers to
ensure the page count fits within the buffer_page::id field. It enforces
this limit during buffer allocation and user-mapping, while increasing
the id field to 31 bits.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index ec13779922ffb..5e26eeff588f2 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
[ ... ]
> @@ -652,6 +652,15 @@ static bool rb_is_static(struct ring_buffer_per_cpu *cpu_buffer)
>  	return cpu_buffer->user_mapped || cpu_buffer->remote || cpu_buffer->ring_meta;
>  }
>  
> +static unsigned long rb_static_max_pages(void)
> +{
> +	/*
> +	 * Static ring buffers are using bpage::id and must account for the
> +	 * reader page.
> +	 */
> +	return (1UL << 31) - 1;
> +}

[Severity: High]
This is a pre-existing issue, but does a maximum page limit this large cause
integer overflow in pointer arithmetic when mapping memory?

While the limit restricts the number of pages to (1UL << 31) - 1, configuring
a very large memory-mapped trace buffer (e.g., >= 2GB per CPU, or ~524,288
pages) can cause multiplication overflows elsewhere:

kernel/trace/ring_buffer.c:rb_range_meta() {
    ...
    ptr += subbuf_size * nr_subbufs;
    ...
}

And similarly in rb_range_buffer():

kernel/trace/ring_buffer.c:rb_range_buffer() {
    ...
    ptr += subbuf_size * idx;
    ...
}

Since the operands are treated as 32-bit signed integers, configuring a
buffer with 524,288 pages causes the multiplication to exceed INT_MAX and
overflow into a negative value. This negative offset is added to the starting
pointer, resulting in the mapped buffer incorrectly pointing to memory before
the valid allocation range.

> +
>  struct ring_buffer_iter {
>  	struct ring_buffer_per_cpu	*cpu_buffer;
>  	unsigned long			head;
[ ... ]
> @@ -2837,6 +2846,10 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
>  		 */
>  		nr_pages = (size - sizeof(struct ring_buffer_cpu_meta)) /
>  			(subbuf_size + sizeof(int));

[Severity: High]
This is a pre-existing issue, but does an integer underflow here silently
bypass the newly introduced bound check on 32-bit platforms?

If the memory-mapped ring buffer size is extremely small, the calculation of
the available size for per-CPU data underflows, causing the size variable to
become a massive positive value.

On 32-bit architectures, the nr_pages variable evaluates to approximately 1
million pages. Since this is less than the rb_static_max_pages() limit, the
check passes. The subsequent loop calculating subbuffer pointers then
overflows 32-bit arithmetic, bypassing bounds checks and allowing
out-of-bounds memory corruption.

> +
> +		if (nr_pages > rb_static_max_pages())
> +			goto fail_free_buffers;

[Severity: Medium]
This is a pre-existing issue, but does adding new goto error paths here
perpetuate a mixture of goto-based error handling and scope-based cleanup?

The alloc_buffer() function uses scope-based cleanup:

    struct trace_buffer *buffer __free(kfree) = NULL;

Mixing __free() declarations and manual goto cleanup labels in the same
function violates subsystem guidelines and increases the risk of memory leaks
or confusing ownership semantics.

> +
>  		/* Need at least two pages plus the reader page */
>  		if (nr_pages < 3)
>  			goto fail_free_buffers;

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

[PATCH v7 3/3] ring-buffer: Prevent truncation of nr_pages / nr_subbufs

[Vincent Donnefort]

Although ring_buffer_per_cpu::nr_pages is defined as unsigned long, it
is capped to 32-bits in a few places, limiting the operations possible
on a very large buffer. Use `unsigned long` where appropriate and
prevent truncation of values using nr_pages (or nr_subbufs).

While at it, subbuf_size must be at least `unsigned int`.

Note that persistent, remote and user-mapped ring buffers are capping
the number of pages to 31 bits already, making "int" safe in many
places.

Signed-off-by: Vincent Donnefort <vdonnefort@google.com>

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 5e26eeff588f..7d22f10df3d6 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -1678,7 +1678,7 @@ static void rb_check_pages(struct ring_buffer_per_cpu *cpu_buffer)
  * This is used to help find the next per cpu subbuffer within a mapped range.
  */
 static unsigned long
-rb_range_align_subbuf(unsigned long addr, int subbuf_size, int nr_subbufs)
+rb_range_align_subbuf(unsigned long addr, unsigned int subbuf_size, unsigned long nr_subbufs)
 {
 	addr += sizeof(struct ring_buffer_cpu_meta) +
 		sizeof(int) * nr_subbufs;
@@ -1688,13 +1688,12 @@ rb_range_align_subbuf(unsigned long addr, int subbuf_size, int nr_subbufs)
 /*
  * Return the ring_buffer_meta for a given @cpu.
  */
-static void *rb_range_meta(struct trace_buffer *buffer, int nr_pages, int cpu)
+static void *rb_range_meta(struct trace_buffer *buffer, unsigned long nr_pages, int cpu)
 {
-	int subbuf_size = rb_subbuf_size(buffer);
+	unsigned int subbuf_size = rb_subbuf_size(buffer);
 	struct ring_buffer_cpu_meta *meta;
 	struct ring_buffer_meta *bmeta;
-	unsigned long ptr;
-	int nr_subbufs;
+	unsigned long ptr, nr_subbufs;
 
 	bmeta = buffer->meta;
 	if (!bmeta)
@@ -1740,7 +1739,7 @@ static void *rb_range_meta(struct trace_buffer *buffer, int nr_pages, int cpu)
 /* Return the start of subbufs given the meta pointer */
 static void *rb_subbufs_from_meta(struct ring_buffer_cpu_meta *meta)
 {
-	int subbuf_size = meta->subbuf_size;
+	unsigned int subbuf_size = meta->subbuf_size;
 	unsigned long ptr;
 
 	ptr = (unsigned long)meta;
@@ -1752,11 +1751,11 @@ static void *rb_subbufs_from_meta(struct ring_buffer_cpu_meta *meta)
 /*
  * Return a specific sub-buffer for a given @cpu defined by @idx.
  */
-static void *rb_range_buffer(struct ring_buffer_per_cpu *cpu_buffer, int idx)
+static void *rb_range_buffer(struct ring_buffer_per_cpu *cpu_buffer, unsigned int idx)
 {
 	struct ring_buffer_cpu_meta *meta;
+	unsigned int subbuf_size;
 	unsigned long ptr;
-	int subbuf_size;
 
 	meta = rb_range_meta(cpu_buffer->buffer, 0, cpu_buffer->cpu);
 	if (!meta)
@@ -1772,7 +1771,7 @@ static void *rb_range_buffer(struct ring_buffer_per_cpu *cpu_buffer, int idx)
 
 	ptr = (unsigned long)rb_subbufs_from_meta(meta);
 
-	ptr += subbuf_size * idx;
+	ptr += (unsigned long)subbuf_size * idx;
 	if (ptr + subbuf_size > cpu_buffer->buffer->range_addr_end)
 		return NULL;
 
@@ -1849,13 +1848,13 @@ static bool rb_meta_init(struct trace_buffer *buffer, int scratch_size)
  * must be the same.
  */
 static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
-			      struct trace_buffer *buffer, int nr_pages,
+			      struct trace_buffer *buffer, unsigned long nr_pages,
 			      unsigned long *subbuf_mask)
 {
-	int subbuf_size = PAGE_SIZE;
+	unsigned long subbuf_size = PAGE_SIZE;
 	unsigned long buffers_start;
 	unsigned long buffers_end;
-	int i;
+	unsigned long i;
 
 	if (!subbuf_mask)
 		return false;
@@ -2104,8 +2103,8 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 	struct buffer_page *head_page, *orig_head, *orig_reader;
 	struct rb_validation_state state = { 0 };
 	bool skip = false;
+	unsigned long i;
 	int ret;
-	int i;
 
 	if (!meta || !meta->head_buffer)
 		return;
@@ -2156,7 +2155,7 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 		rb_validate_buffer(head_page, cpu_buffer, meta, &state, 0, state.ts);
 	}
 	if (i)
-		pr_info("Ring buffer [%d] rewound %d pages\n", cpu_buffer->cpu, i);
+		pr_info("Ring buffer [%d] rewound %lu pages\n", cpu_buffer->cpu, i);
 
 	/* The last rewound page must be skipped. */
 	if (head_page != orig_head)
@@ -2240,7 +2239,8 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 	}
 }
 
-static void rb_range_meta_init(struct trace_buffer *buffer, int nr_pages, int scratch_size)
+static void rb_range_meta_init(struct trace_buffer *buffer,
+			       unsigned long nr_pages, int scratch_size)
 {
 	struct ring_buffer_cpu_meta *meta;
 	unsigned long *subbuf_mask;
@@ -2340,8 +2340,8 @@ static int rbm_show(struct seq_file *m, void *v)
 			   rb_meta_subbuf_idx(meta, (void *)meta->head_buffer));
 		seq_printf(m, "commit_buffer: %d\n",
 			   rb_meta_subbuf_idx(meta, (void *)meta->commit_buffer));
-		seq_printf(m, "subbuf_size:   %d\n", meta->subbuf_size);
-		seq_printf(m, "nr_subbufs:    %d\n", meta->nr_subbufs);
+		seq_printf(m, "subbuf_size:   %u\n", meta->subbuf_size);
+		seq_printf(m, "nr_subbufs:    %u\n", meta->nr_subbufs);
 		return 0;
 	}
 
@@ -2426,7 +2426,7 @@ static void *ring_buffer_desc_page(struct ring_buffer_desc *desc, unsigned int p
 }
 
 static int __rb_allocate_pages(struct ring_buffer_per_cpu *cpu_buffer,
-		long nr_pages, struct list_head *pages)
+			       unsigned long nr_pages, struct list_head *pages)
 {
 	struct trace_buffer *buffer = cpu_buffer->buffer;
 	struct ring_buffer_cpu_meta *meta = NULL;
@@ -2554,7 +2554,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, long nr_pages, int cpu)
+rb_allocate_cpu_buffer(struct trace_buffer *buffer, unsigned long nr_pages, int cpu)
 {
 	struct ring_buffer_per_cpu *cpu_buffer __free(kfree) =
 		alloc_cpu_buffer(cpu);
@@ -2711,8 +2711,8 @@ static void rb_test_inject_invalid_pages(struct trace_buffer *buffer)
 	struct ring_buffer_cpu_meta *meta;
 	struct buffer_data_page *dpage;
 	unsigned long entry_bytes = 0;
+	unsigned int subbuf_size;
 	unsigned long ptr;
-	int subbuf_size;
 	int invalid = 0;
 	int cpu;
 	int i;
@@ -2782,8 +2782,8 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
 					 struct ring_buffer_remote *remote)
 {
 	struct trace_buffer *buffer __free(kfree) = NULL;
-	long nr_pages;
-	int subbuf_size;
+	unsigned int subbuf_size;
+	unsigned long nr_pages;
 	int bsize;
 	int cpu;
 	int ret;
@@ -5867,12 +5867,12 @@ __rb_get_reader_page_from_remote(struct ring_buffer_per_cpu *cpu_buffer)
 static struct buffer_page *
 __rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 {
-	int max_loops = cpu_buffer->ring_meta ? cpu_buffer->nr_pages : 3;
+	unsigned long max_loops = cpu_buffer->ring_meta ? cpu_buffer->nr_pages : 3;
 	struct buffer_page *reader = NULL;
+	unsigned long nr_loops = 0;
 	unsigned long overwrite;
 	unsigned long flags;
 	int missed_events = 0;
-	int nr_loops = 0;
 	bool ret;
 
 	local_irq_save(flags);
@@ -6190,8 +6190,8 @@ rb_iter_peek(struct ring_buffer_iter *iter, u64 *ts)
 	struct trace_buffer *buffer;
 	struct ring_buffer_per_cpu *cpu_buffer;
 	struct ring_buffer_event *event;
-	int nr_loops = 0;
-	int max_loops;
+	unsigned long nr_loops = 0;
+	unsigned long max_loops;
 
 	if (ts)
 		*ts = 0;
@@ -7438,8 +7438,8 @@ int ring_buffer_subbuf_order_set(struct trace_buffer *buffer, int order)
 	struct ring_buffer_per_cpu *cpu_buffer;
 	struct buffer_page *bpage, *tmp;
 	unsigned int old_capacity;
+	unsigned long nr_pages;
 	int old_order;
-	int nr_pages;
 	int psize;
 	int err;
 	int cpu;
@@ -7621,10 +7621,10 @@ static void rb_setup_ids_meta_page(struct ring_buffer_per_cpu *cpu_buffer,
 				   struct buffer_page **subbuf_ids)
 {
 	struct trace_buffer_meta *meta = cpu_buffer->meta_page;
-	unsigned int nr_subbufs = cpu_buffer->nr_pages + 1;
+	unsigned long nr_subbufs = cpu_buffer->nr_pages + 1;
 	struct buffer_page *first_subbuf, *subbuf;
-	int cnt = 0;
-	int id = 0;
+	unsigned int cnt = 0;
+	unsigned int id = 0;
 
 	id = rb_page_id(cpu_buffer, cpu_buffer->reader_page, id);
 	subbuf_ids[id++] = cpu_buffer->reader_page;
-- 
2.55.0.691.gc56d675ccc-goog

Re: [PATCH v7 3/3] ring-buffer: Prevent truncation of nr_pages / nr_subbufs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Integer underflow in alloc_buffer() for mapped ring buffers leads to memory corruption on 32-bit systems.
- [High] Out-of-bounds memory corruption in rb_cpu_meta_valid during ring buffer boot meta validation due to unvalidated meta->nr_subbufs.
--

--- Patch [3]: [PATCH v7 3/3] ring-buffer: Prevent truncation of nr_pages / nr_subbufs ---
commit ca74d5f745c91c8b4a7ee42fae91ab814132919c
Author: Vincent Donnefort <vdonnefort@google.com>

ring-buffer: Prevent truncation of nr_pages / nr_subbufs

This commit prevents truncation operations on large ring buffers by changing
nr_pages and nr_subbufs definitions to unsigned long. It also upgrades
subbuf_size to unsigned int.

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c

[ ... ]

> @@ -1849,13 +1848,13 @@ static bool rb_meta_init(struct trace_buffer *buffer, int scratch_size)
>   * must be the same.
>   */
>  static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
> -			      struct trace_buffer *buffer, int nr_pages,
> +			      struct trace_buffer *buffer, unsigned long nr_pages,
>  			      unsigned long *subbuf_mask)
>  {
> -	int subbuf_size = PAGE_SIZE;
> +	unsigned long subbuf_size = PAGE_SIZE;
>  	unsigned long buffers_start;
>  	unsigned long buffers_end;
> -	int i;
> +	unsigned long i;
>  
>  	if (!subbuf_mask)
>  		return false;

[Severity: High]
This is a pre-existing issue, but can this lead to an out-of-bounds memory
overwrite on the kernel heap?

In rb_range_meta_init(), subbuf_mask is allocated using the current boot's
nr_pages + 1:

    subbuf_mask = bitmap_alloc(nr_pages + 1, GFP_KERNEL);

However, later in rb_cpu_meta_valid(), the legacy persistent metadata is used 
to clear and set bits in this mask:

    bitmap_clear(subbuf_mask, 0, meta->nr_subbufs);
    ...
    for (i = 0; i < meta->nr_subbufs; i++) {
        ...
        set_bit(meta->buffers[i], subbuf_mask);
    }

If the system boots with more CPUs than the previous cycle, the per-CPU
nr_pages constraint decreases. Consequently, the legacy meta->nr_subbufs 
could be larger than the newly allocated subbuf_mask, causing bitmap_clear()
and set_bit() to overflow the allocation.

[ ... ]

> @@ -2782,8 +2782,8 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
>  					 struct ring_buffer_remote *remote)
>  {
>  	struct trace_buffer *buffer __free(kfree) = NULL;
> -	long nr_pages;
> -	int subbuf_size;
> +	unsigned int subbuf_size;
> +	unsigned long nr_pages;
>  	int bsize;
>  	int cpu;
>  	int ret;

[Severity: High]
This is a pre-existing issue, but does this function suffer from an integer
underflow that could bypass length boundary validations?

Further down in alloc_buffer(), when calculating the per-CPU size for mapped
buffers:

    size = end - buffers_start;
    size = size / nr_cpu_ids;
    nr_pages = (size - sizeof(struct ring_buffer_cpu_meta)) /
               (subbuf_size + sizeof(int));

If the user-provided mapped range is smaller than the meta structure size,
size - sizeof(struct ring_buffer_cpu_meta) will underflow. On 32-bit systems,
this yields a massive nr_pages value that successfully passes the
rb_static_max_pages() threshold check. 

The subsequently calculated pointers will wrap around the 32-bit address
space, defeating the bounds check against the end boundary, and allowing
mapped metadata structures to be written into arbitrary out-of-bounds memory.

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