On Wed, Aug 12, 2026 at 03:53:12PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
> to consider:
>
> New issues:
> - [High] Use-After-Free dereference of `reader_page` in
> `ring_buffer_read_start()` when bypassing buffer mutex.
>
> Pre-existing issues:
> - [High] 32-bit integer overflow in ring buffer capacity calculation
> truncates large trace buffers.
> - [High] Out-of-bounds kernel memory write and VMAP flush in
> `ring_buffer_map_get_reader()` due to global capacity TOCTOU race.
> --
>
> --- Patch [3]: [PATCH v4 3/9] ring-buffer: Fix subbuf resize race with ring
> buffer readers ---
> commit 9618f28d9a6939c645e7579fa798c42557eb075c
> Author: Vincent Donnefort <[email protected]>
>
> ring-buffer: Fix subbuf resize race with ring buffer readers
>
> This commit changes the trace_buffer to use bpage order instead of
> trace_buffer::subbuf_size in ring_buffer_read_start() and
> ring_buffer_read_page(). It also removes trace_buffer::subbuf_size and
> introduces capacity accessors.
>
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index ec4f5a0c93e85..97449423d3a68 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> [ ... ]
> > @@ -6380,36 +6417,44 @@ EXPORT_SYMBOL_GPL(ring_buffer_consume);
> > struct ring_buffer_iter *
> > ring_buffer_read_start(struct trace_buffer *buffer, int cpu, gfp_t flags)
> > {
> > + struct ring_buffer_iter *iter __free(kfree) = kzalloc_obj(*iter, flags);
> > struct ring_buffer_per_cpu *cpu_buffer;
> > - struct ring_buffer_iter *iter;
> > -
> > - if (!cpumask_test_cpu(cpu, buffer->cpumask))
> > - return NULL;
> >
> > - iter = kzalloc_obj(*iter, flags);
> > if (!iter)
> > return NULL;
> >
> > - /* Holds the entire event: data and meta data */
> > - iter->event_size = buffer->subbuf_size;
> > - iter->event = kmalloc(iter->event_size, flags);
> > - if (!iter->event) {
> > - kfree(iter);
> > + if (!cpumask_test_cpu(cpu, buffer->cpumask))
> > return NULL;
> > - }
> >
> > cpu_buffer = buffer->buffers[cpu];
> >
> > - iter->cpu_buffer = cpu_buffer;
> > + /*
> > + * Only KDB is using GFP_ATOMIC, for the others, lock the buffer to
> > + * prevent concurrent resizing.
> > + */
> > + if (gfpflags_allow_blocking(flags))
> > + mutex_lock(&buffer->mutex);
> >
> > atomic_inc(&cpu_buffer->resize_disabled);
> >
> > + if (gfpflags_allow_blocking(flags))
> > + mutex_unlock(&buffer->mutex);
> > +
> > + /* Holds the entire event: data and meta data. */
> > + iter->event_size = rb_page_capacity(READ_ONCE(cpu_buffer->reader_page));
>
> [Severity: High]
> Can this result in a use-after-free dereference of the reader page?
>
> When ring_buffer_read_start() is called with GFP_ATOMIC, it skips acquiring
> buffer->mutex and only increments cpu_buffer->resize_disabled.
>
> If a concurrent ring_buffer_subbuf_order_set() has already passed its
> resize_disabled check, it will proceed to allocate new pages and free the old
> reader page.
>
> A thread executing ring_buffer_read_start() could load the old reader page
> pointer just before it is freed by the other CPU, dereferencing freed memory
> when accessing the order field.
Yep, as the comment says, we only bypass for KDB.
>
> > + iter->event = kmalloc(iter->event_size, flags);
> > + if (!iter->event) {
> > + atomic_dec(&cpu_buffer->resize_disabled);
> > + return NULL;
> > + }
> [ ... ]
> > @@ -7367,8 +7413,8 @@ int ring_buffer_subbuf_order_set(struct trace_buffer
> > *buffer, int order)
> > }
> >
> > /* Update the number of pages to match the new size */
> > - nr_pages = old_size * buffer->buffers[cpu]->nr_pages;
> > - nr_pages = DIV_ROUND_UP(nr_pages, buffer->subbuf_size);
> > + nr_pages = old_capacity * buffer->buffers[cpu]->nr_pages;
>
> [Severity: High]
> This is a pre-existing issue, but can this 32-bit integer capacity calculation
> truncate large trace buffers?
>
> For buffers larger than 4GB, the multiplication of old_capacity and
> buffer->buffers[cpu]->nr_pages can exceed 32 bits. Because nr_pages is a
> 32-bit signed integer, the result is truncated, and the buffer is shrunk to a
> tiny fraction of its intended size during a resize operation.
This is fixed in a later patch.
>
> > + nr_pages = DIV_ROUND_UP(nr_pages, rb_subbuf_capacity(buffer));
> >
> > /* we need a minimum of two pages */
> > if (nr_pages < 2)
> [ ... ]
> > @@ -7896,7 +7941,7 @@ int ring_buffer_map_get_reader(struct trace_buffer
> > *buffer, int cpu)
> > * missed events, then record it there.
> > */
> > commit = rb_page_size(reader);
> > - if (buffer->subbuf_size - commit >=
> > sizeof(missed_events)) {
> > + if (rb_subbuf_capacity(buffer) - commit >=
> > sizeof(missed_events)) {
>
> [Severity: High]
> This is a pre-existing issue, but does this create a time-of-check to
> time-of-use race leading to an out-of-bounds kernel memory write and
> VMAP flush?
This is fixed in a later patch.
>
> During a sub-buffer order resize, ring_buffer_subbuf_order_set() modifies
> buffer->subbuf_order globally before verifying if resize_disabled is set. If
> the buffer is currently mapped, the resize aborts and restores the old order.
>
> In that race window, a concurrent ring_buffer_map_get_reader() call will see
> the new, larger capacity via rb_subbuf_capacity(buffer) here. It could then
> write missed_events past the end of the old, smaller reader page, and
> subsequently flush memory beyond the mapped VMAP range.
>
> > memcpy(&dpage->data[commit], &missed_events,
> > sizeof(missed_events));
> > local_add(RB_MISSED_STORED, &dpage->commit);
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/[email protected]?part=3
--
Vincent
