ring-buffer: Making persistent ring buffers robust

 kernel/trace/ring_buffer.c |   63 +++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 30 deletions(-)

kernel/trace/ring_buffer.c +6 -0

index b6f3ac99834f..8599de5cf59b 100644
@@ -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 */

Quoted 21 lines · On Sat, 7 Mar 2026 23:26:38 +0900 "Masami Hiramatsu (Google)"…

 kernel/trace/ring_buffer.c |   63 +++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 30 deletions(-)

kernel/trace/ring_buffer.c +6 -0

index b6f3ac99834f..8599de5cf59b 100644
@@ -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 */

kernel/trace/ring_buffer.c static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu, · line 1828

Quoted 5 lines

kernel/trace/ring_buffer.c +1 -1

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

Quoted 8 lines

kernel/trace/ring_buffer.c +0 -0

 	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;
 		}
 

kernel/trace/ring_buffer.c static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu, · line 1833

Quoted 4 lines

kernel/trace/ring_buffer.c +0 -4

-		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

Quoted 33 lines · On Sat, 7 Mar 2026 10:27:11 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 21 lines · On Sat, 7 Mar 2026 23:26:38 +0900 "Masami Hiramatsu (Google)"…

 kernel/trace/ring_buffer.c |   63 +++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 30 deletions(-)

kernel/trace/ring_buffer.c +6 -0

index b6f3ac99834f..8599de5cf59b 100644
@@ -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 */

kernel/trace/ring_buffer.c static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu, · line 1828

Quoted 5 lines

kernel/trace/ring_buffer.c +1 -1

@@ -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?

Quoted 3 lines

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.

Quoted 9 lines

Quoted 9 lines

kernel/trace/ring_buffer.c +0 -0

 	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;
 		}
 

Quoted 7 lines

kernel/trace/ring_buffer.c static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu, · line 1833

Quoted 4 lines

kernel/trace/ring_buffer.c +0 -4

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

Quoted 3 lines

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

Quoted 5 lines

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.

(quoted diff) +0 -1

@@ -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?

Thanks,

Quoted 6 lines

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

-- Steve

Signature · 1 line

Masami Hiramatsu (Google) <[email protected]>

Quoted 10 lines

Quoted 7 lines

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.

Quoted 11 lines

kernel/trace/ring_buffer.c static bool rb_cpu_meta_valid(struct ring_buffer_cpu_meta *meta, int cpu, · line 1833

Quoted 8 lines

kernel/trace/ring_buffer.c +0 -4

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

Quoted 4 lines

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

Quoted 1 line

Quoted 4 lines

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.

Quoted 12 lines · On Mon, 9 Mar 2026 08:53:17 +0900 Masami Hiramatsu (Google)…

Quoted 2 lines

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

Quoted 5 lines

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

Quoted 2 lines

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.

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.

Quoted 6 lines

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.

-- Steve

Quoted 4 lines

Quoted 2 lines

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

Quoted 4 lines

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.

Quoted 10 lines

Quoted 7 lines

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.