Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1672657 > unrolled thread

[PATCH v5 3/4] asm-generic: add KASAN instrumentation to atomic operations

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 3/4] asm-generic: add KASAN instrumentation to atomic operations Dmitry Vyukov <dvyukov@google.com> - 2017-06-22 16:20 +0200
    [tip:locking/core] locking/atomics, asm-generic: Add KASAN  instrumentation to atomic operations tip-bot for Dmitry Vyukov <tipbot@zytor.com> - 2017-06-23 16:10 +0200
    [PATCH] locking/atomics: don't alias ____ptr Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-06-28 12:10 +0200
      Re: [PATCH] locking/atomics: don't alias ____ptr Dmitry Vyukov <dvyukov@google.com> - 2017-06-28 12:20 +0200
        Re: [PATCH] locking/atomics: don't alias ____ptr Thomas Gleixner <tglx@linutronix.de> - 2017-06-28 13:20 +0200
          Re: [PATCH] locking/atomics: don't alias ____ptr Dmitry Vyukov <dvyukov@google.com> - 2017-06-28 13:20 +0200
            Re: [PATCH] locking/atomics: don't alias ____ptr Thomas Gleixner <tglx@linutronix.de> - 2017-06-28 13:30 +0200
              Re: [PATCH] locking/atomics: don't alias ____ptr Mark Rutland <mark.rutland@arm.com> - 2017-06-28 14:50 +0200
            Re: [PATCH] locking/atomics: don't alias ____ptr Dmitry Vyukov <dvyukov@google.com> - 2017-06-28 14:30 +0200
              Re: [PATCH] locking/atomics: don't alias ____ptr Thomas Gleixner <tglx@linutronix.de> - 2017-06-28 15:40 +0200
            Re: [PATCH] locking/atomics: don't alias ____ptr Thomas Gleixner <tglx@linutronix.de> - 2017-06-28 14:30 +0200
        Re: [PATCH] locking/atomics: don't alias ____ptr Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-06-28 14:20 +0200
          Re: [PATCH] locking/atomics: don't alias ____ptr Thomas Gleixner <tglx@linutronix.de> - 2017-06-28 15:30 +0200
            Re: [PATCH] locking/atomics: don't alias ____ptr Thomas Gleixner <tglx@linutronix.de> - 2017-06-28 16:00 +0200
              Re: [PATCH] locking/atomics: don't alias ____ptr Mark Rutland <mark.rutland@arm.com> - 2017-06-28 16:20 +0200
                Re: [PATCH] locking/atomics: don't alias ____ptr Thomas Gleixner <tglx@linutronix.de> - 2017-06-28 17:30 +0200
                  Re: [PATCH] locking/atomics: don't alias ____ptr Mark Rutland <mark.rutland@arm.com> - 2017-06-28 18:00 +0200
                    Re: [PATCH] locking/atomics: don't alias ____ptr Ingo Molnar <mingo@kernel.org> - 2017-06-28 20:00 +0200
                    Re: [PATCH] locking/atomics: don't alias ____ptr Thomas Gleixner <tglx@linutronix.de> - 2017-06-28 20:30 +0200
                      Re: [PATCH] locking/atomics: don't alias ____ptr Thomas Gleixner <tglx@linutronix.de> - 2017-06-29 08:50 +0200

#1672657 — [PATCH v5 3/4] asm-generic: add KASAN instrumentation to atomic operations

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.

Add manual KASAN checks to atomic operations.

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>,
Cc: Andrew Morton <akpm@linux-foundation.org>,
Cc: Ingo Molnar <mingo@redhat.com>,
Cc: kasan-dev@googlegroups.com
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Cc: x86@kernel.org
---
 include/asm-generic/atomic-instrumented.h | 76 +++++++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 4 deletions(-)

diff --git a/include/asm-generic/atomic-instrumented.h b/include/asm-generic/atomic-instrumented.h
index 50401d925290..a0f5b7525bb2 100644
--- a/include/asm-generic/atomic-instrumented.h
+++ b/include/asm-generic/atomic-instrumented.h
@@ -1,43 +1,53 @@
 #ifndef _LINUX_ATOMIC_INSTRUMENTED_H
 #define _LINUX_ATOMIC_INSTRUMENTED_H
 
+#include <linux/kasan-checks.h>
+
 static __always_inline int atomic_read(const atomic_t *v)
 {
+	kasan_check_read(v, sizeof(*v));
 	return arch_atomic_read(v);
 }
 
 static __always_inline s64 atomic64_read(const atomic64_t *v)
 {
+	kasan_check_read(v, sizeof(*v));
 	return arch_atomic64_read(v);
 }
 
 static __always_inline void atomic_set(atomic_t *v, int i)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_set(v, i);
 }
 
 static __always_inline void atomic64_set(atomic64_t *v, s64 i)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_set(v, i);
 }
 
 static __always_inline int atomic_xchg(atomic_t *v, int i)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_xchg(v, i);
 }
 
 static __always_inline s64 atomic64_xchg(atomic64_t *v, s64 i)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_xchg(v, i);
 }
 
 static __always_inline int atomic_cmpxchg(atomic_t *v, int old, int new)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_cmpxchg(v, old, new);
 }
 
 static __always_inline s64 atomic64_cmpxchg(atomic64_t *v, s64 old, s64 new)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_cmpxchg(v, old, new);
 }
 
@@ -45,6 +55,8 @@ static __always_inline s64 atomic64_cmpxchg(atomic64_t *v, s64 old, s64 new)
 #define atomic_try_cmpxchg atomic_try_cmpxchg
 static __always_inline bool atomic_try_cmpxchg(atomic_t *v, int *old, int new)
 {
+	kasan_check_write(v, sizeof(*v));
+	kasan_check_read(old, sizeof(*old));
 	return arch_atomic_try_cmpxchg(v, old, new);
 }
 #endif
@@ -53,254 +65,310 @@ static __always_inline bool atomic_try_cmpxchg(atomic_t *v, int *old, int new)
 #define atomic64_try_cmpxchg atomic64_try_cmpxchg
 static __always_inline bool atomic64_try_cmpxchg(atomic64_t *v, s64 *old, s64 new)
 {
+	kasan_check_write(v, sizeof(*v));
+	kasan_check_read(old, sizeof(*old));
 	return arch_atomic64_try_cmpxchg(v, old, new);
 }
 #endif
 
 static __always_inline int __atomic_add_unless(atomic_t *v, int a, int u)
 {
+	kasan_check_write(v, sizeof(*v));
 	return __arch_atomic_add_unless(v, a, u);
 }
 
 
 static __always_inline bool atomic64_add_unless(atomic64_t *v, s64 a, s64 u)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_add_unless(v, a, u);
 }
 
 static __always_inline void atomic_inc(atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_inc(v);
 }
 
 static __always_inline void atomic64_inc(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_inc(v);
 }
 
 static __always_inline void atomic_dec(atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_dec(v);
 }
 
 static __always_inline void atomic64_dec(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_dec(v);
 }
 
 static __always_inline void atomic_add(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_add(i, v);
 }
 
 static __always_inline void atomic64_add(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_add(i, v);
 }
 
 static __always_inline void atomic_sub(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_sub(i, v);
 }
 
 static __always_inline void atomic64_sub(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_sub(i, v);
 }
 
 static __always_inline void atomic_and(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_and(i, v);
 }
 
 static __always_inline void atomic64_and(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_and(i, v);
 }
 
 static __always_inline void atomic_or(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_or(i, v);
 }
 
 static __always_inline void atomic64_or(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_or(i, v);
 }
 
 static __always_inline void atomic_xor(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_xor(i, v);
 }
 
 static __always_inline void atomic64_xor(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_xor(i, v);
 }
 
 static __always_inline int atomic_inc_return(atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_inc_return(v);
 }
 
 static __always_inline s64 atomic64_inc_return(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_inc_return(v);
 }
 
 static __always_inline int atomic_dec_return(atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_dec_return(v);
 }
 
 static __always_inline s64 atomic64_dec_return(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_dec_return(v);
 }
 
 static __always_inline s64 atomic64_inc_not_zero(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_inc_not_zero(v);
 }
 
 static __always_inline s64 atomic64_dec_if_positive(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_dec_if_positive(v);
 }
 
 static __always_inline bool atomic_dec_and_test(atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_dec_and_test(v);
 }
 
 static __always_inline bool atomic64_dec_and_test(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_dec_and_test(v);
 }
 
 static __always_inline bool atomic_inc_and_test(atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_inc_and_test(v);
 }
 
 static __always_inline bool atomic64_inc_and_test(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_inc_and_test(v);
 }
 
 static __always_inline int atomic_add_return(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_add_return(i, v);
 }
 
 static __always_inline s64 atomic64_add_return(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_add_return(i, v);
 }
 
 static __always_inline int atomic_sub_return(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_sub_return(i, v);
 }
 
 static __always_inline s64 atomic64_sub_return(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_sub_return(i, v);
 }
 
 static __always_inline int atomic_fetch_add(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_fetch_add(i, v);
 }
 
 static __always_inline s64 atomic64_fetch_add(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_fetch_add(i, v);
 }
 
 static __always_inline int atomic_fetch_sub(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_fetch_sub(i, v);
 }
 
 static __always_inline s64 atomic64_fetch_sub(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_fetch_sub(i, v);
 }
 
 static __always_inline int atomic_fetch_and(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_fetch_and(i, v);
 }
 
 static __always_inline s64 atomic64_fetch_and(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_fetch_and(i, v);
 }
 
 static __always_inline int atomic_fetch_or(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_fetch_or(i, v);
 }
 
 static __always_inline s64 atomic64_fetch_or(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_fetch_or(i, v);
 }
 
 static __always_inline int atomic_fetch_xor(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_fetch_xor(i, v);
 }
 
 static __always_inline s64 atomic64_fetch_xor(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_fetch_xor(i, v);
 }
 
 static __always_inline bool atomic_sub_and_test(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_sub_and_test(i, v);
 }
 
 static __always_inline bool atomic64_sub_and_test(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_sub_and_test(i, v);
 }
 
 static __always_inline bool atomic_add_negative(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_add_negative(i, v);
 }
 
 static __always_inline bool atomic64_add_negative(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_add_negative(i, v);
 }
 
 #define cmpxchg(ptr, old, new)				\
 ({							\
+	__typeof__(ptr) ___ptr = (ptr);			\
+	kasan_check_write(___ptr, sizeof(*___ptr));	\
 	arch_cmpxchg((ptr), (old), (new));		\
 })
 
 #define sync_cmpxchg(ptr, old, new)			\
 ({							\
-	arch_sync_cmpxchg((ptr), (old), (new));		\
+	__typeof__(ptr) ___ptr = (ptr);			\
+	kasan_check_write(___ptr, sizeof(*___ptr));	\
+	arch_sync_cmpxchg(___ptr, (old), (new));	\
 })
 
 #define cmpxchg_local(ptr, old, new)			\
 ({							\
-	arch_cmpxchg_local((ptr), (old), (new));	\
+	__typeof__(ptr) ____ptr = (ptr);		\
+	kasan_check_write(____ptr, sizeof(*____ptr));	\
+	arch_cmpxchg_local(____ptr, (old), (new));	\
 })
 
 #define cmpxchg64(ptr, old, new)			\
 ({							\
-	arch_cmpxchg64((ptr), (old), (new));		\
+	__typeof__(ptr) ____ptr = (ptr);		\
+	kasan_check_write(____ptr, sizeof(*____ptr));	\
+	arch_cmpxchg64(____ptr, (old), (new));		\
 })
 
 #define cmpxchg64_local(ptr, old, new)			\
 ({							\
-	arch_cmpxchg64_local((ptr), (old), (new));	\
+	__typeof__(ptr) ____ptr = (ptr);		\
+	kasan_check_write(____ptr, sizeof(*____ptr));	\
+	arch_cmpxchg64_local(____ptr, (old), (new));	\
 })
 
 #define cmpxchg_double(p1, p2, o1, o2, n1, n2)				\
-- 
2.13.1.611.g7e3b11ae1-goog

[toc] | [next] | [standalone]

#1673594 — [tip:locking/core] locking/atomics, asm-generic: Add KASAN instrumentation to atomic operations

Commit-ID:  235a93822a21ef6459f4f8eeb58f7bf48e02b450
Gitweb:     http://git.kernel.org/tip/235a93822a21ef6459f4f8eeb58f7bf48e02b450
Author:     Dmitry Vyukov <dvyukov@google.com>
AuthorDate: Thu, 22 Jun 2017 16:14:18 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 23 Jun 2017 10:50:20 +0200

locking/atomics, asm-generic: Add KASAN instrumentation to atomic operations

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.

Add manual KASAN checks to atomic operations.

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>,
Cc: kasan-dev@googlegroups.com
Cc: linux-mm@kvack.org
Link: http://lkml.kernel.org/r/85d51d3551b676ba1fc40e8fbddd2eadd056d8dd.1498140838.git.dvyukov@google.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/asm-generic/atomic-instrumented.h | 76 +++++++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 4 deletions(-)

diff --git a/include/asm-generic/atomic-instrumented.h b/include/asm-generic/atomic-instrumented.h
index 50401d9..a0f5b75 100644
--- a/include/asm-generic/atomic-instrumented.h
+++ b/include/asm-generic/atomic-instrumented.h
@@ -1,43 +1,53 @@
 #ifndef _LINUX_ATOMIC_INSTRUMENTED_H
 #define _LINUX_ATOMIC_INSTRUMENTED_H
 
+#include <linux/kasan-checks.h>
+
 static __always_inline int atomic_read(const atomic_t *v)
 {
+	kasan_check_read(v, sizeof(*v));
 	return arch_atomic_read(v);
 }
 
 static __always_inline s64 atomic64_read(const atomic64_t *v)
 {
+	kasan_check_read(v, sizeof(*v));
 	return arch_atomic64_read(v);
 }
 
 static __always_inline void atomic_set(atomic_t *v, int i)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_set(v, i);
 }
 
 static __always_inline void atomic64_set(atomic64_t *v, s64 i)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_set(v, i);
 }
 
 static __always_inline int atomic_xchg(atomic_t *v, int i)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_xchg(v, i);
 }
 
 static __always_inline s64 atomic64_xchg(atomic64_t *v, s64 i)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_xchg(v, i);
 }
 
 static __always_inline int atomic_cmpxchg(atomic_t *v, int old, int new)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_cmpxchg(v, old, new);
 }
 
 static __always_inline s64 atomic64_cmpxchg(atomic64_t *v, s64 old, s64 new)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_cmpxchg(v, old, new);
 }
 
@@ -45,6 +55,8 @@ static __always_inline s64 atomic64_cmpxchg(atomic64_t *v, s64 old, s64 new)
 #define atomic_try_cmpxchg atomic_try_cmpxchg
 static __always_inline bool atomic_try_cmpxchg(atomic_t *v, int *old, int new)
 {
+	kasan_check_write(v, sizeof(*v));
+	kasan_check_read(old, sizeof(*old));
 	return arch_atomic_try_cmpxchg(v, old, new);
 }
 #endif
@@ -53,254 +65,310 @@ static __always_inline bool atomic_try_cmpxchg(atomic_t *v, int *old, int new)
 #define atomic64_try_cmpxchg atomic64_try_cmpxchg
 static __always_inline bool atomic64_try_cmpxchg(atomic64_t *v, s64 *old, s64 new)
 {
+	kasan_check_write(v, sizeof(*v));
+	kasan_check_read(old, sizeof(*old));
 	return arch_atomic64_try_cmpxchg(v, old, new);
 }
 #endif
 
 static __always_inline int __atomic_add_unless(atomic_t *v, int a, int u)
 {
+	kasan_check_write(v, sizeof(*v));
 	return __arch_atomic_add_unless(v, a, u);
 }
 
 
 static __always_inline bool atomic64_add_unless(atomic64_t *v, s64 a, s64 u)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_add_unless(v, a, u);
 }
 
 static __always_inline void atomic_inc(atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_inc(v);
 }
 
 static __always_inline void atomic64_inc(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_inc(v);
 }
 
 static __always_inline void atomic_dec(atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_dec(v);
 }
 
 static __always_inline void atomic64_dec(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_dec(v);
 }
 
 static __always_inline void atomic_add(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_add(i, v);
 }
 
 static __always_inline void atomic64_add(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_add(i, v);
 }
 
 static __always_inline void atomic_sub(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_sub(i, v);
 }
 
 static __always_inline void atomic64_sub(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_sub(i, v);
 }
 
 static __always_inline void atomic_and(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_and(i, v);
 }
 
 static __always_inline void atomic64_and(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_and(i, v);
 }
 
 static __always_inline void atomic_or(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_or(i, v);
 }
 
 static __always_inline void atomic64_or(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_or(i, v);
 }
 
 static __always_inline void atomic_xor(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic_xor(i, v);
 }
 
 static __always_inline void atomic64_xor(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	arch_atomic64_xor(i, v);
 }
 
 static __always_inline int atomic_inc_return(atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_inc_return(v);
 }
 
 static __always_inline s64 atomic64_inc_return(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_inc_return(v);
 }
 
 static __always_inline int atomic_dec_return(atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_dec_return(v);
 }
 
 static __always_inline s64 atomic64_dec_return(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_dec_return(v);
 }
 
 static __always_inline s64 atomic64_inc_not_zero(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_inc_not_zero(v);
 }
 
 static __always_inline s64 atomic64_dec_if_positive(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_dec_if_positive(v);
 }
 
 static __always_inline bool atomic_dec_and_test(atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_dec_and_test(v);
 }
 
 static __always_inline bool atomic64_dec_and_test(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_dec_and_test(v);
 }
 
 static __always_inline bool atomic_inc_and_test(atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_inc_and_test(v);
 }
 
 static __always_inline bool atomic64_inc_and_test(atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_inc_and_test(v);
 }
 
 static __always_inline int atomic_add_return(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_add_return(i, v);
 }
 
 static __always_inline s64 atomic64_add_return(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_add_return(i, v);
 }
 
 static __always_inline int atomic_sub_return(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_sub_return(i, v);
 }
 
 static __always_inline s64 atomic64_sub_return(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_sub_return(i, v);
 }
 
 static __always_inline int atomic_fetch_add(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_fetch_add(i, v);
 }
 
 static __always_inline s64 atomic64_fetch_add(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_fetch_add(i, v);
 }
 
 static __always_inline int atomic_fetch_sub(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_fetch_sub(i, v);
 }
 
 static __always_inline s64 atomic64_fetch_sub(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_fetch_sub(i, v);
 }
 
 static __always_inline int atomic_fetch_and(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_fetch_and(i, v);
 }
 
 static __always_inline s64 atomic64_fetch_and(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_fetch_and(i, v);
 }
 
 static __always_inline int atomic_fetch_or(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_fetch_or(i, v);
 }
 
 static __always_inline s64 atomic64_fetch_or(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_fetch_or(i, v);
 }
 
 static __always_inline int atomic_fetch_xor(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_fetch_xor(i, v);
 }
 
 static __always_inline s64 atomic64_fetch_xor(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_fetch_xor(i, v);
 }
 
 static __always_inline bool atomic_sub_and_test(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_sub_and_test(i, v);
 }
 
 static __always_inline bool atomic64_sub_and_test(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_sub_and_test(i, v);
 }
 
 static __always_inline bool atomic_add_negative(int i, atomic_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_add_negative(i, v);
 }
 
 static __always_inline bool atomic64_add_negative(s64 i, atomic64_t *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_add_negative(i, v);
 }
 
 #define cmpxchg(ptr, old, new)				\
 ({							\
+	__typeof__(ptr) ___ptr = (ptr);			\
+	kasan_check_write(___ptr, sizeof(*___ptr));	\
 	arch_cmpxchg((ptr), (old), (new));		\
 })
 
 #define sync_cmpxchg(ptr, old, new)			\
 ({							\
-	arch_sync_cmpxchg((ptr), (old), (new));		\
+	__typeof__(ptr) ___ptr = (ptr);			\
+	kasan_check_write(___ptr, sizeof(*___ptr));	\
+	arch_sync_cmpxchg(___ptr, (old), (new));	\
 })
 
 #define cmpxchg_local(ptr, old, new)			\
 ({							\
-	arch_cmpxchg_local((ptr), (old), (new));	\
+	__typeof__(ptr) ____ptr = (ptr);		\
+	kasan_check_write(____ptr, sizeof(*____ptr));	\
+	arch_cmpxchg_local(____ptr, (old), (new));	\
 })
 
 #define cmpxchg64(ptr, old, new)			\
 ({							\
-	arch_cmpxchg64((ptr), (old), (new));		\
+	__typeof__(ptr) ____ptr = (ptr);		\
+	kasan_check_write(____ptr, sizeof(*____ptr));	\
+	arch_cmpxchg64(____ptr, (old), (new));		\
 })
 
 #define cmpxchg64_local(ptr, old, new)			\
 ({							\
-	arch_cmpxchg64_local((ptr), (old), (new));	\
+	__typeof__(ptr) ____ptr = (ptr);		\
+	kasan_check_write(____ptr, sizeof(*____ptr));	\
+	arch_cmpxchg64_local(____ptr, (old), (new));	\
 })
 
 #define cmpxchg_double(p1, p2, o1, o2, n1, n2)				\

[toc] | [prev] | [next] | [standalone]

#1676508 — [PATCH] locking/atomics: don't alias ____ptr

Trying to boot tip/master resulted in:
|DMAR: dmar0: Using Queued invalidation
|DMAR: dmar1: Using Queued invalidation
|DMAR: Setting RMRR:
|DMAR: Setting identity map for device 0000:00:1a.0 [0xbdcf9000 - 0xbdd1dfff]
|BUG: unable to handle kernel NULL pointer dereference at           (null)
|IP: __domain_mapping+0x10f/0x3d0
|PGD 0
|P4D 0
|
|Oops: 0002 [#1] PREEMPT SMP
|Modules linked in:
|CPU: 19 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc6-00117-g235a93822a21 #113
|task: ffff8805271c2c80 task.stack: ffffc90000058000
|RIP: 0010:__domain_mapping+0x10f/0x3d0
|RSP: 0000:ffffc9000005bca0 EFLAGS: 00010246
|RAX: 0000000000000000 RBX: 00000000bdcf9003 RCX: 0000000000000000
|RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000001
|RBP: ffffc9000005bd00 R08: ffff880a243e9780 R09: ffff8805259e67c8
|R10: 00000000000bdcf9 R11: 0000000000000000 R12: 0000000000000025
|R13: 0000000000000025 R14: 0000000000000000 R15: 00000000000bdcf9
|FS:  0000000000000000(0000) GS:ffff88052acc0000(0000) knlGS:0000000000000000
|CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
|CR2: 0000000000000000 CR3: 0000000001c0f000 CR4: 00000000000406e0
|Call Trace:
| iommu_domain_identity_map+0x5a/0x80
| domain_prepare_identity_map+0x9f/0x160
| iommu_prepare_identity_map+0x7e/0x9b

bisect points to commit 235a93822a21 ("locking/atomics, asm-generic: Add KASAN
instrumentation to atomic operations"), RIP is at
	 tmp = cmpxchg64_local(&pte->val, 0ULL, pteval);
in drivers/iommu/intel-iommu.c. The assembly for this inline assembly
is:
    xor    %edx,%edx
    xor    %eax,%eax
    cmpxchg %rbx,(%rdx)

and as you see edx is set to zero and used later as a pointer via the
full register. This happens with gcc-6, 5 and 8 (snapshot from last
week).
After a longer while of searching and swearing I figured out that this
bug occures once cmpxchg64_local() and cmpxchg_local() uses the same
____ptr macro and they are shadow somehow. What I don't know why edx is
set to zero.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/asm-generic/atomic-instrumented.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/asm-generic/atomic-instrumented.h b/include/asm-generic/atomic-instrumented.h
index a0f5b7525bb2..ac6155362b39 100644
--- a/include/asm-generic/atomic-instrumented.h
+++ b/include/asm-generic/atomic-instrumented.h
@@ -359,16 +359,16 @@ static __always_inline bool atomic64_add_negative(s64 i, atomic64_t *v)
 
 #define cmpxchg64(ptr, old, new)			\
 ({							\
-	__typeof__(ptr) ____ptr = (ptr);		\
-	kasan_check_write(____ptr, sizeof(*____ptr));	\
-	arch_cmpxchg64(____ptr, (old), (new));		\
+	__typeof__(ptr) ____ptr64 = (ptr);		\
+	kasan_check_write(____ptr64, sizeof(*____ptr64));\
+	arch_cmpxchg64(____ptr64, (old), (new));	\
 })
 
 #define cmpxchg64_local(ptr, old, new)			\
 ({							\
-	__typeof__(ptr) ____ptr = (ptr);		\
-	kasan_check_write(____ptr, sizeof(*____ptr));	\
-	arch_cmpxchg64_local(____ptr, (old), (new));	\
+	__typeof__(ptr) ____ptr64 = (ptr);		\
+	kasan_check_write(____ptr64, sizeof(*____ptr64));\
+	arch_cmpxchg64_local(____ptr64, (old), (new));	\
 })
 
 #define cmpxchg_double(p1, p2, o1, o2, n1, n2)				\
-- 
2.13.2

[toc] | [prev] | [next] | [standalone]

#1676511 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, Jun 28, 2017 at 12:02 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> Trying to boot tip/master resulted in:
> |DMAR: dmar0: Using Queued invalidation
> |DMAR: dmar1: Using Queued invalidation
> |DMAR: Setting RMRR:
> |DMAR: Setting identity map for device 0000:00:1a.0 [0xbdcf9000 - 0xbdd1dfff]
> |BUG: unable to handle kernel NULL pointer dereference at           (null)
> |IP: __domain_mapping+0x10f/0x3d0
> |PGD 0
> |P4D 0
> |
> |Oops: 0002 [#1] PREEMPT SMP
> |Modules linked in:
> |CPU: 19 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc6-00117-g235a93822a21 #113
> |task: ffff8805271c2c80 task.stack: ffffc90000058000
> |RIP: 0010:__domain_mapping+0x10f/0x3d0
> |RSP: 0000:ffffc9000005bca0 EFLAGS: 00010246
> |RAX: 0000000000000000 RBX: 00000000bdcf9003 RCX: 0000000000000000
> |RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000001
> |RBP: ffffc9000005bd00 R08: ffff880a243e9780 R09: ffff8805259e67c8
> |R10: 00000000000bdcf9 R11: 0000000000000000 R12: 0000000000000025
> |R13: 0000000000000025 R14: 0000000000000000 R15: 00000000000bdcf9
> |FS:  0000000000000000(0000) GS:ffff88052acc0000(0000) knlGS:0000000000000000
> |CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> |CR2: 0000000000000000 CR3: 0000000001c0f000 CR4: 00000000000406e0
> |Call Trace:
> | iommu_domain_identity_map+0x5a/0x80
> | domain_prepare_identity_map+0x9f/0x160
> | iommu_prepare_identity_map+0x7e/0x9b
>
> bisect points to commit 235a93822a21 ("locking/atomics, asm-generic: Add KASAN
> instrumentation to atomic operations"), RIP is at
>          tmp = cmpxchg64_local(&pte->val, 0ULL, pteval);
> in drivers/iommu/intel-iommu.c. The assembly for this inline assembly
> is:
>     xor    %edx,%edx
>     xor    %eax,%eax
>     cmpxchg %rbx,(%rdx)
>
> and as you see edx is set to zero and used later as a pointer via the
> full register. This happens with gcc-6, 5 and 8 (snapshot from last
> week).
> After a longer while of searching and swearing I figured out that this
> bug occures once cmpxchg64_local() and cmpxchg_local() uses the same
> ____ptr macro and they are shadow somehow. What I don't know why edx is
> set to zero.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  include/asm-generic/atomic-instrumented.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/asm-generic/atomic-instrumented.h b/include/asm-generic/atomic-instrumented.h
> index a0f5b7525bb2..ac6155362b39 100644
> --- a/include/asm-generic/atomic-instrumented.h
> +++ b/include/asm-generic/atomic-instrumented.h
> @@ -359,16 +359,16 @@ static __always_inline bool atomic64_add_negative(s64 i, atomic64_t *v)
>
>  #define cmpxchg64(ptr, old, new)                       \
>  ({                                                     \
> -       __typeof__(ptr) ____ptr = (ptr);                \
> -       kasan_check_write(____ptr, sizeof(*____ptr));   \
> -       arch_cmpxchg64(____ptr, (old), (new));          \
> +       __typeof__(ptr) ____ptr64 = (ptr);              \
> +       kasan_check_write(____ptr64, sizeof(*____ptr64));\
> +       arch_cmpxchg64(____ptr64, (old), (new));        \
>  })
>
>  #define cmpxchg64_local(ptr, old, new)                 \
>  ({                                                     \
> -       __typeof__(ptr) ____ptr = (ptr);                \
> -       kasan_check_write(____ptr, sizeof(*____ptr));   \
> -       arch_cmpxchg64_local(____ptr, (old), (new));    \
> +       __typeof__(ptr) ____ptr64 = (ptr);              \
> +       kasan_check_write(____ptr64, sizeof(*____ptr64));\
> +       arch_cmpxchg64_local(____ptr64, (old), (new));  \
>  })
>
>  #define cmpxchg_double(p1, p2, o1, o2, n1, n2)                         \


Doh! Thanks for fixing this. I think I've a similar crash in a
different place when I developed the patch.
The problem is that when we do:

       __typeof__(ptr) ____ptr = (ptr);                \
       arch_cmpxchg64_local(____ptr, (old), (new));    \

We don't necessary pass value of our just declared ____ptr to
arch_cmpxchg64_local(). We just pass a symbolic identifier. So if
arch_cmpxchg64_local() declares own ____ptr and then tries to use what
we passed ("____ptr") it will actually refer to own variable declared
rather than to what we wanted to pass in.

In my case I ended up with something like:

__typeof__(foo) __ptr = __ptr;

which compiler decided to turn into 0.

Thank you, macros.

We can add more underscores, but the problem can happen again. Should
we prefix current function/macro name to all local vars?..

[toc] | [prev] | [next] | [standalone]

#1676557 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, 28 Jun 2017, Dmitry Vyukov wrote:
> On Wed, Jun 28, 2017 at 12:02 PM, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> > @@ -359,16 +359,16 @@ static __always_inline bool atomic64_add_negative(s64 i, atomic64_t *v)
> >
> >  #define cmpxchg64(ptr, old, new)                       \
> >  ({                                                     \
> > -       __typeof__(ptr) ____ptr = (ptr);                \
> > -       kasan_check_write(____ptr, sizeof(*____ptr));   \
> > -       arch_cmpxchg64(____ptr, (old), (new));          \
> > +       __typeof__(ptr) ____ptr64 = (ptr);              \
> > +       kasan_check_write(____ptr64, sizeof(*____ptr64));\
> > +       arch_cmpxchg64(____ptr64, (old), (new));        \
> >  })
> >
> >  #define cmpxchg64_local(ptr, old, new)                 \
> >  ({                                                     \
> > -       __typeof__(ptr) ____ptr = (ptr);                \
> > -       kasan_check_write(____ptr, sizeof(*____ptr));   \
> > -       arch_cmpxchg64_local(____ptr, (old), (new));    \
> > +       __typeof__(ptr) ____ptr64 = (ptr);              \
> > +       kasan_check_write(____ptr64, sizeof(*____ptr64));\
> > +       arch_cmpxchg64_local(____ptr64, (old), (new));  \
> >  })
> >
> >  #define cmpxchg_double(p1, p2, o1, o2, n1, n2)                         \
> 
> 
> Doh! Thanks for fixing this. I think I've a similar crash in a
> different place when I developed the patch.
> The problem is that when we do:
> 
>        __typeof__(ptr) ____ptr = (ptr);                \
>        arch_cmpxchg64_local(____ptr, (old), (new));    \
> 
> We don't necessary pass value of our just declared ____ptr to
> arch_cmpxchg64_local(). We just pass a symbolic identifier. So if
> arch_cmpxchg64_local() declares own ____ptr and then tries to use what
> we passed ("____ptr") it will actually refer to own variable declared
> rather than to what we wanted to pass in.
> 
> In my case I ended up with something like:
> 
> __typeof__(foo) __ptr = __ptr;
> 
> which compiler decided to turn into 0.
> 
> Thank you, macros.
> 
> We can add more underscores, but the problem can happen again. Should
> we prefix current function/macro name to all local vars?..

Actually we can void that ___ptr dance completely.

Thanks,

	tglx

8<--------------------

--- a/include/asm-generic/atomic-instrumented.h
+++ b/include/asm-generic/atomic-instrumented.h
@@ -359,37 +359,32 @@ static __always_inline bool atomic64_add
 
 #define cmpxchg(ptr, old, new)				\
 ({							\
-	__typeof__(ptr) ___ptr = (ptr);			\
-	kasan_check_write(___ptr, sizeof(*___ptr));	\
+	kasan_check_write((ptr), sizeof(*(ptr)));	\
 	arch_cmpxchg((ptr), (old), (new));		\
 })
 
 #define sync_cmpxchg(ptr, old, new)			\
 ({							\
-	__typeof__(ptr) ___ptr = (ptr);			\
-	kasan_check_write(___ptr, sizeof(*___ptr));	\
-	arch_sync_cmpxchg(___ptr, (old), (new));	\
+	kasan_check_write((ptr), sizeof(*(ptr)));	\
+	arch_sync_cmpxchg((ptr), (old), (new));		\
 })
 
 #define cmpxchg_local(ptr, old, new)			\
 ({							\
-	__typeof__(ptr) ____ptr = (ptr);		\
-	kasan_check_write(____ptr, sizeof(*____ptr));	\
-	arch_cmpxchg_local(____ptr, (old), (new));	\
+	kasan_check_write((ptr), sizeof(*(ptr)));	\
+	arch_cmpxchg_local((ptr), (old), (new));	\
 })
 
 #define cmpxchg64(ptr, old, new)			\
 ({							\
-	__typeof__(ptr) ____ptr = (ptr);		\
-	kasan_check_write(____ptr, sizeof(*____ptr));	\
-	arch_cmpxchg64(____ptr, (old), (new));		\
+	kasan_check_write((ptr), sizeof(*(ptr)));	\
+	arch_cmpxchg64((ptr), (old), (new));		\
 })
 
 #define cmpxchg64_local(ptr, old, new)			\
 ({							\
-	__typeof__(ptr) ____ptr = (ptr);		\
-	kasan_check_write(____ptr, sizeof(*____ptr));	\
-	arch_cmpxchg64_local(____ptr, (old), (new));	\
+	kasan_check_write((ptr), sizeof(*(ptr)));	\
+	arch_cmpxchg64_local((ptr), (old), (new));	\
 })
 
 /*

[toc] | [prev] | [next] | [standalone]

#1676558 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, Jun 28, 2017 at 1:10 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 28 Jun 2017, Dmitry Vyukov wrote:
>> On Wed, Jun 28, 2017 at 12:02 PM, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>> > @@ -359,16 +359,16 @@ static __always_inline bool atomic64_add_negative(s64 i, atomic64_t *v)
>> >
>> >  #define cmpxchg64(ptr, old, new)                       \
>> >  ({                                                     \
>> > -       __typeof__(ptr) ____ptr = (ptr);                \
>> > -       kasan_check_write(____ptr, sizeof(*____ptr));   \
>> > -       arch_cmpxchg64(____ptr, (old), (new));          \
>> > +       __typeof__(ptr) ____ptr64 = (ptr);              \
>> > +       kasan_check_write(____ptr64, sizeof(*____ptr64));\
>> > +       arch_cmpxchg64(____ptr64, (old), (new));        \
>> >  })
>> >
>> >  #define cmpxchg64_local(ptr, old, new)                 \
>> >  ({                                                     \
>> > -       __typeof__(ptr) ____ptr = (ptr);                \
>> > -       kasan_check_write(____ptr, sizeof(*____ptr));   \
>> > -       arch_cmpxchg64_local(____ptr, (old), (new));    \
>> > +       __typeof__(ptr) ____ptr64 = (ptr);              \
>> > +       kasan_check_write(____ptr64, sizeof(*____ptr64));\
>> > +       arch_cmpxchg64_local(____ptr64, (old), (new));  \
>> >  })
>> >
>> >  #define cmpxchg_double(p1, p2, o1, o2, n1, n2)                         \
>>
>>
>> Doh! Thanks for fixing this. I think I've a similar crash in a
>> different place when I developed the patch.
>> The problem is that when we do:
>>
>>        __typeof__(ptr) ____ptr = (ptr);                \
>>        arch_cmpxchg64_local(____ptr, (old), (new));    \
>>
>> We don't necessary pass value of our just declared ____ptr to
>> arch_cmpxchg64_local(). We just pass a symbolic identifier. So if
>> arch_cmpxchg64_local() declares own ____ptr and then tries to use what
>> we passed ("____ptr") it will actually refer to own variable declared
>> rather than to what we wanted to pass in.
>>
>> In my case I ended up with something like:
>>
>> __typeof__(foo) __ptr = __ptr;
>>
>> which compiler decided to turn into 0.
>>
>> Thank you, macros.
>>
>> We can add more underscores, but the problem can happen again. Should
>> we prefix current function/macro name to all local vars?..
>
> Actually we can void that ___ptr dance completely.
>
> Thanks,
>
>         tglx
>
> 8<--------------------
>
> --- a/include/asm-generic/atomic-instrumented.h
> +++ b/include/asm-generic/atomic-instrumented.h
> @@ -359,37 +359,32 @@ static __always_inline bool atomic64_add
>
>  #define cmpxchg(ptr, old, new)                         \
>  ({                                                     \
> -       __typeof__(ptr) ___ptr = (ptr);                 \
> -       kasan_check_write(___ptr, sizeof(*___ptr));     \
> +       kasan_check_write((ptr), sizeof(*(ptr)));       \
>         arch_cmpxchg((ptr), (old), (new));              \
>  })
>
>  #define sync_cmpxchg(ptr, old, new)                    \
>  ({                                                     \
> -       __typeof__(ptr) ___ptr = (ptr);                 \
> -       kasan_check_write(___ptr, sizeof(*___ptr));     \
> -       arch_sync_cmpxchg(___ptr, (old), (new));        \
> +       kasan_check_write((ptr), sizeof(*(ptr)));       \
> +       arch_sync_cmpxchg((ptr), (old), (new));         \
>  })
>
>  #define cmpxchg_local(ptr, old, new)                   \
>  ({                                                     \
> -       __typeof__(ptr) ____ptr = (ptr);                \
> -       kasan_check_write(____ptr, sizeof(*____ptr));   \
> -       arch_cmpxchg_local(____ptr, (old), (new));      \
> +       kasan_check_write((ptr), sizeof(*(ptr)));       \
> +       arch_cmpxchg_local((ptr), (old), (new));        \


/\/\/\/\/\/\/\/\/\/\/\/\

These are macros.
If ptr is foo(), then we will call foo() twice.


>  })
>
>  #define cmpxchg64(ptr, old, new)                       \
>  ({                                                     \
> -       __typeof__(ptr) ____ptr = (ptr);                \
> -       kasan_check_write(____ptr, sizeof(*____ptr));   \
> -       arch_cmpxchg64(____ptr, (old), (new));          \
> +       kasan_check_write((ptr), sizeof(*(ptr)));       \
> +       arch_cmpxchg64((ptr), (old), (new));            \
>  })
>
>  #define cmpxchg64_local(ptr, old, new)                 \
>  ({                                                     \
> -       __typeof__(ptr) ____ptr = (ptr);                \
> -       kasan_check_write(____ptr, sizeof(*____ptr));   \
> -       arch_cmpxchg64_local(____ptr, (old), (new));    \
> +       kasan_check_write((ptr), sizeof(*(ptr)));       \
> +       arch_cmpxchg64_local((ptr), (old), (new));      \
>  })
>
>  /*
>

[toc] | [prev] | [next] | [standalone]

#1676566 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, 28 Jun 2017, Dmitry Vyukov wrote:
> On Wed, Jun 28, 2017 at 1:10 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >> In my case I ended up with something like:
> >>
> >> __typeof__(foo) __ptr = __ptr;
> >>
> >> which compiler decided to turn into 0.
> >>
> >> Thank you, macros.
> >>
> >> We can add more underscores, but the problem can happen again. Should
> >> we prefix current function/macro name to all local vars?..
> >
> > Actually we can void that ___ptr dance completely.
> >
> > Thanks,
> >
> >         tglx
> >
> > 8<--------------------
> >
> > --- a/include/asm-generic/atomic-instrumented.h
> > +++ b/include/asm-generic/atomic-instrumented.h
> > @@ -359,37 +359,32 @@ static __always_inline bool atomic64_add
> >
> >  #define cmpxchg(ptr, old, new)                         \
> >  ({                                                     \
> > -       __typeof__(ptr) ___ptr = (ptr);                 \
> > -       kasan_check_write(___ptr, sizeof(*___ptr));     \
> > +       kasan_check_write((ptr), sizeof(*(ptr)));       \
> >         arch_cmpxchg((ptr), (old), (new));              \
> >  })
> >
> >  #define sync_cmpxchg(ptr, old, new)                    \
> >  ({                                                     \
> > -       __typeof__(ptr) ___ptr = (ptr);                 \
> > -       kasan_check_write(___ptr, sizeof(*___ptr));     \
> > -       arch_sync_cmpxchg(___ptr, (old), (new));        \
> > +       kasan_check_write((ptr), sizeof(*(ptr)));       \
> > +       arch_sync_cmpxchg((ptr), (old), (new));         \
> >  })
> >
> >  #define cmpxchg_local(ptr, old, new)                   \
> >  ({                                                     \
> > -       __typeof__(ptr) ____ptr = (ptr);                \
> > -       kasan_check_write(____ptr, sizeof(*____ptr));   \
> > -       arch_cmpxchg_local(____ptr, (old), (new));      \
> > +       kasan_check_write((ptr), sizeof(*(ptr)));       \
> > +       arch_cmpxchg_local((ptr), (old), (new));        \
> 
> 
> /\/\/\/\/\/\/\/\/\/\/\/\
> 
> These are macros.
> If ptr is foo(), then we will call foo() twice.

Sigh, is that actually used?

That's all insane. The whole crap gets worse because:

      cmpxchg() can be used on u8, u16, u32 ....

There is a reason why type safety matters.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1676625 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, Jun 28, 2017 at 01:21:43PM +0200, Thomas Gleixner wrote:
> On Wed, 28 Jun 2017, Dmitry Vyukov wrote:
> > On Wed, Jun 28, 2017 at 1:10 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >> In my case I ended up with something like:
> > >>
> > >> __typeof__(foo) __ptr = __ptr;
> > >>
> > >> which compiler decided to turn into 0.
> > >>
> > >> Thank you, macros.
> > >>
> > >> We can add more underscores, but the problem can happen again. Should
> > >> we prefix current function/macro name to all local vars?..
> > >
> > > Actually we can void that ___ptr dance completely.
> > >
> > > Thanks,
> > >
> > >         tglx
> > >
> > > 8<--------------------
> > >
> > > --- a/include/asm-generic/atomic-instrumented.h
> > > +++ b/include/asm-generic/atomic-instrumented.h
> > > @@ -359,37 +359,32 @@ static __always_inline bool atomic64_add
> > >
> > >  #define cmpxchg(ptr, old, new)                         \
> > >  ({                                                     \
> > > -       __typeof__(ptr) ___ptr = (ptr);                 \
> > > -       kasan_check_write(___ptr, sizeof(*___ptr));     \
> > > +       kasan_check_write((ptr), sizeof(*(ptr)));       \
> > >         arch_cmpxchg((ptr), (old), (new));              \
> > >  })
> > >
> > >  #define sync_cmpxchg(ptr, old, new)                    \
> > >  ({                                                     \
> > > -       __typeof__(ptr) ___ptr = (ptr);                 \
> > > -       kasan_check_write(___ptr, sizeof(*___ptr));     \
> > > -       arch_sync_cmpxchg(___ptr, (old), (new));        \
> > > +       kasan_check_write((ptr), sizeof(*(ptr)));       \
> > > +       arch_sync_cmpxchg((ptr), (old), (new));         \
> > >  })
> > >
> > >  #define cmpxchg_local(ptr, old, new)                   \
> > >  ({                                                     \
> > > -       __typeof__(ptr) ____ptr = (ptr);                \
> > > -       kasan_check_write(____ptr, sizeof(*____ptr));   \
> > > -       arch_cmpxchg_local(____ptr, (old), (new));      \
> > > +       kasan_check_write((ptr), sizeof(*(ptr)));       \
> > > +       arch_cmpxchg_local((ptr), (old), (new));        \
> > 
> > 
> > /\/\/\/\/\/\/\/\/\/\/\/\
> > 
> > These are macros.
> > If ptr is foo(), then we will call foo() twice.
> 
> Sigh, is that actually used?

For better or worse, we can't rule it out.

We'd risk even more subtle bugs in future trying to rely on that not
being the case. :/

> That's all insane. The whole crap gets worse because:
> 
>       cmpxchg() can be used on u8, u16, u32 ....

Yup, that's the whole reason for the macro insanity in the fist place.

Anoother option is something like:

static inline unsigned long
cmpxchg_size(unsigned long *ptr, unsigned long old, unsigned long new, int size)
{
	kasan_check_write(ptr, size);

	switch (size) {
	case 1:
		return arch_cmpxchg((u8 *)ptr, (u8)old, (u8)new);
	case 2:
		return arch_cmpxchg((u16 *)ptr, (u16)old, (u16)new);
	case 4:
		return arch_cmpxchg((u32 *)ptr, (u32)old, (u32)new);
	case 8:
		return arch_cmpxchg((u64 *)ptr, (u64)old, (u64)new);
	}

	BUILD_BUG();
}

#define cmpxchg(ptr, old, new)	\
	cmpxchg_size(ptr, old, new, sizeof(*ptr))

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1676613 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, Jun 28, 2017 at 2:24 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 28 Jun 2017, Dmitry Vyukov wrote:
>> On Wed, Jun 28, 2017 at 1:10 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> >  #define cmpxchg_local(ptr, old, new)                   \
>> >  ({                                                     \
>> > -       __typeof__(ptr) ____ptr = (ptr);                \
>> > -       kasan_check_write(____ptr, sizeof(*____ptr));   \
>> > -       arch_cmpxchg_local(____ptr, (old), (new));      \
>> > +       kasan_check_write((ptr), sizeof(*(ptr)));       \
>> > +       arch_cmpxchg_local((ptr), (old), (new));        \
>>
>>
>> /\/\/\/\/\/\/\/\/\/\/\/\
>>
>> These are macros.
>> If ptr is foo(), then we will call foo() twice.
>
> If that's true, the foo() will be evaluated a gazillion more times down the
> way to the end of this macro maze.

No. If we do:

__typeof__(ptr) ____ptr = (ptr);

and then only use ____ptr, then ptr is evaluated only once regardless
of what the rest of macros do.

[toc] | [prev] | [next] | [standalone]

#1676660 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, 28 Jun 2017, Dmitry Vyukov wrote:
> On Wed, Jun 28, 2017 at 2:24 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Wed, 28 Jun 2017, Dmitry Vyukov wrote:
> >> On Wed, Jun 28, 2017 at 1:10 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >> >  #define cmpxchg_local(ptr, old, new)                   \
> >> >  ({                                                     \
> >> > -       __typeof__(ptr) ____ptr = (ptr);                \
> >> > -       kasan_check_write(____ptr, sizeof(*____ptr));   \
> >> > -       arch_cmpxchg_local(____ptr, (old), (new));      \
> >> > +       kasan_check_write((ptr), sizeof(*(ptr)));       \
> >> > +       arch_cmpxchg_local((ptr), (old), (new));        \
> >>
> >>
> >> /\/\/\/\/\/\/\/\/\/\/\/\
> >>
> >> These are macros.
> >> If ptr is foo(), then we will call foo() twice.
> >
> > If that's true, the foo() will be evaluated a gazillion more times down the
> > way to the end of this macro maze.
> 
> No. If we do:
> 
> __typeof__(ptr) ____ptr = (ptr);
> 
> and then only use ____ptr, then ptr is evaluated only once regardless
> of what the rest of macros do.

What I meant is, that we have today nested macros which do a lot of that
same nonsense even w/o that patch.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1676618 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, 28 Jun 2017, Dmitry Vyukov wrote:
> On Wed, Jun 28, 2017 at 1:10 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >  #define cmpxchg_local(ptr, old, new)                   \
> >  ({                                                     \
> > -       __typeof__(ptr) ____ptr = (ptr);                \
> > -       kasan_check_write(____ptr, sizeof(*____ptr));   \
> > -       arch_cmpxchg_local(____ptr, (old), (new));      \
> > +       kasan_check_write((ptr), sizeof(*(ptr)));       \
> > +       arch_cmpxchg_local((ptr), (old), (new));        \
> 
> 
> /\/\/\/\/\/\/\/\/\/\/\/\
> 
> These are macros.
> If ptr is foo(), then we will call foo() twice.

If that's true, the foo() will be evaluated a gazillion more times down the
way to the end of this macro maze.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1676608 — Re: [PATCH] locking/atomics: don't alias ____ptr

On 2017-06-28 14:15:18 [+0300], Andrey Ryabinin wrote:
> The main problem here is that arch_cmpxchg64_local() calls cmpxhg_local() instead of using arch_cmpxchg_local().
> 
> So, the patch bellow should fix the problem, also this will fix double instrumentation of cmpcxchg64[_local]().
> But I haven't tested this patch yet.

tested, works. Next step?

> ---
>  arch/x86/include/asm/cmpxchg_64.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cmpxchg_64.h b/arch/x86/include/asm/cmpxchg_64.h
> index fafaebacca2d..7046a3cc2493 100644
> --- a/arch/x86/include/asm/cmpxchg_64.h
> +++ b/arch/x86/include/asm/cmpxchg_64.h
> @@ -9,13 +9,13 @@ static inline void set_64bit(volatile u64 *ptr, u64 val)
>  #define arch_cmpxchg64(ptr, o, n)					\
>  ({									\
>  	BUILD_BUG_ON(sizeof(*(ptr)) != 8);				\
> -	cmpxchg((ptr), (o), (n));					\
> +	arch_cmpxchg((ptr), (o), (n));					\
>  })
>  
>  #define arch_cmpxchg64_local(ptr, o, n)					\
>  ({									\
>  	BUILD_BUG_ON(sizeof(*(ptr)) != 8);				\
> -	cmpxchg_local((ptr), (o), (n));					\
> +	arch_cmpxchg_local((ptr), (o), (n));					\
>  })
>  
>  #define system_has_cmpxchg_double() boot_cpu_has(X86_FEATURE_CX16)

Sebastian

[toc] | [prev] | [next] | [standalone]

#1676655 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, 28 Jun 2017, Sebastian Andrzej Siewior wrote:
> On 2017-06-28 14:15:18 [+0300], Andrey Ryabinin wrote:
> > The main problem here is that arch_cmpxchg64_local() calls cmpxhg_local() instead of using arch_cmpxchg_local().
> > 
> > So, the patch bellow should fix the problem, also this will fix double instrumentation of cmpcxchg64[_local]().
> > But I haven't tested this patch yet.
> 
> tested, works. Next step?

Check all other implementations in every architecture whether there is a
similar problem .....

But this really want's a proper cleanup unless we want to waste the time
over and over again with the next hard to figure out macro expansion fail.

First of all, cmpxchg64[_local]() can be implemented as inlines right away.

For cmpxchg*(), the situation is slightly different, but the sizeof()
evaluation should be done at the top most level, even if we do it further
down in the low level arch/asm-generic implementation once more.

Something along the lines of:

static inline unsigned long cmpxchg_varsize(void *ptr, unsigned long old,
					    unsigned long new, int size)
{
	switch (size) {
	case 1:
	case 2:
	case 4:
		break;
	case 8:
		if (sizeof(unsigned long) == 8)
			break;
	default:
		BUILD_BUG_ON(1);
	}
	kasan_check(ptr, size);
	return arch_cmpxchg(ptr, old, new);
}

#define cmpxchg(ptr, o, n)						\
({									\
	((__typeof__(*(ptr)))cmpxchg_varsize((ptr), (unsigned long)(o), \
			     (unsigned long)(n), sizeof(*(ptr))));	\
})

That's the first step to cure the actual mess.

Ideally we get rid of that whole macro maze and convert everything to
proper inlines with actual cmpxchg8/16/32/64() variants, but that's going
to take some time. As an intermediate step we can at least propagate 'size'
to arch_cmpxchg(), which is not that much of an effort.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1676685 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, 28 Jun 2017, Thomas Gleixner wrote:
> On Wed, 28 Jun 2017, Sebastian Andrzej Siewior wrote:
> > On 2017-06-28 14:15:18 [+0300], Andrey Ryabinin wrote:
> > > The main problem here is that arch_cmpxchg64_local() calls cmpxhg_local() instead of using arch_cmpxchg_local().
> > > 
> > > So, the patch bellow should fix the problem, also this will fix double instrumentation of cmpcxchg64[_local]().
> > > But I haven't tested this patch yet.
> > 
> > tested, works. Next step?
> 
> Check all other implementations in every architecture whether there is a
> similar problem .....
> 
> But this really want's a proper cleanup unless we want to waste the time
> over and over again with the next hard to figure out macro expansion fail.
> 
> First of all, cmpxchg64[_local]() can be implemented as inlines right away.
> 
> For cmpxchg*(), the situation is slightly different, but the sizeof()
> evaluation should be done at the top most level, even if we do it further
> down in the low level arch/asm-generic implementation once more.
> 
> Something along the lines of:
> 
> static inline unsigned long cmpxchg_varsize(void *ptr, unsigned long old,
> 					    unsigned long new, int size)
> {
> 	switch (size) {
> 	case 1:
> 	case 2:
> 	case 4:
> 		break;
> 	case 8:
> 		if (sizeof(unsigned long) == 8)
> 			break;
> 	default:
> 		BUILD_BUG_ON(1);
> 	}
> 	kasan_check(ptr, size);
> 	return arch_cmpxchg(ptr, old, new);
> }
> 
> #define cmpxchg(ptr, o, n)						\
> ({									\
> 	((__typeof__(*(ptr)))cmpxchg_varsize((ptr), (unsigned long)(o), \
> 			     (unsigned long)(n), sizeof(*(ptr))));	\
> })
> 
> That's the first step to cure the actual mess.
> 
> Ideally we get rid of that whole macro maze and convert everything to
> proper inlines with actual cmpxchg8/16/32/64() variants, but that's going
> to take some time. As an intermediate step we can at least propagate 'size'
> to arch_cmpxchg(), which is not that much of an effort.

And to be honest. That should have be done in the first place _BEFORE_
adding that atomic-instrumented stuff. I'm tempted to revert that mess
instead of 'fixing' it half arsed.

As a side note, we have files (aside of x86/asm/atomic.h) which include
asm/cmpxchg.h ...

net/sunrpc/xprtmultipath.c:#include <asm/cmpxchg.h>
arch/x86/kvm/mmu.c:#include <asm/cmpxchg.h>
arch/x86/um/asm/barrier.h:#include <asm/cmpxchg.h>

I'm really tired of all this featuritis crammed into the code without much
thought. Dammit, can we please stop this and clean up the existing mess
first before duct taping more mess on top of it.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1676699 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, Jun 28, 2017 at 03:54:42PM +0200, Thomas Gleixner wrote:
> On Wed, 28 Jun 2017, Thomas Gleixner wrote:
> > On Wed, 28 Jun 2017, Sebastian Andrzej Siewior wrote:
> > > On 2017-06-28 14:15:18 [+0300], Andrey Ryabinin wrote:
> > > > The main problem here is that arch_cmpxchg64_local() calls cmpxhg_local() instead of using arch_cmpxchg_local().
> > > > 
> > > > So, the patch bellow should fix the problem, also this will fix double instrumentation of cmpcxchg64[_local]().
> > > > But I haven't tested this patch yet.
> > > 
> > > tested, works. Next step?
> > 
> > Check all other implementations in every architecture whether there is a
> > similar problem .....

FWIW, as x86 is the only user of atomic-instrumented.h, any similar
issues are unrelated to this series.

That's not to say they don't exist, just that they're orthognal to this.

I've been reworking things for arm64 [1], but there's more cleanup
needed first.

> > But this really want's a proper cleanup unless we want to waste the time
> > over and over again with the next hard to figure out macro expansion fail.
> > 
> > First of all, cmpxchg64[_local]() can be implemented as inlines right away.
> > 
> > For cmpxchg*(), the situation is slightly different, but the sizeof()
> > evaluation should be done at the top most level, even if we do it further
> > down in the low level arch/asm-generic implementation once more.
> > 
> > Something along the lines of:
> > 
> > static inline unsigned long cmpxchg_varsize(void *ptr, unsigned long old,
> > 					    unsigned long new, int size)
> > {
> > 	switch (size) {
> > 	case 1:
> > 	case 2:
> > 	case 4:
> > 		break;
> > 	case 8:
> > 		if (sizeof(unsigned long) == 8)
> > 			break;
> > 	default:
> > 		BUILD_BUG_ON(1);
> > 	}
> > 	kasan_check(ptr, size);
> > 	return arch_cmpxchg(ptr, old, new);
> > }

This'll need to re-cast things before the call to arch_cmpxchg(), and we
can move the check above the switch, as in [2].

> > #define cmpxchg(ptr, o, n)						\
> > ({									\
> > 	((__typeof__(*(ptr)))cmpxchg_varsize((ptr), (unsigned long)(o), \
> > 			     (unsigned long)(n), sizeof(*(ptr))));	\
> > })
> > 
> > That's the first step to cure the actual mess.
> > 
> > Ideally we get rid of that whole macro maze and convert everything to
> > proper inlines with actual cmpxchg8/16/32/64() variants, but that's going
> > to take some time. As an intermediate step we can at least propagate 'size'
> > to arch_cmpxchg(), which is not that much of an effort.
> 
> And to be honest. That should have be done in the first place _BEFORE_
> adding that atomic-instrumented stuff. I'm tempted to revert that mess
> instead of 'fixing' it half arsed.

Sure.

Let's figure out what this *should* look like first.

If that's sufficiently different to what we have now, we revert this and
clean things up first.

> As a side note, we have files (aside of x86/asm/atomic.h) which include
> asm/cmpxchg.h ...
> 
> net/sunrpc/xprtmultipath.c:#include <asm/cmpxchg.h>
> arch/x86/kvm/mmu.c:#include <asm/cmpxchg.h>
> arch/x86/um/asm/barrier.h:#include <asm/cmpxchg.h>

Ugh. I'd sent out a patch [3] for the first of these a while back, as I
spotted that when experimenting with arm64, but tht got dropped on the
floor.

I can resend that, if you like?

I guess it'd also make sense to fix the x86 bits at the same time, so
I'm fine with tahat being folded with other fixes.

> I'm really tired of all this featuritis crammed into the code without much
> thought. Dammit, can we please stop this and clean up the existing mess
> first before duct taping more mess on top of it.

Sorry for adding to the mess here.

Thanks,
Mark.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/kasan-atomic
[2] https://lkml.kernel.org/r/20170628124552.GG5981@leverpostej
[3] http://lkml.kernel.org/r/1489574142-20856-1-git-send-email-mark.rutland@arm.com

[toc] | [prev] | [next] | [standalone]

#1676834 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, 28 Jun 2017, Mark Rutland wrote:
> On Wed, Jun 28, 2017 at 03:54:42PM +0200, Thomas Gleixner wrote:
> > > static inline unsigned long cmpxchg_varsize(void *ptr, unsigned long old,
> > > 					    unsigned long new, int size)
> > > {
> > > 	switch (size) {
> > > 	case 1:
> > > 	case 2:
> > > 	case 4:
> > > 		break;
> > > 	case 8:
> > > 		if (sizeof(unsigned long) == 8)
> > > 			break;
> > > 	default:
> > > 		BUILD_BUG_ON(1);
> > > 	}
> > > 	kasan_check(ptr, size);
> > > 	return arch_cmpxchg(ptr, old, new);
> > > }
> 
> This'll need to re-cast things before the call to arch_cmpxchg(), and we
> can move the check above the switch, as in [2].

Sure, but I rather see that changed to:

1) Create arch_cmpxchg8/16/32/64() inlines first

2) Add that varsize wrapper:

static inline unsigned long cmpxchg_varsize(void *ptr, unsigned long old,
                                            unsigned long new, int size)
{
        switch (size) {
        case 1:
                kasan_check_write(ptr, size);
                return arch_cmpxchg8((u8 *)ptr, (u8) old, (u8)new);
        case 2:
                kasan_check_write(ptr, size);
                return arch_cmpxchg16((u16 *)ptr, (u16) old, (u16)new);
        case 4:
                kasan_check_write(ptr, size);
                return arch_cmpxchg32((u32 *)ptr, (u32) old, (u32)new);
        case 8:
                if (sizeof(unsigned long) == 8) {
                        kasan_check_write(ptr, size);
                        return arch_cmpxchg64((u64 *)ptr, (u64) old, (u64)new);
                }
        default:
                BUILD_BUG();
        }
}

#define cmpxchg(ptr, o, n)                                              \
({                                                                      \
        ((__typeof__(*(ptr)))cmpxchg_varsize((ptr), (unsigned long)(o), \
                             (unsigned long)(n), sizeof(*(ptr))));      \
})

Which allows us to create:

static inline u8 cmpxchg8(u8 *ptr, u8 old, u8 new)
{
	kasan_check_write(ptr, sizeof(old));
	return arch_cmpxchg8(ptr, old, new);
}

and friends as well and later migrate the existing users away from that
untyped macro mess.

And instead of adding

    #include <asm/atomic-instrumented.h>

to the architecture code, we rather do

# mv arch/xxx/include/asm/atomic.h mv arch/xxx/include/asm/arch_atomic.h
# echo '#include <asm-generic/atomic.h>' >arch/xxx/include/asm/atomic.h

# mv include/asm-generic/atomic.h include/asm-generic/atomic_up.h

and create a new include/asm-generic/atomic.h

#ifndef __ASM_GENERIC_ATOMIC_H
#define __ASM_GENERIC_ATOMIC_H

#ifdef CONFIG_ATOMIC_INSTRUMENTED_H
#include <asm-generic/atomic_instrumented.h>
#else
#include <asm-generic/atomic_up.h>
#endif

#endif

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1676878 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, Jun 28, 2017 at 05:24:24PM +0200, Thomas Gleixner wrote:
> On Wed, 28 Jun 2017, Mark Rutland wrote:
> > On Wed, Jun 28, 2017 at 03:54:42PM +0200, Thomas Gleixner wrote:
> > > > static inline unsigned long cmpxchg_varsize(void *ptr, unsigned long old,
> > > > 					    unsigned long new, int size)
> > > > {
> > > > 	switch (size) {
> > > > 	case 1:
> > > > 	case 2:
> > > > 	case 4:
> > > > 		break;
> > > > 	case 8:
> > > > 		if (sizeof(unsigned long) == 8)
> > > > 			break;
> > > > 	default:
> > > > 		BUILD_BUG_ON(1);
> > > > 	}
> > > > 	kasan_check(ptr, size);
> > > > 	return arch_cmpxchg(ptr, old, new);
> > > > }
> > 
> > This'll need to re-cast things before the call to arch_cmpxchg(), and we
> > can move the check above the switch, as in [2].
> 
> Sure, but I rather see that changed to:
> 
> 1) Create arch_cmpxchg8/16/32/64() inlines first
> 
> 2) Add that varsize wrapper:
> 
> static inline unsigned long cmpxchg_varsize(void *ptr, unsigned long old,
>                                             unsigned long new, int size)
> {
>         switch (size) {
>         case 1:
>                 kasan_check_write(ptr, size);
>                 return arch_cmpxchg8((u8 *)ptr, (u8) old, (u8)new);
>         case 2:
>                 kasan_check_write(ptr, size);
>                 return arch_cmpxchg16((u16 *)ptr, (u16) old, (u16)new);
>         case 4:
>                 kasan_check_write(ptr, size);
>                 return arch_cmpxchg32((u32 *)ptr, (u32) old, (u32)new);
>         case 8:
>                 if (sizeof(unsigned long) == 8) {
>                         kasan_check_write(ptr, size);
>                         return arch_cmpxchg64((u64 *)ptr, (u64) old, (u64)new);
>                 }
>         default:
>                 BUILD_BUG();
>         }
> }
> 
> #define cmpxchg(ptr, o, n)                                              \
> ({                                                                      \
>         ((__typeof__(*(ptr)))cmpxchg_varsize((ptr), (unsigned long)(o), \
>                              (unsigned long)(n), sizeof(*(ptr))));      \
> })
> 
> Which allows us to create:
> 
> static inline u8 cmpxchg8(u8 *ptr, u8 old, u8 new)
> {
> 	kasan_check_write(ptr, sizeof(old));
> 	return arch_cmpxchg8(ptr, old, new);
> }
> 
> and friends as well and later migrate the existing users away from that
> untyped macro mess.

Sure, that makes sense to me.

> 
> And instead of adding
> 
>     #include <asm/atomic-instrumented.h>
> 
> to the architecture code, we rather do
> 
> # mv arch/xxx/include/asm/atomic.h mv arch/xxx/include/asm/arch_atomic.h
> # echo '#include <asm-generic/atomic.h>' >arch/xxx/include/asm/atomic.h
> 
> # mv include/asm-generic/atomic.h include/asm-generic/atomic_up.h
> 
> and create a new include/asm-generic/atomic.h
> 
> #ifndef __ASM_GENERIC_ATOMIC_H
> #define __ASM_GENERIC_ATOMIC_H
> 
> #ifdef CONFIG_ATOMIC_INSTRUMENTED_H
> #include <asm-generic/atomic_instrumented.h>
> #else
> #include <asm-generic/atomic_up.h>
> #endif
> 
> #endif

Given we're gonig to clean things up, we may as well avoid the backwards
include of <asm-generic/atomic_instrumented.h>, whcih was only there as
a bodge:

For the UP arches we do:
# echo '#include <asm-generic/atomic_up.h>' >arch/xxx/include/asm/atomic.h
# mv include/asm-generic/atomic.h include/asm-generic/atomic_up.h

Then we add a <linux/atomic_instrumented.h>:

#ifndef __LINUX_ATOMIC_INSTRUMENTED_H
#define __LINUX_ATOMIC INSTRUMENTED_H

#include <asm/atomic.h>

#if CONFIG_ATOMIC_INSTRUMENTED_H
<instrumentation>
#endif

#endif /* __LINUX_ATOMIC_ARCH_H */

... and make <linux/atomic.h> incldue that rather than <asm/atomic.h>.

That way the instrumentation's orthogonal to the UP-ness of the arch,
and we can fold any other instrumentation in there, or later move it
directly into <linux/atomic.h>

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1676938 — Re: [PATCH] locking/atomics: don't alias ____ptr

* Mark Rutland <mark.rutland@arm.com> wrote:

> > And instead of adding
> > 
> >     #include <asm/atomic-instrumented.h>
> > 
> > to the architecture code, we rather do
> > 
> > # mv arch/xxx/include/asm/atomic.h mv arch/xxx/include/asm/arch_atomic.h
> > # echo '#include <asm-generic/atomic.h>' >arch/xxx/include/asm/atomic.h
> > 
> > # mv include/asm-generic/atomic.h include/asm-generic/atomic_up.h
> > 
> > and create a new include/asm-generic/atomic.h
> > 
> > #ifndef __ASM_GENERIC_ATOMIC_H
> > #define __ASM_GENERIC_ATOMIC_H
> > 
> > #ifdef CONFIG_ATOMIC_INSTRUMENTED_H
> > #include <asm-generic/atomic_instrumented.h>
> > #else
> > #include <asm-generic/atomic_up.h>
> > #endif
> > 
> > #endif
> 
> Given we're gonig to clean things up, we may as well avoid the backwards
> include of <asm-generic/atomic_instrumented.h>, whcih was only there as
> a bodge:

So, since the final v4.12 release is so close, I've put the following KASAN 
commits aside into tip:WIP.locking/atomics:

4b47cc154eed: locking/atomic/x86, asm-generic: Add comments for atomic instrumentation
35787d9d7ca4: locking/atomics, asm-generic: Add KASAN instrumentation to atomic operations
68c1ed1fdb0a: kasan: Allow kasan_check_read/write() to accept pointers to volatiles
f1c3049f6729: locking/atomic/x86: Switch atomic.h to use atomic-instrumented.h
d079eebb3958: locking/atomic: Add asm-generic/atomic-instrumented.h
007d185b4462: locking/atomic/x86: Use 's64 *' for 'old' argument of atomic64_try_cmpxchg()
ba1c9f83f633: locking/atomic/x86: Un-macro-ify atomic ops implementation

(Note, I had to rebase these freshly, to decouple them from a refcount_t commit 
that we need.)

and won't send them to Linus unless the cleanups are done and acked by Thomas.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1677042 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, 28 Jun 2017, Mark Rutland wrote:
> On Wed, Jun 28, 2017 at 05:24:24PM +0200, Thomas Gleixner wrote:
> Given we're gonig to clean things up, we may as well avoid the backwards
> include of <asm-generic/atomic_instrumented.h>, whcih was only there as
> a bodge:
> 
> For the UP arches we do:
> # echo '#include <asm-generic/atomic_up.h>' >arch/xxx/include/asm/atomic.h
> # mv include/asm-generic/atomic.h include/asm-generic/atomic_up.h
> 
> Then we add a <linux/atomic_instrumented.h>:
> 
> #ifndef __LINUX_ATOMIC_INSTRUMENTED_H
> #define __LINUX_ATOMIC INSTRUMENTED_H
> 
> #include <asm/atomic.h>
> 
> #if CONFIG_ATOMIC_INSTRUMENTED_H
> <instrumentation>
> #endif
> 
> #endif /* __LINUX_ATOMIC_ARCH_H */
> 
> ... and make <linux/atomic.h> incldue that rather than <asm/atomic.h>.
> 
> That way the instrumentation's orthogonal to the UP-ness of the arch,
> and we can fold any other instrumentation in there, or later move it
> directly into <linux/atomic.h>

Sounds like a plan.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1677444 — Re: [PATCH] locking/atomics: don't alias ____ptr

On Wed, 28 Jun 2017, Thomas Gleixner wrote:
> On Wed, 28 Jun 2017, Mark Rutland wrote:
> > On Wed, Jun 28, 2017 at 05:24:24PM +0200, Thomas Gleixner wrote:
> > Given we're gonig to clean things up, we may as well avoid the backwards
> > include of <asm-generic/atomic_instrumented.h>, whcih was only there as
> > a bodge:
> > 
> > For the UP arches we do:
> > # echo '#include <asm-generic/atomic_up.h>' >arch/xxx/include/asm/atomic.h
> > # mv include/asm-generic/atomic.h include/asm-generic/atomic_up.h
> > 
> > Then we add a <linux/atomic_instrumented.h>:
> > 
> > #ifndef __LINUX_ATOMIC_INSTRUMENTED_H
> > #define __LINUX_ATOMIC INSTRUMENTED_H
> > 
> > #include <asm/atomic.h>
> > 
> > #if CONFIG_ATOMIC_INSTRUMENTED_H
> > <instrumentation>
> > #endif
> > 
> > #endif /* __LINUX_ATOMIC_ARCH_H */
> > 
> > ... and make <linux/atomic.h> incldue that rather than <asm/atomic.h>.
> > 
> > That way the instrumentation's orthogonal to the UP-ness of the arch,
> > and we can fold any other instrumentation in there, or later move it
> > directly into <linux/atomic.h>
> 
> Sounds like a plan.

Actually we should make it slightly different and make asm-generic/atomic.h
the central point for everything.

It should contain the wrapper macro and the central inlines including the
kasan stuff and include either arch/arch_atomic.h or
asm-generic/atomic_up.h.

That way all potential instrumentation happens in the generic header (which
is a NOP for archs which do not support it) and pull in the appropriate
arch specific or generic UP low level implementations.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web