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

Date: Fri, 1 Mar 2024 11:37:54 -0500
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Steven Rostedt <rostedt@...dmis.org>, linke <lilinke99@...com>,
 paulmck <paulmck@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 mhiramat@...nel.org, Peter Zijlstra <peterz@...radead.org>,
 Josh Triplett <josh@...htriplett.org>,
 Linus Torvalds <torvalds@...ux-foundation.org>,
 Andrew Morton <akpm@...ux-foundation.org>, Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH] ring-buffer: use READ_ONCE() to read
 cpu_buffer->commit_page in concurrent environment

On 2024-03-01 10:49, Steven Rostedt wrote:
> On Fri, 1 Mar 2024 13:37:18 +0800
> linke <lilinke99@...com> wrote:
> 
>>> So basically you are worried about read-tearing?
>>>
>>> That wasn't mentioned in the change log.
>>
>> Yes. Sorry for making this confused, I am not very familiar with this and
>> still learning.
> 
> No problem. We all have to learn this anyway.
> 
>>
>>> 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.
>>
>> I agree with you here.
>>
>> 	commit = rb_page_commit(commit_page);
>>
>> But if commit_page above is the result of a torn read, the commit field
>> read by rb_page_commit() may not represent a valid value.
> 
> But commit_page is a word length, and I will argue that any compiler that
> tears "long" words is broken. ;-)

[ For those tuning in, we are discussing ring_buffer_iter_empty()
   "commit_page = cpu_buffer->commit_page;" racy load. ]

I counter-argue that real-world compilers *are* broken based on your
personal definition, but we have to deal with them, as documented
in Documentation/memory-barriers.txt (see below).

What is the added overhead of using a READ_ONCE() there ? Why are
we wasting effort trying to guess the compiler behavior if the
real-world performance impact is insignificant ?

Quote from memory-barrier.txt explaining the purpose of {READ,WRITE}_ONCE():

"(*) For aligned memory locations whose size allows them to be accessed
      with a single memory-reference instruction, prevents "load tearing"
      and "store tearing," in which a single large access is replaced by
      multiple smaller accesses."

I agree that {READ,WRITE}_ONCE() are really not needed at initialization,
when there are demonstrably no concurrent accesses to the data

But trying to eliminate {READ,WRITE}_ONCE() on concurrently accessed fields
just adds complexity, prevents static analyzers to properly understand the
code and report issues, and just obfuscates the code.

Thanks,

Mathieu

> 
>>
>> In this case, READ_ONCE() is only needed for the commit_page.
> 
> But we can at least keep the READ_ONCE() on the commit_page just because it
> is used in the next instruction.
> 
> -- Steve

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com