Re: [PATCH v7 1/3] tracing: Fix subbuf resize races with trace_pipe_raw readers

[Vincent Donnefort <vdonnefort@google.com>]

On Sat, Aug 22, 2026 at 11:04:24AM -0400, Steven Rostedt wrote:
> On Mon, 17 Aug 2026 14:08:11 +0000
> sashiko-bot@kernel.org 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;
> > > +		}  
> > 
>