On Sat, Aug 22, 2026 at 11:04:24AM -0400, Steven Rostedt wrote:
> On Mon, 17 Aug 2026 14:08:11 +0000
> [email protected] 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;
> > > + }
> >
>
