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. > + > + 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. > > - 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. > + 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? > - > 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. > + 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 > + } > + > 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; > }; > > /**
