Re: [PATCH v7 2/2] ring-buffer: Skip invalid sub-buffers when validating persistent ring buffer

[Google]

On Mon, 9 Mar 2026 08:53:17 +0900
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> wrote:

> On Sat, 7 Mar 2026 10:27:11 -0500
> Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> 
> > On Sat,  7 Mar 2026 23:26:38 +0900
> > "Masami Hiramatsu (Google)" <mhiramat@xxxxxxxxxx> wrote:
> > 
> > >  kernel/trace/ring_buffer.c |   63 +++++++++++++++++++++++---------------------
> > >  1 file changed, 33 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > > index b6f3ac99834f..8599de5cf59b 100644
> > > --- a/kernel/trace/ring_buffer.c
> > > +++ b/kernel/trace/ring_buffer.c
> > > @@ -396,6 +396,12 @@ static __always_inline unsigned int rb_page_commit(struct buffer_page *bpage)
> > >  	return local_read(&bpage->page->commit);
> > >  }
> > >  
> > > +/* Size is determined by what has been committed */
> > > +static __always_inline unsigned int rb_page_size(struct buffer_page *bpage)
> > > +{
> > > +	return rb_page_commit(bpage) & ~RB_MISSED_MASK;
> > > +}
> > > +
> > >  static void free_buffer_page(struct buffer_page *bpage)
> > >  {
> > >  	/* Range pages are not to be freed */
> > > @@ -1819,7 +1825,7 @@ static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
> > >  
> > >  	bitmap_clear(subbuf_mask, 0, meta->nr_subbufs);
> > >  
> > > -	/* Is the meta buffers and the subbufs themselves have correct data? */
> > > +	/* Is the meta buffers themselves have correct data? */
> > 
> > I just realized that the origin didn't have correct grammar. But we
> > still check the subbufs, why remove that comment?
> > 
> > The original should have said:
> > 
> > 	/* Do the meta buffers and subbufs have correct data? */
> 
> I just removed the data check from this loop, so I think this should
> focus on checking metadata itself. The data is checked later.

Other checks in the loop are;

- the entries in meta::buffers[] are inside correct range.
- the duplicated entries in the meta::buffers[].

So this only checks the meta::buffers[] (index array) now.

/*
 * Ensure the meta::buffers have correct data. The data in each subbufs are
 * checked later in rb_meta_validate_events().
 */

This will be more clear.

> 
> > 
> > >  	for (i = 0; i < meta->nr_subbufs; i++) {
> > >  		if (meta->buffers[i] < 0 ||
> > >  		    meta->buffers[i] >= meta->nr_subbufs) {
> > > @@ -1827,11 +1833,6 @@ static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
> > >  			return false;
> > >  		}
> > >  
> > > -		if ((unsigned)local_read(&subbuf->commit) > subbuf_size) {
> > > -			pr_info("Ring buffer boot meta [%d] buffer invalid commit\n", cpu);
> > > -			return false;
> > > -		}
> > 
> > This should still be checked, although it doesn't need to fail the loop
> > but instead continue to the next buffer.
> 
> We already have another check of the data in the loop in
> rb_meta_validate_events() so data corruption should be
> handled there.
> 
> > 
> > Also, I mentioned that if the commit == RB_MISSED_EVENTS, then we know
> > the sub buffer was corrupted and should be skipped.
> 
> Yes, if RB_MISSED_EVENTS bit is set, the commit field is out of range.
> That is checked in rb_validate_buffer().
> 
> > 
> > And honestly, the commit should never be greater than the subbuf_size,
> > even if corrupted. As we are only worried about corruption due to cache
> > not writing out. That should not corrupt the commit size (now we can
> > ignore the flags and use page size instead).
> 
> Hmm, but if the kernel crash and reboot when it sets RB_MISSED_EVENTS,
> we will see the bit is set and commit size is different. 
> 
> Note, I think the reader_page RB_MISSED_EVENTS flag is not cleared after
> read. commit ca296d32ece3 ("tracing: ring_buffer: Rewind persistent
> ring buffer on reboot") drops clearing commit field for unwinding the
> buffer.
> 
> @@ -5342,7 +5440,6 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
>          */
>         local_set(&cpu_buffer->reader_page->write, 0);
>         local_set(&cpu_buffer->reader_page->entries, 0);
> -       local_set(&cpu_buffer->reader_page->page->commit, 0);
>         cpu_buffer->reader_page->real_end = 0;
>  
> Should we clear the RB_MISSED_* bits here?

Ah, no. ignore this. If there is a sudden reboot, the broken
commit will be there anyway. But we can recover it.

Thank you,

> 
> Thanks,
> 
> > 
> > So, perhaps we should invalidate the entire buffer if the commit part
> > is corrupted, as that is a major corruption.
> > 
> > -- Steve
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>


-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>