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

[Steven Rostedt]

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? */

>  	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.

Also, I mentioned that if the commit == RB_MISSED_EVENTS, then we know
the sub buffer was corrupted and should be skipped.

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).

So, perhaps we should invalidate the entire buffer if the commit part
is corrupted, as that is a major corruption.

-- Steve