Re: [PATCH v5 00/18] Rework READ_ONCE() to improve codegen

[Will Deacon]

Hi Marco,

On Thu, May 14, 2020 at 09:31:49AM +0200, Marco Elver wrote:
> Ouch. With the __{READ,WRITE}_ONCE requirement, we're going to need
> Clang 11 though.
> 
> Because without the data_race() around __*_ONCE,
> arch_atomic_{read,set} will be broken for KCSAN, but we can't have
> data_race() because it would still add
> kcsan_{enable,disable}_current() calls to __no_sanitize functions (if
> compilation unit is instrumented). We can't make arch_atomic functions
> __no_sanitize_or_inline, because even in code that we want to
> sanitize, they should remain __always_inline (so they work properly in
> __no_sanitize functions). Therefore, Clang 11 with support for
> distinguishing volatiles will be the compiler that will satisfy all
> the constraints.
> 
> If this is what we want, let me prepare a series on top of
> -tip/locking/kcsan with all the things I think we need.

Stepping back a second, the locking/kcsan branch is at least functional at
the moment by virtue of KCSAN_SANITIZE := n being used liberally in
arch/x86/. However, I still think we want to do better than that because (a)
it would be good to get more x86 coverage and (b) enabling this for arm64,
where objtool is not yet available, will be fragile if we have to whitelist
object files. There's also a fair bit of arm64 low-level code spread around
drivers/, so it feels like we'd end up with a really bad case of whack-a-mole.

Talking off-list, Clang >= 7 is pretty reasonable wrt inlining decisions
and the behaviour for __always_inline is:

  * An __always_inline function inlined into a __no_sanitize function is
    not instrumented
  * An __always_inline function inlined into an instrumented function is
    instrumented
  * You can't mark a function as both __always_inline __no_sanitize, because
    __no_sanitize functions are never inlined

GCC, on the other hand, may still inline __no_sanitize functions and then
subsequently instrument them.

So if were willing to make KCSAN depend on Clang >= 7, then we could:

  - Remove the data_race() from __{READ,WRITE}_ONCE()
  - Wrap arch_atomic*() in data_race() when called from the instrumented
    atomic wrappers

At which point, I *think* everything works as expected. READ_ONCE_NOCHECK()
won't generate any surprises, and Peter can happily use arch_atomic()
from non-instrumented code.

Thoughts? I don't see the need to support buggy compilers when enabling
a new debug feature.

Will