On Wed, Mar 8, 2017 at 6:43 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Mar 08, 2017 at 03:20:41PM +0000, Mark Rutland wrote:
>> On Wed, Mar 08, 2017 at 02:42:10PM +0100, Dmitry Vyukov wrote:
>> > I think if we scope compiler atomic builtins to KASAN/KTSAN/KMSAN (and
>> > consequently x86/arm64) initially, it becomes more realistic. For the
>> > tools we don't care about absolute efficiency and this gets rid of
>> > Will's points (2), (4) and (6) here https://lwn.net/Articles/691295/.
>> > Re (3) I think rmb/wmb can be reasonably replaced with
>> > atomic_thread_fence(acquire/release). Re (5) situation with
>> > correctness becomes better very quickly as more people use them in
>> > user-space. Since KASAN is not intended to be used in production (or
>> > at least such build is expected to crash), we can afford to shake out
>> > any remaining correctness issues in such build. (1) I don't fully
>> > understand, what exactly is the problem with seq_cst?
>>
>> I'll have to leave it to Will to have the final word on these; I'm
>> certainly not familiar enough with the C11 memory model to comment on
>> (1).
>
> rmb()/wmb() are not remotely similar to
> atomic_thread_fenc_{acquire,release}, even if you restrict ordering to
> coherent CPUs (i.e. the smp_* variants). Please don't do that :)
>
> I'm also terrified of the optimisations that the compiler is theoretically
> allowed to make to C11 atomics given the assumptions of the language
> virtual machine, which are not necessarily valid in the kernel environment.
> We would at least need well-supported compiler options to disable these
> options, and also to allow data races with things like READ_ONCE.

Hello,

I've prototyped what Mark suggested:
 - prefix arch atomics with arch_
 - add <asm-generic/atomic-instrumented.h> which defined atomics and
forwards to the arch_ version

Patch attached. It boot with/without KASAN.

Does it look reasonable to you?

If so, I will split it into:
 - minor kasan patch that adds volatile to kasan_check_read/write
 - main patch that adds arch_ prefix and
<asm-generic/atomic-instrumented.h> header
 - kasan instrumentation in <asm-generic/atomic-instrumented.h>

Any other suggestions?