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

Since the MSBs of rb_data_page::commit are used for storing
RB_MISSED_EVENTS and RB_MISSED_STORED, we need to mask out those bits
when it is used for finding the size of data pages.

Fixes: 5f3b6e839f3c ("ring-buffer: Validate boot range memory events")
Fixes: 5b7be9c709e1 ("ring-buffer: Add test to validate the time stamp deltas")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 Changes in v5:
   - Do not move rb_commit_index().
   - Fix verify_event() and rb_cpu_meta_valid() too.
 Changes in v4:
   - Fix to move rb_commit_index() after ring_buffer_per_cpu definition.
---
 kernel/trace/ring_buffer.c |   27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 0eb6e6595f37..2e9b8ce6b4dc 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -395,6 +395,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 */
@@ -676,7 +682,7 @@ static void verify_event(struct ring_buffer_per_cpu *cpu_buffer,
 	do {
 		if (page == tail_page || WARN_ON_ONCE(stop++ > 100))
 			done = true;
-		commit = local_read(&page->page->commit);
+		commit = rb_page_size(page);
 		write = local_read(&page->write);
 		if (addr >= (unsigned long)&page->page->data[commit] &&
 		    addr < (unsigned long)&page->page->data[write])
@@ -1820,13 +1826,16 @@ static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu,
 
 	/* Is the meta buffers and the subbufs themselves have correct data? */
 	for (i = 0; i < meta->nr_subbufs; i++) {
+		unsigned long commit;
+
 		if (meta->buffers[i] < 0 ||
 		    meta->buffers[i] >= meta->nr_subbufs) {
 			pr_info("Ring buffer boot meta [%d] array out of range\n", cpu);
 			return false;
 		}
 
-		if ((unsigned)local_read(&subbuf->commit) > subbuf_size) {
+		commit = local_read(&subbuf->commit) & ~RB_MISSED_MASK;
+		if (commit > subbuf_size) {
 			pr_info("Ring buffer boot meta [%d] buffer invalid commit\n", cpu);
 			return false;
 		}
@@ -1907,7 +1916,7 @@ static int rb_validate_buffer(struct buffer_data_page *dpage, int cpu)
 	u64 delta;
 	int tail;
 
-	tail = local_read(&dpage->commit);
+	tail = local_read(&dpage->commit) & ~RB_MISSED_MASK;
 	return rb_read_data_buffer(dpage, tail, cpu, &ts, &delta);
 }
 
@@ -1934,7 +1943,7 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 		goto invalid;
 	}
 	entries += ret;
-	entry_bytes += local_read(&cpu_buffer->reader_page->page->commit);
+	entry_bytes += rb_page_size(cpu_buffer->reader_page);
 	local_set(&cpu_buffer->reader_page->entries, ret);
 
 	ts = head_page->page->time_stamp;
@@ -2054,7 +2063,7 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 			local_inc(&cpu_buffer->pages_touched);
 
 		entries += ret;
-		entry_bytes += local_read(&head_page->page->commit);
+		entry_bytes += rb_page_size(head_page);
 		local_set(&cpu_buffer->head_page->entries, ret);
 
 		if (head_page == cpu_buffer->commit_page)
@@ -3256,12 +3265,6 @@ rb_iter_head_event(struct ring_buffer_iter *iter)
 	return NULL;
 }
 
-/* Size is determined by what has been committed */
-static __always_inline unsigned rb_page_size(struct buffer_page *bpage)
-{
-	return rb_page_commit(bpage) & ~RB_MISSED_MASK;
-}
-
 static __always_inline unsigned
 rb_commit_index(struct ring_buffer_per_cpu *cpu_buffer)
 {
@@ -4432,7 +4435,7 @@ static void check_buffer(struct ring_buffer_per_cpu *cpu_buffer,
 
 	if (tail == CHECK_FULL_PAGE) {
 		full = true;
-		tail = local_read(&bpage->commit);
+		tail = local_read(&bpage->commit) & ~RB_MISSED_MASK;
 	} else if (info->add_timestamp &
 		   (RB_ADD_STAMP_FORCE | RB_ADD_STAMP_ABSOLUTE)) {
 		/* Ignore events with absolute time stamps */

On Thu, 26 Feb 2026 22:38:43 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Since the MSBs of rb_data_page::commit are used for storing
> RB_MISSED_EVENTS and RB_MISSED_STORED, we need to mask out those bits
> when it is used for finding the size of data pages.
> 
> Fixes: 5f3b6e839f3c ("ring-buffer: Validate boot range memory events")
> Fixes: 5b7be9c709e1 ("ring-buffer: Add test to validate the time stamp deltas")
> Cc: stable@vger.kernel.org

This is unneeded for the current way things work.

The missed events flags are added when a page is read, so the commits in
the write buffer should never have those flags set. If they did, the ring
buffer code itself would break.

But as patch 3 is adding a flag, you should likely merge this and patch 3
together, as the only way that flag would get set is if the validator set
it on a previous boot. And then this would be needed for subsequent boots
that did not reset the buffer.

Hmm, I don't think we even need to do that! Because if it is set, it would
simply warn again that a page is invalid, and I think we *want* that! As it
would preserve that pages were invalid and not be cleared with a simple
reboot.

-- Steve

On Thu, 5 Mar 2026 13:03:48 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 26 Feb 2026 22:38:43 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Since the MSBs of rb_data_page::commit are used for storing
> > RB_MISSED_EVENTS and RB_MISSED_STORED, we need to mask out those bits
> > when it is used for finding the size of data pages.
> > 
> > Fixes: 5f3b6e839f3c ("ring-buffer: Validate boot range memory events")
> > Fixes: 5b7be9c709e1 ("ring-buffer: Add test to validate the time stamp deltas")
> > Cc: stable@vger.kernel.org
> 
> This is unneeded for the current way things work.
> 
> The missed events flags are added when a page is read, so the commits in
> the write buffer should never have those flags set. If they did, the ring
> buffer code itself would break.

Hmm, but commit ca296d32ece3 ("tracing: ring_buffer: Rewind persistent 
ring buffer on reboot") may change it. Maybe we should treat it while
unwinding it?

> 
> But as patch 3 is adding a flag, you should likely merge this and patch 3
> together, as the only way that flag would get set is if the validator set
> it on a previous boot. And then this would be needed for subsequent boots
> that did not reset the buffer.

It is OK to combine these 2 patches. But my question is that when the flag
must be checked and when it must be ignored. Since the flags are encoded
to commit, if that is used for limiting or indexing inside the page,
we must mask the flag or check the max size to avoid accessing outside of
the subpage.

> 
> Hmm, I don't think we even need to do that! Because if it is set, it would
> simply warn again that a page is invalid, and I think we *want* that! As it
> would preserve that pages were invalid and not be cleared with a simple
> reboot.

OK, then I don't mark it, but just invalidate the subpage.

Thanks,

> 
> -- Steve
> 


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

On Fri, 6 Mar 2026 17:46:09 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Thu, 5 Mar 2026 13:03:48 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Thu, 26 Feb 2026 22:38:43 +0900
> > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> >   
> > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > 
> > > Since the MSBs of rb_data_page::commit are used for storing
> > > RB_MISSED_EVENTS and RB_MISSED_STORED, we need to mask out those bits
> > > when it is used for finding the size of data pages.
> > > 
> > > Fixes: 5f3b6e839f3c ("ring-buffer: Validate boot range memory events")
> > > Fixes: 5b7be9c709e1 ("ring-buffer: Add test to validate the time stamp deltas")
> > > Cc: stable@vger.kernel.org  
> > 
> > This is unneeded for the current way things work.
> > 
> > The missed events flags are added when a page is read, so the commits in
> > the write buffer should never have those flags set. If they did, the ring
> > buffer code itself would break.  
> 
> Hmm, but commit ca296d32ece3 ("tracing: ring_buffer: Rewind persistent 
> ring buffer on reboot") may change it. Maybe we should treat it while
> unwinding it?

Might change what?

> 
> > 
> > But as patch 3 is adding a flag, you should likely merge this and patch 3
> > together, as the only way that flag would get set is if the validator set
> > it on a previous boot. And then this would be needed for subsequent boots
> > that did not reset the buffer.  
> 
> It is OK to combine these 2 patches. But my question is that when the flag
> must be checked and when it must be ignored. Since the flags are encoded
> to commit, if that is used for limiting or indexing inside the page,
> we must mask the flag or check the max size to avoid accessing outside of
> the subpage.
> 
> > 
> > Hmm, I don't think we even need to do that! Because if it is set, it would
> > simply warn again that a page is invalid, and I think we *want* that! As it
> > would preserve that pages were invalid and not be cleared with a simple
> > reboot.  
> 
> OK, then I don't mark it, but just invalidate the subpage.

No, I mean you can mark it, but then just have the validator skip it.
Something like:

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 156ed19fb569..c98ab86cf384 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -1950,6 +1951,10 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 	rb_dec_page(&head_page);
 	for (i = 0; i < meta->nr_subbufs + 1; i++, rb_dec_page(&head_page)) {
 
+		/* Skip if this buffer was flagged as bad before */
+		if (local_read(&head_page->page->commit) ==  RB_MISSED_EVENTS)
+			continue;
+
 		/* Rewind until tail (writer) page. */
 		if (head_page == cpu_buffer->tail_page)
 			break;

On Fri, 6 Mar 2026 09:59:35 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 6 Mar 2026 17:46:09 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > On Thu, 5 Mar 2026 13:03:48 -0500
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > On Thu, 26 Feb 2026 22:38:43 +0900
> > > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> > >   
> > > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > > 
> > > > Since the MSBs of rb_data_page::commit are used for storing
> > > > RB_MISSED_EVENTS and RB_MISSED_STORED, we need to mask out those bits
> > > > when it is used for finding the size of data pages.
> > > > 
> > > > Fixes: 5f3b6e839f3c ("ring-buffer: Validate boot range memory events")
> > > > Fixes: 5b7be9c709e1 ("ring-buffer: Add test to validate the time stamp deltas")
> > > > Cc: stable@vger.kernel.org  
> > > 
> > > This is unneeded for the current way things work.
> > > 
> > > The missed events flags are added when a page is read, so the commits in
> > > the write buffer should never have those flags set. If they did, the ring
> > > buffer code itself would break.  
> > 
> > Hmm, but commit ca296d32ece3 ("tracing: ring_buffer: Rewind persistent 
> > ring buffer on reboot") may change it. Maybe we should treat it while
> > unwinding it?
> 
> Might change what?

So the above commit removes clearing (resetting) meta->commit after read the
page (thus the missed events flags will be there). But those pages will be
recovered (rewound) after reboot. Thus the validation runs on those pages
(and detect invalid pages)

> 
> > 
> > > 
> > > But as patch 3 is adding a flag, you should likely merge this and patch 3
> > > together, as the only way that flag would get set is if the validator set
> > > it on a previous boot. And then this would be needed for subsequent boots
> > > that did not reset the buffer.  
> > 
> > It is OK to combine these 2 patches. But my question is that when the flag
> > must be checked and when it must be ignored. Since the flags are encoded
> > to commit, if that is used for limiting or indexing inside the page,
> > we must mask the flag or check the max size to avoid accessing outside of
> > the subpage.
> > 
> > > 
> > > Hmm, I don't think we even need to do that! Because if it is set, it would
> > > simply warn again that a page is invalid, and I think we *want* that! As it
> > > would preserve that pages were invalid and not be cleared with a simple
> > > reboot.  
> > 
> > OK, then I don't mark it, but just invalidate the subpage.
> 
> No, I mean you can mark it, but then just have the validator skip it.
> Something like:

This may not work because RB_MISSED_EVENTS are added, instead of set.
So the commit will be `original_commit + RB_MISSED_EVENTS`.

Anyway, in v8, any of invalid (out of range) commit value is treated
as corrupted in rb_validate_buffer().

Thanks,

> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 156ed19fb569..c98ab86cf384 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -1950,6 +1951,10 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
>  	rb_dec_page(&head_page);
>  	for (i = 0; i < meta->nr_subbufs + 1; i++, rb_dec_page(&head_page)) {
>  
> +		/* Skip if this buffer was flagged as bad before */
> +		if (local_read(&head_page->page->commit) ==  RB_MISSED_EVENTS)
> +			continue;
> +
>  		/* Rewind until tail (writer) page. */
>  		if (head_page == cpu_buffer->tail_page)
>  			break;
> 


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