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

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]

Message-ID: <CACT4Y+YMUghcyjweQPtG9m4W=AJ=raSAz=7MqeFYc2Jok-uYfQ@mail.gmail.com>
Date:   Sat, 17 Jun 2017 11:21:29 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Andrey Ryabinin <aryabinin@...tuozzo.com>
Cc:     Mark Rutland <mark.rutland@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Will Deacon <will.deacon@....com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        kasan-dev <kasan-dev@...glegroups.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH v3 7/7] asm-generic, x86: add comments for atomic instrumentation

On Fri, Jun 16, 2017 at 6:29 PM, Andrey Ryabinin
<aryabinin@...tuozzo.com> 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@...gle.com>
>> Cc: Mark Rutland <mark.rutland@....com>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Cc: Will Deacon <will.deacon@....com>
>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>> Cc: Andrey Ryabinin <aryabinin@...tuozzo.com>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: kasan-dev@...glegroups.com
>> Cc: linux-mm@...ck.org
>> Cc: linux-kernel@...r.kernel.org
>> Cc: x86@...nel.org
>> ---
>>  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);
>>  }
>>