Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations

[Ingo Molnar]

* 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);
>  }

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:

>  
>  /**
> - * 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.

Thanks,

	Ingo