[PATCH] ring-buffer: Fix persistent buffer when commit page is the reader page

[PATCH] ring-buffer: Fix persistent buffer when commit page is the reader page

[Steven Rostedt]

\From: Steven Rostedt <rostedt@goodmis.org>

The ring buffer is made up of sub buffers (sometimes called pages as they
are by default PAGE_SIZE). It has the following "pages":

  "tail page" - this is the page that the next write will write to
  "head page" - this is the page that the reader will swap the reader page with.
  "reader page" - This belongs to the reader, where it will swap the head
                  page from the ring buffer so that the reader does not
                  race with the writer.

The writer may end up on the "reader page" if the ring buffer hasn't
written more than one page, where the "tail page" and the "head page" are
the same.

The persistent ring buffer has meta data that points to where these pages
exist so on reboot it can re-create the pointers to the cpu_buffer
descriptor. But when the commit page is on the reader page, the logic is
incorrect.

The check to see if the commit page is on the reader page checked if the
head page was the reader page, which would never happen, as the head page
is always in the ring buffer. The correct check would be to test if the
commit page is on the reader page. If that's the case, then it can exit
out early as the commit page is only on the reader page when there's only
one page of data in the buffer. There's no reason to iterate the ring
buffer pages to find the "commit page" as it is already found.

To trigger this bug:

  # echo 1 > /sys/kernel/tracing/instances/boot_mapped/events/syscalls/sys_enter_fchownat/enable
  # touch /tmp/x
  # chown sshd /tmp/x
  # reboot

On boot up, the dmesg will have:
 Ring buffer meta [0] is from previous boot!
 Ring buffer meta [1] is from previous boot!
 Ring buffer meta [2] is from previous boot!
 Ring buffer meta [3] is from previous boot!
 Ring buffer meta [4] commit page not found
 Ring buffer meta [5] is from previous boot!
 Ring buffer meta [6] is from previous boot!
 Ring buffer meta [7] is from previous boot!

Where the buffer on CPU 4 had a "commit page not found" error and that
buffer is cleared and reset causing the output to be empty and the data lost.

When it works correctly, it has:

  # cat /sys/kernel/tracing/instances/boot_mapped/trace_pipe
        <...>-1137    [004] .....   998.205323: sys_enter_fchownat: __syscall_nr=0x104 (260) dfd=0xffffff9c (4294967196) filename=(0xffffc90000a0002c) user=0x3e8 (1000) group=0xffffffff (4294967295) flag=0x0 (0

Cc: stable@vger.kernel.org
Fixes: 5f3b6e839f3ce ("ring-buffer: Validate boot range memory events")
Reported-by: Tasos Sahanidis <tasos@tasossah.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ring_buffer.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 1ca482955dae..6859008ca34d 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -1887,10 +1887,12 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
 
 	head_page = cpu_buffer->head_page;
 
-	/* If both the head and commit are on the reader_page then we are done. */
-	if (head_page == cpu_buffer->reader_page &&
-	    head_page == cpu_buffer->commit_page)
+	/* If the commit_buffer is the reader page, update the commit page */
+	if (meta->commit_buffer == (unsigned long)cpu_buffer->reader_page->page) {
+		cpu_buffer->commit_page = cpu_buffer->reader_page;
+		/* Nothing more to do, the only page is the reader page */
 		goto done;
+	}
 
 	/* Iterate until finding the commit page */
 	for (i = 0; i < meta->nr_subbufs + 1; i++, rb_inc_page(&head_page)) {
-- 
2.47.2

Re: [PATCH] ring-buffer: Fix persistent buffer when commit page is the reader page

[Tasos Sahanidis]

On 2025-05-13 18:50, Steven Rostedt wrote:
> To trigger this bug:
> 
>   # echo 1 > /sys/kernel/tracing/instances/boot_mapped/events/syscalls/sys_enter_fchownat/enable
>   # touch /tmp/x
>   # chown sshd /tmp/x
>   # reboot
> 
> On boot up, the dmesg will have:
>  Ring buffer meta [0] is from previous boot!
>  Ring buffer meta [1] is from previous boot!
>  Ring buffer meta [2] is from previous boot!
>  Ring buffer meta [3] is from previous boot!
>  Ring buffer meta [4] commit page not found
>  Ring buffer meta [5] is from previous boot!
>  Ring buffer meta [6] is from previous boot!
>  Ring buffer meta [7] is from previous boot!
> 
> Where the buffer on CPU 4 had a "commit page not found" error and that
> buffer is cleared and reset causing the output to be empty and the data lost.
> 
> When it works correctly, it has:
> 
>   # cat /sys/kernel/tracing/instances/boot_mapped/trace_pipe
>         <...>-1137    [004] .....   998.205323: sys_enter_fchownat: __syscall_nr=0x104 (260) dfd=0xffffff9c (4294967196) filename=(0xffffc90000a0002c) user=0x3e8 (1000) group=0xffffffff (4294967295) flag=0x0 (0
> 
Tested-by: Tasos Sahanidis <tasos@tasossah.com>

Patch resolves the issue on my end. Cheers!

Re: [PATCH] ring-buffer: Fix persistent buffer when commit page is the reader page

[Masami Hiramatsu]

On Tue, 13 May 2025 11:50:32 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> \From: Steven Rostedt <rostedt@goodmis.org>
> 
> The ring buffer is made up of sub buffers (sometimes called pages as they
> are by default PAGE_SIZE). It has the following "pages":
> 
>   "tail page" - this is the page that the next write will write to
>   "head page" - this is the page that the reader will swap the reader page with.
>   "reader page" - This belongs to the reader, where it will swap the head
>                   page from the ring buffer so that the reader does not
>                   race with the writer.
> 
> The writer may end up on the "reader page" if the ring buffer hasn't
> written more than one page, where the "tail page" and the "head page" are
> the same.
> 
> The persistent ring buffer has meta data that points to where these pages
> exist so on reboot it can re-create the pointers to the cpu_buffer
> descriptor. But when the commit page is on the reader page, the logic is
> incorrect.
> 
> The check to see if the commit page is on the reader page checked if the
> head page was the reader page, which would never happen, as the head page
> is always in the ring buffer. The correct check would be to test if the
> commit page is on the reader page. If that's the case, then it can exit
> out early as the commit page is only on the reader page when there's only
> one page of data in the buffer. There's no reason to iterate the ring
> buffer pages to find the "commit page" as it is already found.
> 
> To trigger this bug:
> 
>   # echo 1 > /sys/kernel/tracing/instances/boot_mapped/events/syscalls/sys_enter_fchownat/enable
>   # touch /tmp/x
>   # chown sshd /tmp/x
>   # reboot
> 
> On boot up, the dmesg will have:
>  Ring buffer meta [0] is from previous boot!
>  Ring buffer meta [1] is from previous boot!
>  Ring buffer meta [2] is from previous boot!
>  Ring buffer meta [3] is from previous boot!
>  Ring buffer meta [4] commit page not found
>  Ring buffer meta [5] is from previous boot!
>  Ring buffer meta [6] is from previous boot!
>  Ring buffer meta [7] is from previous boot!
> 
> Where the buffer on CPU 4 had a "commit page not found" error and that
> buffer is cleared and reset causing the output to be empty and the data lost.
> 
> When it works correctly, it has:
> 
>   # cat /sys/kernel/tracing/instances/boot_mapped/trace_pipe
>         <...>-1137    [004] .....   998.205323: sys_enter_fchownat: __syscall_nr=0x104 (260) dfd=0xffffff9c (4294967196) filename=(0xffffc90000a0002c) user=0x3e8 (1000) group=0xffffffff (4294967295) flag=0x0 (0
> 
> Cc: stable@vger.kernel.org
> Fixes: 5f3b6e839f3ce ("ring-buffer: Validate boot range memory events")
> Reported-by: Tasos Sahanidis <tasos@tasossah.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Looks good to me.

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

Thanks,

> ---
>  kernel/trace/ring_buffer.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 1ca482955dae..6859008ca34d 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -1887,10 +1887,12 @@ static void rb_meta_validate_events(struct ring_buffer_per_cpu *cpu_buffer)
>  
>  	head_page = cpu_buffer->head_page;
>  
> -	/* If both the head and commit are on the reader_page then we are done. */
> -	if (head_page == cpu_buffer->reader_page &&
> -	    head_page == cpu_buffer->commit_page)
> +	/* If the commit_buffer is the reader page, update the commit page */
> +	if (meta->commit_buffer == (unsigned long)cpu_buffer->reader_page->page) {
> +		cpu_buffer->commit_page = cpu_buffer->reader_page;
> +		/* Nothing more to do, the only page is the reader page */
>  		goto done;
> +	}
>  
>  	/* Iterate until finding the commit page */
>  	for (i = 0; i < meta->nr_subbufs + 1; i++, rb_inc_page(&head_page)) {
> -- 
> 2.47.2
> 


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

Re: [PATCH] ring-buffer: Fix persistent buffer when commit page is the reader page

[Steven Rostedt]

On Wed, 14 May 2025 09:20:12 +0300
Tasos Sahanidis <tasos@tasossah.com> wrote:

> > When it works correctly, it has:
> > 
> >   # cat /sys/kernel/tracing/instances/boot_mapped/trace_pipe
> >         <...>-1137    [004] .....   998.205323: sys_enter_fchownat: __syscall_nr=0x104 (260) dfd=0xffffff9c (4294967196) filename=(0xffffc90000a0002c) user=0x3e8 (1000) group=0xffffffff (4294967295) flag=0x0 (0
> >   
> Tested-by: Tasos Sahanidis <tasos@tasossah.com>
> 
> Patch resolves the issue on my end. Cheers!


Thanks for validating!

-- Steve