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

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

Hi Steve,

On Sun, 8 Mar 2026 22:19:01 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 9 Mar 2026 08:53:17 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > > 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. 
> 
> The RB_MISSED_EVENTS is only set on the reader page.

When is it reset after read? I think I removed that with commit ca296d32ece3 ("tracing: ring_buffer: Rewind persistent ring buffer on reboot"). So the
flags will be remains until the page is reused (as a writer page).

> 
> If the kernel crashes no boot up while reading the validated buffer,
> then that's a bit more than what this is supposed to handle.

But the above commit recovers the subbufs which has been read in the
previous boot. This means commit field of those recovered subbufs
can have those flags.

> 
> > 
> > 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.
> 
> But that should be fine, as it's only read only. Once tracing is
> started, it should be reset.

IIUC, RB_MISSED_* flags are using 30 and 31 th bits and committed
bytes counter is usually use 0-11 th bits. So other bits should be
cleared. 

So, if "commit & RB_MISSED_MASK" is under the subbuf_size, this
subbuf looks OK for checking its entries(timestamp data).
e.g.

unsigned long commit;

commit = local_read(subbuf->commit) & ~RB_MISSED_MASK;
if (commit > meta->subbuf_size)
  return -EINVAL;
return ;

Thank you,

> 
> -- Steve


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>