On Fri, Mar 24, 2017 at 7:52 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote: > > * Dmitry Vyukov <dvyukov@xxxxxxxxxx> 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@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 | 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... -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx"> email@xxxxxxxxx </a>