On Thu, Sep 03, 2026 at 11:48:45AM -0400, Steven Rostedt wrote:
> On Tue, 1 Sep 2026 16:54:43 +0100
> Vincent Donnefort <[email protected]> wrote:
>
> > 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 the resizing of an
> > existing buffer_data_read_page if necessary and add a new
> > ring_buffer_read_page_size(). This new function enables ring-buffer
> > buffer_data_read_page users to not call the racy
> > ring_buffer_subbuf_size_get(). This makes the spare_size member of
> > ftrace_buffer_info redundant.
> >
> > Finally, handle buffer_data_read_page/reader_page order discrepancy in
> > ring_buffer_read_page(). On a mismatch simply copy manually the data to
> > the buffer_data_read_page.
>
> Let's add here:
>
> Link:
> https://lore.kernel.org/all/[email protected]/
>
> As it has more information about why we came up with this solution.
>
> >
> > Fixes: bce761d75745 ("ring-buffer: Read and write to ring buffers with
> > custom sub buffer size")
> > Signed-off-by: Vincent Donnefort <[email protected]>
>
>
>
> > -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;
>
> Hmm, should we add a READ_ONCE() around the subbuf_order? There's no locks
> taken here and couldn't we get some inconsistency if things change. I feel
> more comfortable knowing that "order" is consistent throughout this
> function.
Ack, I'll see if there are other lockless readers without READ_ONCE().
>
> > +
> > + 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);
> >
> > @@ -7050,21 +7077,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];
> >
> > /*
> > @@ -7072,14 +7108,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;
> > }
> >
> > @@ -7087,8 +7123,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);
> >
> > @@ -7159,10 +7195,9 @@ int ring_buffer_read_page(struct trace_buffer
> > *buffer,
> > if (!dpage)
> > return -1;
> >
> > - guard(raw_spinlock_irqsave)(&cpu_buffer->reader_lock);
> > + len = min_t(size_t, len, rb_read_page_capacity(data_page));
> >
> > - if (data_page->order != cpu_buffer->reader_page->order)
> > - return -1;
> > + guard(raw_spinlock_irqsave)(&cpu_buffer->reader_lock);
> >
> > reader = rb_get_reader_page(cpu_buffer);
> > if (!reader)
> > @@ -7177,16 +7212,18 @@ int ring_buffer_read_page(struct trace_buffer
> > *buffer,
> > /* Check if any events were dropped */
> > missed_events = cpu_buffer->lost_events;
> >
> > - /*
> > - * If this page has been partially read or
> > - * if len is not big enough to read the rest of the page or
> > - * a writer is still on the page, then
> > - * we must copy the data from the page to the buffer.
> > - * Otherwise, we can simply swap the page with the one passed in.
> > - */
> > + /*
> > + * It is not possible to swap the reader page if:
> > + * - It has been partially read
> > + * - len is not big enough to read it entirely
> > + * - A writer is still on it
> > + * - The ring buffer is static
> > + * - The order doesn't match
> > + */
> > if (read || (len < (size - read)) ||
> > cpu_buffer->reader_page == cpu_buffer->commit_page ||
> > - rb_is_static(cpu_buffer)) {
> > + rb_is_static(cpu_buffer) ||
> > + data_page->order != reader->order) {
> > struct buffer_data_page *rpage = cpu_buffer->reader_page->page;
> > unsigned int rpos = read;
> > unsigned int pos = 0;
> > @@ -7280,7 +7317,7 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
> > * missed events, then record it there.
> > */
> > if (missed_events > 0 &&
> > - rb_page_capacity(reader) - size >= sizeof(missed_events)) {
> > + rb_read_page_capacity(data_page) - size >=
> > sizeof(missed_events)) {
> > memcpy(&dpage->data[size], &missed_events,
> > sizeof(missed_events));
> > local_add(RB_MISSED_STORED, &dpage->commit);
> > @@ -7300,8 +7337,8 @@ int ring_buffer_read_page(struct trace_buffer *buffer,
> > /*
> > * This page may be off to user land. Zero it out here.
> > */
> > - if (size < rb_page_capacity(reader))
> > - memset(&dpage->data[size], 0, rb_page_capacity(reader) - size);
> > + if (size < rb_read_page_capacity(data_page))
> > + memset(&dpage->data[size], 0, rb_read_page_capacity(data_page)
> > - size);
> >
> > return read;
> > }
> > @@ -7319,6 +7356,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 a946e0183fd1..7ba3856daf44 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -7082,8 +7082,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;
> >
> > @@ -7093,36 +7093,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;
> > }
>
> I would have ring_buffer_free_read_page() accept a null pointer and then here
> do:
>
> spare_size = ring_buffer_read_page_size(info->spare);
>
> again:
> /* Do we have previous read data to read? */
> if (info->read < spare_size)
> goto read;
>
> As the jump to here below has already calculated the spare_size, why do it
> again?
>
> Have ring_buffer_read_page_size() be:
>
> unsigned int ring_buffer_read_page_size(struct buffer_data_read_page *rpage)
> {
> return rpage ? PAGE_SIZE << rpage->order : 0;
> }
>
> Then info->read could not be less than spare_size if there was no spare.
Sounds good.
>
> >
> > - 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 */
>
> The above comment doesn't really make sense anymore since the user here
> should not care about the order. I would nuke it.
ack.
>
> > + 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,
> > @@ -7148,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);
> > @@ -7199,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);
> > }
> >
> > @@ -7270,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));
> > - }
>
> OK, you are removing this so that it is tested in the loop?
Yes, page_size can now be modified between iterations.
>
> > -
> > if (splice_grow_spd(pipe, &spd))
> > return -ENOMEM;
> >
> > @@ -7294,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++) {
>
> Is there a reason you moved the len -= page_size from here to the end of
> the loop? Basically that has no functional change.
Same reason as above, page_size can be modified and I thought it'd be more clear
to see the declaration within the loop, but yes, that wouldn't change anything.
I'll keep it as-is.
>
> > + unsigned int page_size;
>
> Was that just to move page_size here?
>
> Let's keep it as-is.
>
> > struct page *page;
> > int r;
> >
> > @@ -7306,25 +7285,36 @@ 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;
> > +
> > + 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);
> > + } 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;
>
> This is more likely to be true not because the subbuf order was modified,
> but also if the length was not a multiple of page_size. From the code you
> removed;
>
> - if (len & (page_size - 1)) {
> - if (len < page_size)
> - return -EINVAL;
> - len &= (~(page_size - 1));
> - }
>
> It would error if len was smaller than page_size but otherwise it would
> modify len to be a multiple of page_size.
>
> The overall behavior is the same, but the comment needs to be updated.
>
> -- Steve
I believe "len < page_size" is what I meant by "invalid userspace input", I'll
rephrase it to something more clear. I've just had a hard time myself
understanding that comment.
--
Vincent
>
> > + }
> > +
> > 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;
> > @@ -7332,6 +7322,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 74a7a50d1e78..203d098ee14e 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;
> > };
> >
> > /**
>
