[PATCH v7 0/2] ring-buffer: Making persistent ring buffers robust

[PATCH v7 0/2] ring-buffer: Making persistent ring buffers robust

[Masami Hiramatsu (Google)]

Hi,

Here is the 7th version of improvement patches for making persistent
ring buffers robust to failures. The previous version is here:

https://lore.kernel.org/all/177218401821.1988514.5579163042147205021.stgit@mhiramat.tok.corp.google.com/

In this version, I dropped Handle RB_MISSED_* flags patch and partially
include required change to the last one[2/2], it also removes a redundant
subbuffer data check in per-cpu metadata validation and do NOT mark
RB_MISSED_EVENTS bit on discarded pages.

Thank you,

---

Masami Hiramatsu (Google) (2):
      ring-buffer: Flush and stop persistent ring buffer on panic
      ring-buffer: Skip invalid sub-buffers when validating persistent ring buffer


 arch/alpha/include/asm/Kbuild        |    1 
 arch/arc/include/asm/Kbuild          |    1 
 arch/arm/include/asm/Kbuild          |    1 
 arch/arm64/include/asm/ring_buffer.h |   10 ++++
 arch/csky/include/asm/Kbuild         |    1 
 arch/hexagon/include/asm/Kbuild      |    1 
 arch/loongarch/include/asm/Kbuild    |    1 
 arch/m68k/include/asm/Kbuild         |    1 
 arch/microblaze/include/asm/Kbuild   |    1 
 arch/mips/include/asm/Kbuild         |    1 
 arch/nios2/include/asm/Kbuild        |    1 
 arch/openrisc/include/asm/Kbuild     |    1 
 arch/parisc/include/asm/Kbuild       |    1 
 arch/powerpc/include/asm/Kbuild      |    1 
 arch/riscv/include/asm/Kbuild        |    1 
 arch/s390/include/asm/Kbuild         |    1 
 arch/sh/include/asm/Kbuild           |    1 
 arch/sparc/include/asm/Kbuild        |    1 
 arch/um/include/asm/Kbuild           |    1 
 arch/x86/include/asm/Kbuild          |    1 
 arch/xtensa/include/asm/Kbuild       |    1 
 include/asm-generic/ring_buffer.h    |   13 +++++
 kernel/trace/ring_buffer.c           |   85 ++++++++++++++++++++++------------
 23 files changed, 98 insertions(+), 30 deletions(-)
 create mode 100644 arch/arm64/include/asm/ring_buffer.h
 create mode 100644 include/asm-generic/ring_buffer.h

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

[PATCH v7 1/2] ring-buffer: Flush and stop persistent ring buffer on panic

[Masami Hiramatsu (Google)]

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

On real hardware, panic and machine reboot may not flush hardware cache
to memory. This means the persistent ring buffer, which relies on a
coherent state of memory, may not have its events written to the buffer
and they may be lost. Moreover, there may be inconsistency with the
counters which are used for validation of the integrity of the
persistent ring buffer which may cause all data to be discarded.

To avoid this issue, stop recording of the ring buffer on panic and
flush the cache of the ring buffer's memory.

Fixes: e645535a954a ("tracing: Add option to use memmapped memory for trace boot instance")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 arch/alpha/include/asm/Kbuild        |    1 +
 arch/arc/include/asm/Kbuild          |    1 +
 arch/arm/include/asm/Kbuild          |    1 +
 arch/arm64/include/asm/ring_buffer.h |   10 ++++++++++
 arch/csky/include/asm/Kbuild         |    1 +
 arch/hexagon/include/asm/Kbuild      |    1 +
 arch/loongarch/include/asm/Kbuild    |    1 +
 arch/m68k/include/asm/Kbuild         |    1 +
 arch/microblaze/include/asm/Kbuild   |    1 +
 arch/mips/include/asm/Kbuild         |    1 +
 arch/nios2/include/asm/Kbuild        |    1 +
 arch/openrisc/include/asm/Kbuild     |    1 +
 arch/parisc/include/asm/Kbuild       |    1 +
 arch/powerpc/include/asm/Kbuild      |    1 +
 arch/riscv/include/asm/Kbuild        |    1 +
 arch/s390/include/asm/Kbuild         |    1 +
 arch/sh/include/asm/Kbuild           |    1 +
 arch/sparc/include/asm/Kbuild        |    1 +
 arch/um/include/asm/Kbuild           |    1 +
 arch/x86/include/asm/Kbuild          |    1 +
 arch/xtensa/include/asm/Kbuild       |    1 +
 include/asm-generic/ring_buffer.h    |   13 +++++++++++++
 kernel/trace/ring_buffer.c           |   22 ++++++++++++++++++++++
 23 files changed, 65 insertions(+)
 create mode 100644 arch/arm64/include/asm/ring_buffer.h
 create mode 100644 include/asm-generic/ring_buffer.h

diff --git a/arch/alpha/include/asm/Kbuild b/arch/alpha/include/asm/Kbuild
index 483965c5a4de..b154b4e3dfa8 100644
--- a/arch/alpha/include/asm/Kbuild
+++ b/arch/alpha/include/asm/Kbuild
@@ -5,4 +5,5 @@ generic-y += agp.h
 generic-y += asm-offsets.h
 generic-y += kvm_para.h
 generic-y += mcs_spinlock.h
+generic-y += ring_buffer.h
 generic-y += text-patching.h
diff --git a/arch/arc/include/asm/Kbuild b/arch/arc/include/asm/Kbuild
index 4c69522e0328..483caacc6988 100644
--- a/arch/arc/include/asm/Kbuild
+++ b/arch/arc/include/asm/Kbuild
@@ -5,5 +5,6 @@ generic-y += extable.h
 generic-y += kvm_para.h
 generic-y += mcs_spinlock.h
 generic-y += parport.h
+generic-y += ring_buffer.h
 generic-y += user.h
 generic-y += text-patching.h
diff --git a/arch/arm/include/asm/Kbuild b/arch/arm/include/asm/Kbuild
index 03657ff8fbe3..decad5f2c826 100644
--- a/arch/arm/include/asm/Kbuild
+++ b/arch/arm/include/asm/Kbuild
@@ -3,6 +3,7 @@ generic-y += early_ioremap.h
 generic-y += extable.h
 generic-y += flat.h
 generic-y += parport.h
+generic-y += ring_buffer.h
 
 generated-y += mach-types.h
 generated-y += unistd-nr.h
diff --git a/arch/arm64/include/asm/ring_buffer.h b/arch/arm64/include/asm/ring_buffer.h
new file mode 100644
index 000000000000..62316c406888
--- /dev/null
+++ b/arch/arm64/include/asm/ring_buffer.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _ASM_ARM64_RING_BUFFER_H
+#define _ASM_ARM64_RING_BUFFER_H
+
+#include <asm/cacheflush.h>
+
+/* Flush D-cache on persistent ring buffer */
+#define arch_ring_buffer_flush_range(start, end)	dcache_clean_pop(start, end)
+
+#endif /* _ASM_ARM64_RING_BUFFER_H */
diff --git a/arch/csky/include/asm/Kbuild b/arch/csky/include/asm/Kbuild
index 3a5c7f6e5aac..7dca0c6cdc84 100644
--- a/arch/csky/include/asm/Kbuild
+++ b/arch/csky/include/asm/Kbuild
@@ -9,6 +9,7 @@ generic-y += qrwlock.h
 generic-y += qrwlock_types.h
 generic-y += qspinlock.h
 generic-y += parport.h
+generic-y += ring_buffer.h
 generic-y += user.h
 generic-y += vmlinux.lds.h
 generic-y += text-patching.h
diff --git a/arch/hexagon/include/asm/Kbuild b/arch/hexagon/include/asm/Kbuild
index 1efa1e993d4b..0f887d4238ed 100644
--- a/arch/hexagon/include/asm/Kbuild
+++ b/arch/hexagon/include/asm/Kbuild
@@ -5,4 +5,5 @@ generic-y += extable.h
 generic-y += iomap.h
 generic-y += kvm_para.h
 generic-y += mcs_spinlock.h
+generic-y += ring_buffer.h
 generic-y += text-patching.h
diff --git a/arch/loongarch/include/asm/Kbuild b/arch/loongarch/include/asm/Kbuild
index 9034b583a88a..7e92957baf6a 100644
--- a/arch/loongarch/include/asm/Kbuild
+++ b/arch/loongarch/include/asm/Kbuild
@@ -10,5 +10,6 @@ generic-y += qrwlock.h
 generic-y += user.h
 generic-y += ioctl.h
 generic-y += mmzone.h
+generic-y += ring_buffer.h
 generic-y += statfs.h
 generic-y += text-patching.h
diff --git a/arch/m68k/include/asm/Kbuild b/arch/m68k/include/asm/Kbuild
index b282e0dd8dc1..62543bf305ff 100644
--- a/arch/m68k/include/asm/Kbuild
+++ b/arch/m68k/include/asm/Kbuild
@@ -3,5 +3,6 @@ generated-y += syscall_table.h
 generic-y += extable.h
 generic-y += kvm_para.h
 generic-y += mcs_spinlock.h
+generic-y += ring_buffer.h
 generic-y += spinlock.h
 generic-y += text-patching.h
diff --git a/arch/microblaze/include/asm/Kbuild b/arch/microblaze/include/asm/Kbuild
index 7178f990e8b3..0030309b47ad 100644
--- a/arch/microblaze/include/asm/Kbuild
+++ b/arch/microblaze/include/asm/Kbuild
@@ -5,6 +5,7 @@ generic-y += extable.h
 generic-y += kvm_para.h
 generic-y += mcs_spinlock.h
 generic-y += parport.h
+generic-y += ring_buffer.h
 generic-y += syscalls.h
 generic-y += tlb.h
 generic-y += user.h
diff --git a/arch/mips/include/asm/Kbuild b/arch/mips/include/asm/Kbuild
index 684569b2ecd6..9771c3d85074 100644
--- a/arch/mips/include/asm/Kbuild
+++ b/arch/mips/include/asm/Kbuild
@@ -12,5 +12,6 @@ generic-y += mcs_spinlock.h
 generic-y += parport.h
 generic-y += qrwlock.h
 generic-y += qspinlock.h
+generic-y += ring_buffer.h
 generic-y += user.h
 generic-y += text-patching.h
diff --git a/arch/nios2/include/asm/Kbuild b/arch/nios2/include/asm/Kbuild
index 28004301c236..0a2530964413 100644
--- a/arch/nios2/include/asm/Kbuild
+++ b/arch/nios2/include/asm/Kbuild
@@ -5,6 +5,7 @@ generic-y += cmpxchg.h
 generic-y += extable.h
 generic-y += kvm_para.h
 generic-y += mcs_spinlock.h
+generic-y += ring_buffer.h
 generic-y += spinlock.h
 generic-y += user.h
 generic-y += text-patching.h
diff --git a/arch/openrisc/include/asm/Kbuild b/arch/openrisc/include/asm/Kbuild
index cef49d60d74c..8aa34621702d 100644
--- a/arch/openrisc/include/asm/Kbuild
+++ b/arch/openrisc/include/asm/Kbuild
@@ -8,4 +8,5 @@ generic-y += spinlock_types.h
 generic-y += spinlock.h
 generic-y += qrwlock_types.h
 generic-y += qrwlock.h
+generic-y += ring_buffer.h
 generic-y += user.h
diff --git a/arch/parisc/include/asm/Kbuild b/arch/parisc/include/asm/Kbuild
index 4fb596d94c89..d48d158f7241 100644
--- a/arch/parisc/include/asm/Kbuild
+++ b/arch/parisc/include/asm/Kbuild
@@ -4,4 +4,5 @@ generated-y += syscall_table_64.h
 generic-y += agp.h
 generic-y += kvm_para.h
 generic-y += mcs_spinlock.h
+generic-y += ring_buffer.h
 generic-y += user.h
diff --git a/arch/powerpc/include/asm/Kbuild b/arch/powerpc/include/asm/Kbuild
index 2e23533b67e3..805b5aeebb6f 100644
--- a/arch/powerpc/include/asm/Kbuild
+++ b/arch/powerpc/include/asm/Kbuild
@@ -5,4 +5,5 @@ generated-y += syscall_table_spu.h
 generic-y += agp.h
 generic-y += mcs_spinlock.h
 generic-y += qrwlock.h
+generic-y += ring_buffer.h
 generic-y += early_ioremap.h
diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
index bd5fc9403295..7721b63642f4 100644
--- a/arch/riscv/include/asm/Kbuild
+++ b/arch/riscv/include/asm/Kbuild
@@ -14,5 +14,6 @@ generic-y += ticket_spinlock.h
 generic-y += qrwlock.h
 generic-y += qrwlock_types.h
 generic-y += qspinlock.h
+generic-y += ring_buffer.h
 generic-y += user.h
 generic-y += vmlinux.lds.h
diff --git a/arch/s390/include/asm/Kbuild b/arch/s390/include/asm/Kbuild
index 80bad7de7a04..0c1fc47c3ba0 100644
--- a/arch/s390/include/asm/Kbuild
+++ b/arch/s390/include/asm/Kbuild
@@ -7,3 +7,4 @@ generated-y += unistd_nr.h
 generic-y += asm-offsets.h
 generic-y += mcs_spinlock.h
 generic-y += mmzone.h
+generic-y += ring_buffer.h
diff --git a/arch/sh/include/asm/Kbuild b/arch/sh/include/asm/Kbuild
index 4d3f10ed8275..f0403d3ee8ab 100644
--- a/arch/sh/include/asm/Kbuild
+++ b/arch/sh/include/asm/Kbuild
@@ -3,4 +3,5 @@ generated-y += syscall_table.h
 generic-y += kvm_para.h
 generic-y += mcs_spinlock.h
 generic-y += parport.h
+generic-y += ring_buffer.h
 generic-y += text-patching.h
diff --git a/arch/sparc/include/asm/Kbuild b/arch/sparc/include/asm/Kbuild
index 17ee8a273aa6..49c6bb326b75 100644
--- a/arch/sparc/include/asm/Kbuild
+++ b/arch/sparc/include/asm/Kbuild
@@ -4,4 +4,5 @@ generated-y += syscall_table_64.h
 generic-y += agp.h
 generic-y += kvm_para.h
 generic-y += mcs_spinlock.h
+generic-y += ring_buffer.h
 generic-y += text-patching.h
diff --git a/arch/um/include/asm/Kbuild b/arch/um/include/asm/Kbuild
index 1b9b82bbe322..2a1629ba8140 100644
--- a/arch/um/include/asm/Kbuild
+++ b/arch/um/include/asm/Kbuild
@@ -17,6 +17,7 @@ generic-y += module.lds.h
 generic-y += parport.h
 generic-y += percpu.h
 generic-y += preempt.h
+generic-y += ring_buffer.h
 generic-y += runtime-const.h
 generic-y += softirq_stack.h
 generic-y += switch_to.h
diff --git a/arch/x86/include/asm/Kbuild b/arch/x86/include/asm/Kbuild
index 4566000e15c4..078fd2c0d69d 100644
--- a/arch/x86/include/asm/Kbuild
+++ b/arch/x86/include/asm/Kbuild
@@ -14,3 +14,4 @@ generic-y += early_ioremap.h
 generic-y += fprobe.h
 generic-y += mcs_spinlock.h
 generic-y += mmzone.h
+generic-y += ring_buffer.h
diff --git a/arch/xtensa/include/asm/Kbuild b/arch/xtensa/include/asm/Kbuild
index 13fe45dea296..e57af619263a 100644
--- a/arch/xtensa/include/asm/Kbuild
+++ b/arch/xtensa/include/asm/Kbuild
@@ -6,5 +6,6 @@ generic-y += mcs_spinlock.h
 generic-y += parport.h
 generic-y += qrwlock.h
 generic-y += qspinlock.h
+generic-y += ring_buffer.h
 generic-y += user.h
 generic-y += text-patching.h
diff --git a/include/asm-generic/ring_buffer.h b/include/asm-generic/ring_buffer.h
new file mode 100644
index 000000000000..dbe94f5785f9
--- /dev/null
+++ b/include/asm-generic/ring_buffer.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Generiuc arch dependent ring_buffer macros.
+ */
+#ifndef __ASM_GENERIC_RING_BUFFER_H__
+#define __ASM_GENERIC_RING_BUFFER_H__
+
+#include <linux/cacheflush.h>
+
+/* Flush cache on ring buffer range if needed */
+#define arch_ring_buffer_flush_range(start, end)	flush_cache_vmap(start, end)
+
+#endif /* __ASM_GENERIC_RING_BUFFER_H__ */
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 17d0ea0cc3e6..b6f3ac99834f 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -6,6 +6,7 @@
  */
 #include <linux/sched/isolation.h>
 #include <linux/trace_recursion.h>
+#include <linux/panic_notifier.h>
 #include <linux/trace_events.h>
 #include <linux/ring_buffer.h>
 #include <linux/trace_clock.h>
@@ -30,6 +31,7 @@
 #include <linux/oom.h>
 #include <linux/mm.h>
 
+#include <asm/ring_buffer.h>
 #include <asm/local64.h>
 #include <asm/local.h>
 #include <asm/setup.h>
@@ -589,6 +591,7 @@ struct trace_buffer {
 
 	unsigned long			range_addr_start;
 	unsigned long			range_addr_end;
+	struct notifier_block		flush_nb;
 
 	struct ring_buffer_meta		*meta;
 
@@ -2471,6 +2474,16 @@ static void rb_free_cpu_buffer(struct ring_buffer_per_cpu *cpu_buffer)
 	kfree(cpu_buffer);
 }
 
+/* Stop recording on a persistent buffer and flush cache if needed. */
+static int rb_flush_buffer_cb(struct notifier_block *nb, unsigned long event, void *data)
+{
+	struct trace_buffer *buffer = container_of(nb, struct trace_buffer, flush_nb);
+
+	ring_buffer_record_off(buffer);
+	arch_ring_buffer_flush_range(buffer->range_addr_start, buffer->range_addr_end);
+	return NOTIFY_DONE;
+}
+
 static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
 					 int order, unsigned long start,
 					 unsigned long end,
@@ -2590,6 +2603,12 @@ static struct trace_buffer *alloc_buffer(unsigned long size, unsigned flags,
 
 	mutex_init(&buffer->mutex);
 
+	/* Persistent ring buffer needs to flush cache before reboot. */
+	if (start & end) {
+		buffer->flush_nb.notifier_call = rb_flush_buffer_cb;
+		atomic_notifier_chain_register(&panic_notifier_list, &buffer->flush_nb);
+	}
+
 	return_ptr(buffer);
 
  fail_free_buffers:
@@ -2677,6 +2696,9 @@ ring_buffer_free(struct trace_buffer *buffer)
 {
 	int cpu;
 
+	if (buffer->range_addr_start && buffer->range_addr_end)
+		atomic_notifier_chain_unregister(&panic_notifier_list, &buffer->flush_nb);
+
 	cpuhp_state_remove_instance(CPUHP_TRACE_RB_PREPARE, &buffer->node);
 
 	irq_work_sync(&buffer->irq_work.work);

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

[Masami Hiramatsu (Google)]

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

Skip invalid sub-buffers when validating the persistent ring buffer
instead of discarding the entire ring buffer. Only skipped buffers
are invalidated (cleared).

If the cache data in memory fails to be synchronized during a reboot,
the persistent ring buffer may become partially corrupted, but other
sub-buffers may still contain readable event data. Only discard the
subbuffersa that ar found to be corrupted.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
  Changes in v7:
  - Combined with Handling RB_MISSED_* flags patch, focus on validation at boot.
  - Remove checking subbuffer data when validating metadata, because it should be done
    later.
  - Do not mark the discarded sub buffer page but just reset it.
  Changes in v6:
  - Show invalid page detection message once per CPU.
  Changes in v5:
  - Instead of showing errors for each page, just show the number
    of discarded pages at last.
  Changes in v3:
  - Record missed data event on commit.
---
 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? */
 	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;
-		}
-
 		if (test_bit(meta->buffers[i], subbuf_mask)) {
 			pr_info("Ring buffer boot meta [%d] array has duplicates\n", cpu);
 			return false;
@@ -1902,13 +1903,16 @@ static int rb_read_data_buffer(struct buffer_data_page *dpage, int tail, int cpu
 	return events;
 }
 
-static int rb_validate_buffer(struct buffer_data_page *dpage, int cpu)
+static int rb_validate_buffer(struct buffer_data_page *dpage, int cpu,
+			      struct ring_buffer_cpu_meta *meta)
 {
 	unsigned long long ts;
 	u64 delta;
 	int tail;
 
 	tail = local_read(&dpage->commit);
+	if (tail <= 0 || tail > meta->subbuf_size)
+		return -1;
 	return rb_read_data_buffer(dpage, tail, cpu, &ts, &delta);
 }
 
@@ -1919,6 +1923,7 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 	struct buffer_page *head_page, *orig_head;
 	unsigned long entry_bytes = 0;
 	unsigned long entries = 0;
+	int discarded = 0;
 	int ret;
 	u64 ts;
 	int i;
@@ -1929,13 +1934,13 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 	orig_head = head_page = cpu_buffer->head_page;
 
 	/* Do the reader page first */
-	ret = rb_validate_buffer(cpu_buffer->reader_page->page, cpu_buffer->cpu);
+	ret = rb_validate_buffer(cpu_buffer->reader_page->page, cpu_buffer->cpu, meta);
 	if (ret < 0) {
 		pr_info("Ring buffer reader page is invalid\n");
 		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;
@@ -1964,7 +1969,7 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 			break;
 
 		/* Stop rewind if the page is invalid. */
-		ret = rb_validate_buffer(head_page->page, cpu_buffer->cpu);
+		ret = rb_validate_buffer(head_page->page, cpu_buffer->cpu, meta);
 		if (ret < 0)
 			break;
 
@@ -2043,21 +2048,24 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 		if (head_page == cpu_buffer->reader_page)
 			continue;
 
-		ret = rb_validate_buffer(head_page->page, cpu_buffer->cpu);
+		ret = rb_validate_buffer(head_page->page, cpu_buffer->cpu, meta);
 		if (ret < 0) {
-			pr_info("Ring buffer meta [%d] invalid buffer page\n",
-				cpu_buffer->cpu);
-			goto invalid;
-		}
-
-		/* If the buffer has content, update pages_touched */
-		if (ret)
-			local_inc(&cpu_buffer->pages_touched);
-
-		entries += ret;
-		entry_bytes += local_read(&head_page->page->commit);
-		local_set(&cpu_buffer->head_page->entries, ret);
+			if (!discarded)
+				pr_info("Ring buffer meta [%d] invalid buffer page detected\n",
+					cpu_buffer->cpu);
+			discarded++;
+			/* Instead of discard whole ring buffer, discard only this sub-buffer. */
+			local_set(&head_page->entries, 0);
+			local_set(&head_page->page->commit, RB_MISSED_EVENTS);
+		} else {
+			/* If the buffer has content, update pages_touched */
+			if (ret)
+				local_inc(&cpu_buffer->pages_touched);
 
+			entries += ret;
+			entry_bytes += rb_page_size(head_page);
+			local_set(&cpu_buffer->head_page->entries, ret);
+		}
 		if (head_page == cpu_buffer->commit_page)
 			break;
 	}
@@ -2071,7 +2079,8 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 	local_set(&cpu_buffer->entries, entries);
 	local_set(&cpu_buffer->entries_bytes, entry_bytes);
 
-	pr_info("Ring buffer meta [%d] is from previous boot!\n", cpu_buffer->cpu);
+	pr_info("Ring buffer meta [%d] is from previous boot! (%d pages discarded)\n",
+		cpu_buffer->cpu, discarded);
 	return;
 
  invalid:
@@ -3258,12 +3267,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)
 {

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

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

[Masami Hiramatsu]

On Sat, 7 Mar 2026 10:27:11 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat,  7 Mar 2026 23:26:38 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> 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.

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

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

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

[Masami Hiramatsu]

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

> On Sat, 7 Mar 2026 10:27:11 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Sat,  7 Mar 2026 23:26:38 +0900
> > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> 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@kernel.org>


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

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

[Steven Rostedt]

On Mon, 9 Mar 2026 09:53:07 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

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

Sure.

-- Steve

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

[Steven Rostedt]

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

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

Hmm, OK.

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

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.

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

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

[Masami Hiramatsu]

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>

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

[Steven Rostedt]

On Mon, 9 Mar 2026 22:32:44 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

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

And that shouldn't be a problem.

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

Yes, and since those flags are only set by the validator, if the validator
sees them set, it should skip processing the subbuffer. It knows it was
already flagged as corrupt.

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

But it shouldn't be.  The only way the RB_MISSED flag gets set in the
writer portion is if on a previous boot the validator detected that the
subbuffer was corrupted. If they are set for any other reason, the
subbuffer should be considered corrupted anyway.

-- Steve