One issue turned up while checking the suggested locking: ring_buffer_subbuf_order_set() writes buffer->subbuf_order before taking reader_lock and never takes cpu_buffer->lock. An allocator can therefore take cpu_buffer->lock after the new order is published but before resize clears the old-order free_page, tag that old page with the new order, and return it.
I can keep the allocations outside buffer->mutex and hold the mutex only while snapshotting subbuf_order and taking or returning free_page. That removes allocation from the critical section and serializes the order/free-page pair with resize. Would you prefer that, or should free_page and the order transition be synchronized another way? On Tue, 30 Jun 2026 10:14:25 -0400, Steven Rostedt <[email protected]> wrote: > On Sun, 28 Jun 2026 02:46:53 +0200 > Yousef Alhouseen <[email protected]> wrote: > > > ring_buffer_read_page() checks that its spare page has the current > > subbuffer order before taking cpu_buffer->reader_lock. A concurrent > > ring_buffer_subbuf_order_set() can change the order and replace the > > reader page after that check. The reader then copies a larger subbuffer > > into the old allocation, causing an out-of-bounds write. > > > > Keep spare-page allocation and release under buffer->mutex, which already > > serializes order changes. Move the read-side order check under > > reader_lock, the lock used by resize when replacing per-CPU pages. > > > > Fixes: f9b94daa542a ("ring-buffer: Set new size of the ring buffer sub > > page") > > Reported-by: [email protected] > > Closes: https://syzkaller.appspot.com/bug?extid=2dd9d02f60775ce5c1fb > > Cc: [email protected] > > Signed-off-by: Yousef Alhouseen <[email protected]> > > --- > > kernel/trace/ring_buffer.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > > index 56a328e94395..eed5d7cffdee 100644 > > --- a/kernel/trace/ring_buffer.c > > +++ b/kernel/trace/ring_buffer.c > > @@ -6950,6 +6950,8 @@ ring_buffer_alloc_read_page(struct trace_buffer > > *buffer, int cpu) > > if (!cpumask_test_cpu(cpu, buffer->cpumask)) > > return ERR_PTR(-ENODEV); > > > > + guard(mutex)(&buffer->mutex); > > + > > bpage = kzalloc_obj(*bpage); > > First, do not grab locks around allocations unless the are really needed. > This is bad practice, as it extends the critical section and may even add > the allocation locking to the lock chain. > > That said, just moving things around the current locks should work. > > Like this (not compiled nor tested): > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > index 56a328e94395..8352f935a223 100644 > --- a/kernel/trace/ring_buffer.c > +++ b/kernel/trace/ring_buffer.c > @@ -6954,11 +6954,11 @@ ring_buffer_alloc_read_page(struct trace_buffer > *buffer, int cpu) > if (!bpage) > return ERR_PTR(-ENOMEM); > > - bpage->order = buffer->subbuf_order; > cpu_buffer = buffer->buffers[cpu]; > local_irq_save(flags); > arch_spin_lock(&cpu_buffer->lock); > > + bpage->order = buffer->subbuf_order; > if (cpu_buffer->free_page) { > bpage->data = cpu_buffer->free_page; > cpu_buffer->free_page = NULL; > @@ -7007,13 +7007,13 @@ 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) > goto out; > > local_irq_save(flags); > arch_spin_lock(&cpu_buffer->lock); > > - if (!cpu_buffer->free_page) { > + if (!cpu_buffer->free_page && data_page->order == buffer->subbuf_order) > cpu_buffer->free_page = dpage; > dpage = NULL; > } > @@ -7091,15 +7091,15 @@ int ring_buffer_read_page(struct trace_buffer *buffer, > if (!data_page || !data_page->data) > return -1; > > - if (data_page->order != buffer->subbuf_order) > - return -1; > - > dpage = data_page->data; > if (!dpage) > return -1; > > guard(raw_spinlock_irqsave)(&cpu_buffer->reader_lock); > > + if (data_page->order != buffer->subbuf_order) > + return -1; > + > reader = rb_get_reader_page(cpu_buffer); > if (!reader) > return -1; > > -- Steve
