Re: [PATCH] ring-buffer: Add barrire in rb_move_tail

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 5 Sep 2022 11:23:14 +0800
lijiazi <jqqlijiazi@gmail.com> wrote:

> >From ramdump, current reader page's commit is 0xff0, not bigger than  
> BUF_PAGE_SIZE:
> crash> struct buffer_page 0xffffffd10b599580 -x  
> struct buffer_page {
>   list = {
>     next = 0xffffffd10b599500,
>     prev = 0xffffffd10b599680
>   },
>   write = {
>     a = {
>       counter = 0x100ff0
>     }
>   },
>   read = 0xfd4,
>   entries = {
>     a = {
>       counter = 0x100053
>     }
>   },
>   real_end = 0xfd4,
>   page = 0xffffffd10b553000
> }
> crash> struct buffer_data_page 0xffffffd10b553000 -x  
> struct buffer_data_page {
>   time_stamp = 0xe2679ca0cd3d,
>   commit = {
>     a = {
>       counter = 0xff0
>     }
>   },
>   data = 0xffffffd10b553010 "\b"
> }
> I also can extrace trace log from ramdump by crash-trace extension tool:
> bsp: <...>-32191 [006] 249032.606401: signal_generate:      sig=17 errno=0 code=1 comm=WifiDiagnostics pid=1535 grp=1 res=1
> bsp: <...>-32183 [006] 249032.625192: sched_process_exit:   comm=osi_bin pid=32183 prio=120
> bsp: <...>-32196 [006] 249033.677333: sched_process_exit:   comm=ip pid=32196 prio=120
> bsp: <...>-32196 [006] 249033.677562: signal_generate:      sig=17 errno=0 code=1 comm=sh pid=32195 grp=1 res=0
> Above logs is on reader page, reader task try to read PADDING event
> after this event and lead to crash.

Ah, it's not an issue with the commit value but the write value.

Can you test this patch.

-- Steve

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index d59b6a328b7f..6bf7706bb33b 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -2608,6 +2608,9 @@ rb_reset_tail(struct ring_buffer_per_cpu *cpu_buffer,
 		/* Mark the rest of the page with padding */
 		rb_event_set_padding(event);
 
+		/* Make sure the padding is visible before the write update */
+		smp_wmb();
+
 		/* Set the write back to the previous setting */
 		local_sub(length, &tail_page->write);
 		return;
@@ -4580,6 +4583,13 @@ rb_get_reader_page(struct ring_buffer_per_cpu *cpu_buffer)
 	goto again;
 
  out:
+	/* If the write is past the end of page, a writer is still updating it */
+	if (reader && reader->write > BUF_PAGE_SIZE)
+		reader = NULL;
+
+	/* Make sure we see any padding after the write update */
+	smp_rmb();
+
 	/* Update the read_stamp on the first event */
 	if (reader && reader->read == 0)
 		cpu_buffer->read_stamp = reader->page->time_stamp;