| 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
| ||
|
Message-ID: <CACT4Y+af=UPjL9EUCv9Z5SjHMRdOdUC1OOpq7LLKEHHKm8zysA@mail.gmail.com>
Date: Fri, 24 Mar 2017 08:14:46 +0100
From: Dmitry Vyukov <dvyukov@...gle.com>
To: Ingo Molnar <mingo@...nel.org>
Cc: Mark Rutland <mark.rutland@....com>,
Peter Zijlstra <peterz@...radead.org>,
Andrey Ryabinin <aryabinin@...tuozzo.com>,
Ingo Molnar <mingo@...hat.com>,
Will Deacon <will.deacon@....com>,
Andrew Morton <akpm@...ux-foundation.org>,
kasan-dev <kasan-dev@...glegroups.com>,
"linux-mm@...ck.org" <linux-mm@...ck.org>,
"x86@...nel.org" <x86@...nel.org>,
LKML <linux-kernel@...r.kernel.org>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Thomas Gleixner <tglx@...utronix.de>,
"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations
On Fri, Mar 24, 2017 at 7:52 AM, Ingo Molnar <mingo@...nel.org> wrote:
>
> * Dmitry Vyukov <dvyukov@...gle.com> wrote:
>
>> KASAN uses compiler instrumentation to intercept all memory accesses.
>> But it does not see memory accesses done in assembly code.
>> One notable user of assembly code is atomic operations. Frequently,
>> for example, an atomic reference decrement is the last access to an
>> object and a good candidate for a racy use-after-free.
>>
>> Atomic operations are defined in arch files, but KASAN instrumentation
>> is required for several archs that support KASAN. Later we will need
>> similar hooks for KMSAN (uninit use detector) and KTSAN (data race
>> detector).
>>
>> This change introduces wrappers around atomic operations that can be
>> used to add KASAN/KMSAN/KTSAN instrumentation across several archs.
>> This patch uses the wrappers only for x86 arch. Arm64 will be switched
>> later.
>>
>> 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 | 100 +++++++-------
>> arch/x86/include/asm/atomic64_32.h | 86 ++++++------
>> arch/x86/include/asm/atomic64_64.h | 90 ++++++-------
>> arch/x86/include/asm/cmpxchg.h | 12 +-
>> arch/x86/include/asm/cmpxchg_32.h | 8 +-
>> arch/x86/include/asm/cmpxchg_64.h | 4 +-
>> include/asm-generic/atomic-instrumented.h | 210 ++++++++++++++++++++++++++++++
>> 7 files changed, 367 insertions(+), 143 deletions(-)
>
> Ugh, that's disgusting really...
>
>>
>> diff --git a/arch/x86/include/asm/atomic.h b/arch/x86/include/asm/atomic.h
>> index 14635c5ea025..95dd167eb3af 100644
>> --- a/arch/x86/include/asm/atomic.h
>> +++ b/arch/x86/include/asm/atomic.h
>> @@ -16,36 +16,46 @@
>> #define ATOMIC_INIT(i) { (i) }
>>
>> /**
>> - * atomic_read - read atomic variable
>> + * arch_atomic_read - read atomic variable
>> * @v: pointer of type atomic_t
>> *
>> * Atomically reads the value of @v.
>> */
>> -static __always_inline int atomic_read(const atomic_t *v)
>> +static __always_inline int arch_atomic_read(const atomic_t *v)
>> {
>> - return READ_ONCE((v)->counter);
>> + /*
>> + * We use READ_ONCE_NOCHECK() because atomic_read() contains KASAN
>> + * instrumentation. Double instrumentation is unnecessary.
>> + */
>> + return READ_ONCE_NOCHECK((v)->counter);
>> }
Hello Ingo,
> Firstly, the patch is way too large, please split off new the documentation parts
> of the patch to reduce the size and to make it easier to read!
>
> Secondly, the next patch should do the rename to arch_atomic_*() pattern - and
> nothing else:
Next after what? Please provide full list of patches as you see them.
How do we avoid build breakage if we do only the rename in a separate patch?
>> /**
>> - * atomic_set - set atomic variable
>> + * arch_atomic_set - set atomic variable
>> * @v: pointer of type atomic_t
>> * @i: required value
>> *
>> * Atomically sets the value of @v to @i.
>> */
>> -static __always_inline void atomic_set(atomic_t *v, int i)
>> +static __always_inline void arch_atomic_set(atomic_t *v, int i)
>
>
> Third, the prototype CPP complications:
>
>> +#define __INSTR_VOID1(op, sz) \
>> +static __always_inline void atomic##sz##_##op(atomic##sz##_t *v) \
>> +{ \
>> + arch_atomic##sz##_##op(v); \
>> +}
>> +
>> +#define INSTR_VOID1(op) \
>> +__INSTR_VOID1(op,); \
>> +__INSTR_VOID1(op, 64)
>> +
>> +INSTR_VOID1(inc);
>> +INSTR_VOID1(dec);
>> +
>> +#undef __INSTR_VOID1
>> +#undef INSTR_VOID1
>> +
>> +#define __INSTR_VOID2(op, sz, type) \
>> +static __always_inline void atomic##sz##_##op(type i, atomic##sz##_t *v)\
>> +{ \
>> + arch_atomic##sz##_##op(i, v); \
>> +}
>> +
>> +#define INSTR_VOID2(op) \
>> +__INSTR_VOID2(op, , int); \
>> +__INSTR_VOID2(op, 64, long long)
>> +
>> +INSTR_VOID2(add);
>> +INSTR_VOID2(sub);
>> +INSTR_VOID2(and);
>> +INSTR_VOID2(or);
>> +INSTR_VOID2(xor);
>> +
>> +#undef __INSTR_VOID2
>> +#undef INSTR_VOID2
>> +
>> +#define __INSTR_RET1(op, sz, type, rtype) \
>> +static __always_inline rtype atomic##sz##_##op(atomic##sz##_t *v) \
>> +{ \
>> + return arch_atomic##sz##_##op(v); \
>> +}
>> +
>> +#define INSTR_RET1(op) \
>> +__INSTR_RET1(op, , int, int); \
>> +__INSTR_RET1(op, 64, long long, long long)
>> +
>> +INSTR_RET1(inc_return);
>> +INSTR_RET1(dec_return);
>> +__INSTR_RET1(inc_not_zero, 64, long long, long long);
>> +__INSTR_RET1(dec_if_positive, 64, long long, long long);
>> +
>> +#define INSTR_RET_BOOL1(op) \
>> +__INSTR_RET1(op, , int, bool); \
>> +__INSTR_RET1(op, 64, long long, bool)
>> +
>> +INSTR_RET_BOOL1(dec_and_test);
>> +INSTR_RET_BOOL1(inc_and_test);
>> +
>> +#undef __INSTR_RET1
>> +#undef INSTR_RET1
>> +#undef INSTR_RET_BOOL1
>> +
>> +#define __INSTR_RET2(op, sz, type, rtype) \
>> +static __always_inline rtype atomic##sz##_##op(type i, atomic##sz##_t *v) \
>> +{ \
>> + return arch_atomic##sz##_##op(i, v); \
>> +}
>> +
>> +#define INSTR_RET2(op) \
>> +__INSTR_RET2(op, , int, int); \
>> +__INSTR_RET2(op, 64, long long, long long)
>> +
>> +INSTR_RET2(add_return);
>> +INSTR_RET2(sub_return);
>> +INSTR_RET2(fetch_add);
>> +INSTR_RET2(fetch_sub);
>> +INSTR_RET2(fetch_and);
>> +INSTR_RET2(fetch_or);
>> +INSTR_RET2(fetch_xor);
>> +
>> +#define INSTR_RET_BOOL2(op) \
>> +__INSTR_RET2(op, , int, bool); \
>> +__INSTR_RET2(op, 64, long long, bool)
>> +
>> +INSTR_RET_BOOL2(sub_and_test);
>> +INSTR_RET_BOOL2(add_negative);
>> +
>> +#undef __INSTR_RET2
>> +#undef INSTR_RET2
>> +#undef INSTR_RET_BOOL2
>
> Are just utterly disgusting that turn perfectly readable code into an unreadable,
> unmaintainable mess.
>
> You need to find some better, cleaner solution please, or convince me that no such
> solution is possible. NAK for the time being.
Well, I can just write all functions as is. Does it better confirm to
kernel style? I've just looked at the x86 atomic.h and it uses macros
for similar purpose (ATOMIC_OP/ATOMIC_FETCH_OP), so I thought that
must be idiomatic kernel style...
Powered by blists - more mailing lists