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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20250514155220.54584891585d57c317aa22ac@kernel.org>
Date: Wed, 14 May 2025 15:52:20 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Linux Trace Kernel
 <linux-trace-kernel@...r.kernel.org>, Masami Hiramatsu
 <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Tasos Sahanidis <tasos@...ossah.com>
Subject: Re: [PATCH] ring-buffer: Fix persistent buffer when commit page is
 the reader page

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

> \From: Steven Rostedt <rostedt@...dmis.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@...r.kernel.org
> Fixes: 5f3b6e839f3ce ("ring-buffer: Validate boot range memory events")
> Reported-by: Tasos Sahanidis <tasos@...ossah.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>

Looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@...nel.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@...nel.org>