linux-kernel - Re: [PATCH] ring-buffer: use READ_ONCE() to read cpu_buffer->commit

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] [day] [month] [year] [list]

Date: Thu, 29 Feb 2024 10:21:19 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: linke <lilinke99@...com>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 mathieu.desnoyers@...icios.com, mhiramat@...nel.org
Subject: Re: [PATCH] ring-buffer: use READ_ONCE() to read
 cpu_buffer->commit_page in concurrent environment

On Thu, 29 Feb 2024 20:32:26 +0800
linke <lilinke99@...com> wrote:

> Hi Steven, sorry for the late reply.
> 
> > 
> > Now the reason for the above READ_ONCE() is because the variables *are*
> > going to be used again. We do *not* want the compiler to play any games
> > with that.
> >   
> 
> I don't think it is because the variables are going to be used again. 
> Compiler optimizations barely do bad things in single thread programs. It
> is because cpu_buffer->commit_page may change concurrently and should be
> accessed atomically.

So basically you are worried about read-tearing?

That wasn't mentioned in the change log.

> 
> 	/* Make sure commit page didn't change */
> 	curr_commit_page = READ_ONCE(cpu_buffer->commit_page);
> 	curr_commit_ts = READ_ONCE(curr_commit_page->page->time_stamp);
> 
> 	/* If the commit page changed, then there's more data */
> 	if (curr_commit_page != commit_page ||
> 	    curr_commit_ts != commit_ts)
> 		return 0;
> 
> This code read cpu_buffer->commit_page and time_stamp again to check
> whether commit page changed. It shows that cpu_buffer->commit_page and 
> time_stamp may be changed by other threads.
> 
>         commit_page = cpu_buffer->commit_page;
>         commit_ts = commit_page->page->time_stamp;
> 
> So the commit_page and time_stamp above is read while other threads may
> change it. I think it is a data race if it is not atomic. Thus it is 
> necessary to use READ_ONCE() here.

Funny part is, if the above timestamp read did a tear, then this would
definitely not match, and would return the correct value. That is, the
buffer is not empty because the only way for this to get corrupted is if
something is in the process of writing to it.

Now we could add a comment stating this.

So, I don't even think the reading of the commit_page is needed (it's long
size so it should not tear, and if it does, I consider that more a bug in
the compiler).

Please explain why READ_ONCE() is needed, and what exactly is it "fixing".
That is, what breaks if it's not there?

-- Steve