Re: [PATCH v3 7/7] asm-generic, x86: add comments for atomic instrumentation

[Dmitry Vyukov]

On Fri, Jun 16, 2017 at 6:29 PM, Andrey Ryabinin
<aryabinin@xxxxxxxxxxxxx> wrote:
> On 06/06/2017 01:11 PM, Dmitry Vyukov wrote:
>> The comments are factored out from the code changes to make them
>> easier to read. Add them separately to explain some non-obvious
>> aspects.
>>
>> Signed-off-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> Cc: Will Deacon <will.deacon@xxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Cc: kasan-dev@xxxxxxxxxxxxxxxx
>> Cc: linux-mm@xxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Cc: x86@xxxxxxxxxx
>> ---
>>  arch/x86/include/asm/atomic.h             |  7 +++++++
>>  include/asm-generic/atomic-instrumented.h | 30 ++++++++++++++++++++++++++++++
>>  2 files changed, 37 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/atomic.h b/arch/x86/include/asm/atomic.h
>> index b7900346c77e..8a9e65e585db 100644
>> --- a/arch/x86/include/asm/atomic.h
>> +++ b/arch/x86/include/asm/atomic.h
>> @@ -23,6 +23,13 @@
>>   */
>>  static __always_inline int arch_atomic_read(const atomic_t *v)
>>  {
>> +     /*
>> +      * Note: READ_ONCE() here leads to double instrumentation as
>> +      * both READ_ONCE() and atomic_read() contain instrumentation.
>> +      * This is a deliberate choice. READ_ONCE_NOCHECK() is compiled to a
>> +      * non-inlined function call that considerably increases binary size
>> +      * and stack usage under KASAN.
>> +      */
>
>
> Not sure that this worth commenting. Whoever is looking into arch_atomic_read() internals
> probably don't even think about KASAN instrumentation, so I'd remove this comment.


My concern is that if somebody does think about KASAN, he/she will
conclude that it should be READ_ONCE_NOCHECK(). And that's what I did
initially, until you pointed the negative effects. I don't like
pointless comments that repeat code too, but this one explains
something that basically cannot be inferred from source code.

I've made it clear that it's for KASAN and also significantly shorten
it so that it does not obscure code too much. Now it is:

/*
 * Note for KASAN: we deliberately don't use READ_ONCE_NOCHECK() here,
 * it's non-inlined function that increases binary size and stack usage.
 */

Does it look better to you?

I've mailed v4 with the 2 fixed.

Thanks for review!


>>       return READ_ONCE((v)->counter);
>>  }
>>