x86, kasan: add KASAN checks to atomic operations

PATCH In review Dmitry Vyukov 7 participants 27 messages Mar 14, 2017 – Mar 30, 2017

Subsystems (5)

Memory management x86 Generic include/asm header files KASAN +1 more subsystems

lore

Share & tools

lore

Dmitry Vyukov

author reviewer

2017-03-14 19:24 UTC #0 lore raw

cover x86, kasan: add KASAN checks 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.

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, and adds KASAN checks to them.

This patch uses the wrappers only for x86 arch. Arm64 will be switched later. And we also plan to instrument bitops in a similar way.

Within a day it has found its first bug:

BUG: KASAN: use-after-free in atomic_dec_and_test
arch/x86/include/asm/atomic.h:123 [inline] at addr ffff880079c30158
Write of size 4 by task syz-executor6/25698
CPU: 2 PID: 25698 Comm: syz-executor6 Not tainted 4.10.0+ #302
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
 kasan_check_write+0x14/0x20 mm/kasan/kasan.c:344
 atomic_dec_and_test arch/x86/include/asm/atomic.h:123 [inline]
 put_task_struct include/linux/sched/task.h:93 [inline]
 put_ctx+0xcf/0x110 kernel/events/core.c:1131
 perf_event_release_kernel+0x3ad/0xc90 kernel/events/core.c:4322
 perf_release+0x37/0x50 kernel/events/core.c:4338
 __fput+0x332/0x800 fs/file_table.c:209
 ____fput+0x15/0x20 fs/file_table.c:245
 task_work_run+0x197/0x260 kernel/task_work.c:116
 exit_task_work include/linux/task_work.h:21 [inline]
 do_exit+0xb38/0x29c0 kernel/exit.c:880
 do_group_exit+0x149/0x420 kernel/exit.c:984
 get_signal+0x7e0/0x1820 kernel/signal.c:2318
 do_signal+0xd2/0x2190 arch/x86/kernel/signal.c:808
 exit_to_usermode_loop+0x200/0x2a0 arch/x86/entry/common.c:157
 syscall_return_slowpath arch/x86/entry/common.c:191 [inline]
 do_syscall_64+0x6fc/0x930 arch/x86/entry/common.c:286
 entry_SYSCALL64_slow_path+0x25/0x25
RIP: 0033:0x4458d9
RSP: 002b:00007f3f07187cf8 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca
RAX: fffffffffffffe00 RBX: 00000000007080c8 RCX: 00000000004458d9
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00000000007080c8
RBP: 00000000007080a8 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 0000000000000000 R14: 00007f3f071889c0 R15: 00007f3f07188700
Object at ffff880079c30140, in cache task_struct size: 5376
Allocated:
PID = 25681
 kmem_cache_alloc_node+0x122/0x6f0 mm/slab.c:3662
 alloc_task_struct_node kernel/fork.c:153 [inline]
 dup_task_struct kernel/fork.c:495 [inline]
 copy_process.part.38+0x19c8/0x4aa0 kernel/fork.c:1560
 copy_process kernel/fork.c:1531 [inline]
 _do_fork+0x200/0x1010 kernel/fork.c:1994
 SYSC_clone kernel/fork.c:2104 [inline]
 SyS_clone+0x37/0x50 kernel/fork.c:2098
 do_syscall_64+0x2e8/0x930 arch/x86/entry/common.c:281
 return_from_SYSCALL_64+0x0/0x7a
Freed:
PID = 25681
 __cache_free mm/slab.c:3514 [inline]
 kmem_cache_free+0x71/0x240 mm/slab.c:3774
 free_task_struct kernel/fork.c:158 [inline]
 free_task+0x151/0x1d0 kernel/fork.c:370
 copy_process.part.38+0x18e5/0x4aa0 kernel/fork.c:1931
 copy_process kernel/fork.c:1531 [inline]
 _do_fork+0x200/0x1010 kernel/fork.c:1994
 SYSC_clone kernel/fork.c:2104 [inline]
 SyS_clone+0x37/0x50 kernel/fork.c:2098
 do_syscall_64+0x2e8/0x930 arch/x86/entry/common.c:281
 return_from_SYSCALL_64+0x0/0x7a

Dmitry Vyukov (3):
kasan: allow kasan_check_read/write() to accept pointers to volatiles asm-generic, x86: wrap atomic operations
asm-generic: add KASAN instrumentation to atomic operations

 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 | 246 ++++++++++++++++++++++++++++++
 include/linux/kasan-checks.h              |  10 +-
 mm/kasan/kasan.c                          |   4 +-
 9 files changed, 411 insertions(+), 149 deletions(-)
 create mode 100644 include/asm-generic/atomic-instrumented.h

Signature · 7 lines

2.12.0.367.g23dc2f6d3c-goog

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Dmitry Vyukov

author reviewer

2017-03-14 19:24 UTC #1 lore raw

PATCH 1/3 kasan: allow kasan_check_read/write() to accept pointers to volatiles

Currently kasan_check_read/write() accept 'const void*', make them accept 'const volatile void*'. This is required for instrumentation of atomic operations and there is just no reason to not allow that.

Signed-off-by Dmitry Vyukov <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Will Deacon <[email protected]>,

Cc Andrew Morton <[email protected]>,

Cc Andrey Ryabinin <[email protected]>,

Cc Ingo Molnar <[email protected]>,

Cc [email protected]

---
 include/linux/kasan-checks.h | 10 ++++++----
 mm/kasan/kasan.c             |  4 ++--
 2 files changed, 8 insertions(+), 6 deletions(-)

include/linux/kasan-checks.h +6 -4

index b7f8aced7870..41960fecf783 100644
@@ -2,11 +2,13 @@
 #define _LINUX_KASAN_CHECKS_H
 
 #ifdef CONFIG_KASAN
-void kasan_check_read(const void *p, unsigned int size);
-void kasan_check_write(const void *p, unsigned int size);
+void kasan_check_read(const volatile void *p, unsigned int size);
+void kasan_check_write(const volatile void *p, unsigned int size);
 #else
-static inline void kasan_check_read(const void *p, unsigned int size) { }
-static inline void kasan_check_write(const void *p, unsigned int size) { }
+static inline void kasan_check_read(const volatile void *p, unsigned int size)
+{ }
+static inline void kasan_check_write(const volatile void *p, unsigned int size)
+{ }
 #endif
 
 #endif

mm/kasan/kasan.c +2 -2

index 98b27195e38b..db46e66eb1d4 100644
@@ -333,13 +333,13 @@ static void check_memory_region(unsigned long addr,
 	check_memory_region_inline(addr, size, write, ret_ip);
 }
 
-void kasan_check_read(const void *p, unsigned int size)
+void kasan_check_read(const volatile void *p, unsigned int size)
 {
 	check_memory_region((unsigned long)p, size, false, _RET_IP_);
 }
 EXPORT_SYMBOL(kasan_check_read);
 
-void kasan_check_write(const void *p, unsigned int size)
+void kasan_check_write(const volatile void *p, unsigned int size)
 {
 	check_memory_region((unsigned long)p, size, true, _RET_IP_);
 }

Signature · 7 lines

2.12.0.367.g23dc2f6d3c-goog

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Dmitry Vyukov

author reviewer

2017-03-14 19:24 UTC #2 lore raw

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

In reply to Dmitry Vyukov

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 <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Will Deacon <[email protected]>,

Cc Andrew Morton <[email protected]>,

Cc Andrey Ryabinin <[email protected]>,

Cc Ingo Molnar <[email protected]>,

Cc [email protected]

---
 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(-)

arch/x86/include/asm/atomic.h +56 ~~-44~~

index 14635c5ea025..95dd167eb3af 100644
@@ -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);
 }
 
 /**
- * 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)
 {
+	/*
+	 * We could use WRITE_ONCE_NOCHECK() if it exists, similar to
+	 * READ_ONCE_NOCHECK() in arch_atomic_read(). But there is no such
+	 * thing at the moment, and introducing it for this case does not
+	 * worth it.
+	 */
 	WRITE_ONCE(v->counter, i);
 }
 
 /**
- * atomic_add - add integer to atomic variable
+ * arch_atomic_add - add integer to atomic variable
  * @i: integer value to add
  * @v: pointer of type atomic_t
  *
  * Atomically adds @i to @v.
  */
-static __always_inline void atomic_add(int i, atomic_t *v)
+static __always_inline void arch_atomic_add(int i, atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "addl %1,%0"
 		     : "+m" (v->counter)

@@ -53,13 +63,13 @@ static __always_inline void atomic_add(int i, atomic_t *v)
 }
 
 /**
- * atomic_sub - subtract integer from atomic variable
+ * arch_atomic_sub - subtract integer from atomic variable
  * @i: integer value to subtract
  * @v: pointer of type atomic_t
  *
  * Atomically subtracts @i from @v.
  */
-static __always_inline void atomic_sub(int i, atomic_t *v)
+static __always_inline void arch_atomic_sub(int i, atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "subl %1,%0"
 		     : "+m" (v->counter)

@@ -67,7 +77,7 @@ static __always_inline void atomic_sub(int i, atomic_t *v)
 }
 
 /**
- * atomic_sub_and_test - subtract value from variable and test result
+ * arch_atomic_sub_and_test - subtract value from variable and test result
  * @i: integer value to subtract
  * @v: pointer of type atomic_t
  *

@@ -75,63 +85,63 @@ static __always_inline void atomic_sub(int i, atomic_t *v)
  * true if the result is zero, or false for all
  * other cases.
  */
-static __always_inline bool atomic_sub_and_test(int i, atomic_t *v)
+static __always_inline bool arch_atomic_sub_and_test(int i, atomic_t *v)
 {
 	GEN_BINARY_RMWcc(LOCK_PREFIX "subl", v->counter, "er", i, "%0", e);
 }
 
 /**
- * atomic_inc - increment atomic variable
+ * arch_atomic_inc - increment atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1.
  */
-static __always_inline void atomic_inc(atomic_t *v)
+static __always_inline void arch_atomic_inc(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "incl %0"
 		     : "+m" (v->counter));
 }
 
 /**
- * atomic_dec - decrement atomic variable
+ * arch_atomic_dec - decrement atomic variable
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1.
  */
-static __always_inline void atomic_dec(atomic_t *v)
+static __always_inline void arch_atomic_dec(atomic_t *v)
 {
 	asm volatile(LOCK_PREFIX "decl %0"
 		     : "+m" (v->counter));
 }
 
 /**
- * atomic_dec_and_test - decrement and test
+ * arch_atomic_dec_and_test - decrement and test
  * @v: pointer of type atomic_t
  *
  * Atomically decrements @v by 1 and
  * returns true if the result is 0, or false for all other
  * cases.
  */
-static __always_inline bool atomic_dec_and_test(atomic_t *v)
+static __always_inline bool arch_atomic_dec_and_test(atomic_t *v)
 {
 	GEN_UNARY_RMWcc(LOCK_PREFIX "decl", v->counter, "%0", e);
 }
 
 /**
- * atomic_inc_and_test - increment and test
+ * arch_atomic_inc_and_test - increment and test
  * @v: pointer of type atomic_t
  *
  * Atomically increments @v by 1
  * and returns true if the result is zero, or false for all
  * other cases.
  */
-static __always_inline bool atomic_inc_and_test(atomic_t *v)
+static __always_inline bool arch_atomic_inc_and_test(atomic_t *v)
 {
 	GEN_UNARY_RMWcc(LOCK_PREFIX "incl", v->counter, "%0", e);
 }
 
 /**
- * atomic_add_negative - add and test if negative
+ * arch_atomic_add_negative - add and test if negative
  * @i: integer value to add
  * @v: pointer of type atomic_t
  *

@@ -139,60 +149,60 @@ static __always_inline bool atomic_inc_and_test(atomic_t *v)
  * if the result is negative, or false when
  * result is greater than or equal to zero.
  */
-static __always_inline bool atomic_add_negative(int i, atomic_t *v)
+static __always_inline bool arch_atomic_add_negative(int i, atomic_t *v)
 {
 	GEN_BINARY_RMWcc(LOCK_PREFIX "addl", v->counter, "er", i, "%0", s);
 }
 
 /**
- * atomic_add_return - add integer and return
+ * arch_atomic_add_return - add integer and return
  * @i: integer value to add
  * @v: pointer of type atomic_t
  *
  * Atomically adds @i to @v and returns @i + @v
  */
-static __always_inline int atomic_add_return(int i, atomic_t *v)
+static __always_inline int arch_atomic_add_return(int i, atomic_t *v)
 {
 	return i + xadd(&v->counter, i);
 }
 
 /**
- * atomic_sub_return - subtract integer and return
+ * arch_atomic_sub_return - subtract integer and return
  * @v: pointer of type atomic_t
  * @i: integer value to subtract
  *
  * Atomically subtracts @i from @v and returns @v - @i
  */
-static __always_inline int atomic_sub_return(int i, atomic_t *v)
+static __always_inline int arch_atomic_sub_return(int i, atomic_t *v)
 {
-	return atomic_add_return(-i, v);
+	return arch_atomic_add_return(-i, v);
 }
 
-#define atomic_inc_return(v)  (atomic_add_return(1, v))
-#define atomic_dec_return(v)  (atomic_sub_return(1, v))
+#define arch_atomic_inc_return(v)  (arch_atomic_add_return(1, v))
+#define arch_atomic_dec_return(v)  (arch_atomic_sub_return(1, v))
 
-static __always_inline int atomic_fetch_add(int i, atomic_t *v)
+static __always_inline int arch_atomic_fetch_add(int i, atomic_t *v)
 {
 	return xadd(&v->counter, i);
 }
 
-static __always_inline int atomic_fetch_sub(int i, atomic_t *v)
+static __always_inline int arch_atomic_fetch_sub(int i, atomic_t *v)
 {
 	return xadd(&v->counter, -i);
 }
 
-static __always_inline int atomic_cmpxchg(atomic_t *v, int old, int new)
+static __always_inline int arch_atomic_cmpxchg(atomic_t *v, int old, int new)
 {
-	return cmpxchg(&v->counter, old, new);
+	return arch_cmpxchg(&v->counter, old, new);
 }
 
-static inline int atomic_xchg(atomic_t *v, int new)
+static inline int arch_atomic_xchg(atomic_t *v, int new)
 {
-	return xchg(&v->counter, new);
+	return arch_xchg(&v->counter, new);
 }
 
 #define ATOMIC_OP(op)							\
-static inline void atomic_##op(int i, atomic_t *v)			\
+static inline void arch_atomic_##op(int i, atomic_t *v)			\
 {									\
 	asm volatile(LOCK_PREFIX #op"l %1,%0"				\
 			: "+m" (v->counter)				\

@@ -201,11 +211,11 @@ static inline void atomic_##op(int i, atomic_t *v)			\
 }
 
 #define ATOMIC_FETCH_OP(op, c_op)					\
-static inline int atomic_fetch_##op(int i, atomic_t *v)		\
+static inline int arch_atomic_fetch_##op(int i, atomic_t *v)		\
 {									\
-	int old, val = atomic_read(v);					\
+	int old, val = arch_atomic_read(v);				\
 	for (;;) {							\
-		old = atomic_cmpxchg(v, val, val c_op i);		\
+		old = arch_atomic_cmpxchg(v, val, val c_op i);		\
 		if (old == val)						\
 			break;						\
 		val = old;						\

@@ -226,7 +236,7 @@ ATOMIC_OPS(xor, ^)
 #undef ATOMIC_OP
 
 /**
- * __atomic_add_unless - add unless the number is already a given value
+ * __arch_atomic_add_unless - add unless the number is already a given value
  * @v: pointer of type atomic_t
  * @a: the amount to add to v...
  * @u: ...unless v is equal to u.

@@ -234,14 +244,14 @@ ATOMIC_OPS(xor, ^)
  * Atomically adds @a to @v, so long as @v was not already @u.
  * Returns the old value of @v.
  */
-static __always_inline int __atomic_add_unless(atomic_t *v, int a, int u)
+static __always_inline int __arch_atomic_add_unless(atomic_t *v, int a, int u)
 {
 	int c, old;
-	c = atomic_read(v);
+	c = arch_atomic_read(v);
 	for (;;) {
 		if (unlikely(c == (u)))
 			break;
-		old = atomic_cmpxchg((v), c, c + (a));
+		old = arch_atomic_cmpxchg((v), c, c + (a));
 		if (likely(old == c))
 			break;
 		c = old;

@@ -250,13 +260,13 @@ static __always_inline int __atomic_add_unless(atomic_t *v, int a, int u)
 }
 
 /**
- * atomic_inc_short - increment of a short integer
+ * arch_atomic_inc_short - increment of a short integer
  * @v: pointer to type int
  *
  * Atomically adds 1 to @v
  * Returns the new value of @u
  */
-static __always_inline short int atomic_inc_short(short int *v)
+static __always_inline short int arch_atomic_inc_short(short int *v)
 {
 	asm(LOCK_PREFIX "addw $1, %0" : "+m" (*v));
 	return *v;

@@ -268,4 +278,6 @@ static __always_inline short int atomic_inc_short(short int *v)
 # include <asm/atomic64_64.h>
 #endif
 
+#include <asm-generic/atomic-instrumented.h>
+
 #endif /* _ASM_X86_ATOMIC_H */

arch/x86/include/asm/atomic64_32.h +44 ~~-42~~

281 lines not on this page · raw message

arch/x86/include/asm/atomic64_64.h +45 ~~-45~~

268 lines not on this page · raw message

arch/x86/include/asm/cmpxchg.h +6 -6

index 97848cdfcb1a..75f09809666f 100644
@@ -74,7 +74,7 @@ extern void __add_wrong_size(void)
  * use "asm volatile" and "memory" clobbers to prevent gcc from moving
  * information around.
  */
-#define xchg(ptr, v)	__xchg_op((ptr), (v), xchg, "")
+#define arch_xchg(ptr, v)	__xchg_op((ptr), (v), xchg, "")
 
 /*
  * Atomic compare and exchange.  Compare OLD with MEM, if identical,

@@ -144,13 +144,13 @@ extern void __add_wrong_size(void)
 # include <asm/cmpxchg_64.h>
 #endif
 
-#define cmpxchg(ptr, old, new)						\
+#define arch_cmpxchg(ptr, old, new)					\
 	__cmpxchg(ptr, old, new, sizeof(*(ptr)))
 
-#define sync_cmpxchg(ptr, old, new)					\
+#define arch_sync_cmpxchg(ptr, old, new)				\
 	__sync_cmpxchg(ptr, old, new, sizeof(*(ptr)))
 
-#define cmpxchg_local(ptr, old, new)					\
+#define arch_cmpxchg_local(ptr, old, new)				\
 	__cmpxchg_local(ptr, old, new, sizeof(*(ptr)))
 
 /*

@@ -179,10 +179,10 @@ extern void __add_wrong_size(void)
 	__ret;								\
 })
 
-#define cmpxchg_double(p1, p2, o1, o2, n1, n2) \
+#define arch_cmpxchg_double(p1, p2, o1, o2, n1, n2) \
 	__cmpxchg_double(LOCK_PREFIX, p1, p2, o1, o2, n1, n2)
 
-#define cmpxchg_double_local(p1, p2, o1, o2, n1, n2) \
+#define arch_cmpxchg_double_local(p1, p2, o1, o2, n1, n2) \
 	__cmpxchg_double(, p1, p2, o1, o2, n1, n2)
 
 #endif	/* ASM_X86_CMPXCHG_H */

arch/x86/include/asm/cmpxchg_32.h +4 -4

index e4959d023af8..d897291d2bf9 100644
@@ -35,10 +35,10 @@ static inline void set_64bit(volatile u64 *ptr, u64 value)
 }
 
 #ifdef CONFIG_X86_CMPXCHG64
-#define cmpxchg64(ptr, o, n)						\
+#define arch_cmpxchg64(ptr, o, n)					\
 	((__typeof__(*(ptr)))__cmpxchg64((ptr), (unsigned long long)(o), \
 					 (unsigned long long)(n)))
-#define cmpxchg64_local(ptr, o, n)					\
+#define arch_cmpxchg64_local(ptr, o, n)					\
 	((__typeof__(*(ptr)))__cmpxchg64_local((ptr), (unsigned long long)(o), \
 					       (unsigned long long)(n)))
 #endif

@@ -75,7 +75,7 @@ static inline u64 __cmpxchg64_local(volatile u64 *ptr, u64 old, u64 new)
  * to simulate the cmpxchg8b on the 80386 and 80486 CPU.
  */
 
-#define cmpxchg64(ptr, o, n)					\
+#define arch_cmpxchg64(ptr, o, n)				\
 ({								\
 	__typeof__(*(ptr)) __ret;				\
 	__typeof__(*(ptr)) __old = (o);				\

@@ -92,7 +92,7 @@ static inline u64 __cmpxchg64_local(volatile u64 *ptr, u64 old, u64 new)
 	__ret; })
 
 
-#define cmpxchg64_local(ptr, o, n)				\
+#define arch_cmpxchg64_local(ptr, o, n)				\
 ({								\
 	__typeof__(*(ptr)) __ret;				\
 	__typeof__(*(ptr)) __old = (o);				\

arch/x86/include/asm/cmpxchg_64.h +2 -2

index caa23a34c963..fafaebacca2d 100644
@@ -6,13 +6,13 @@ static inline void set_64bit(volatile u64 *ptr, u64 val)
 	*ptr = val;
 }
 
-#define cmpxchg64(ptr, o, n)						\
+#define arch_cmpxchg64(ptr, o, n)					\
 ({									\
 	BUILD_BUG_ON(sizeof(*(ptr)) != 8);				\
 	cmpxchg((ptr), (o), (n));					\
 })
 
-#define cmpxchg64_local(ptr, o, n)					\
+#define arch_cmpxchg64_local(ptr, o, n)					\
 ({									\
 	BUILD_BUG_ON(sizeof(*(ptr)) != 8);				\
 	cmpxchg_local((ptr), (o), (n));					\

include/asm-generic/atomic-instrumented.h new file +210 -0

210 lines not on this page · raw message

Signature · 7 lines

2.12.0.367.g23dc2f6d3c-goog

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrey Ryabinin

2017-03-20 16:40 UTC #3 lore raw

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

Quoted 13 lines · Dmitry Vyukov

arch/x86/include/asm/atomic.h +1 -1

index 14635c5ea025..95dd167eb3af 100644
@@ -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.

arch/x86/include/asm/atomic.h line 30

Quoted 10 lines

arch/x86/include/asm/atomic.h +6 -2

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

This is kinda questionable "optimization". READ_ONCE_NOCHECK() is actually function call to __read_once_size_nocheck().
So this _NOCHECK adds some bloat to the kernel.
E.g. on my .config remove of _NOCHECK() saves ~400K of .text:

size vmlinux_read
text data bss dec hex filename
40548235 16418838 23289856 80256929 4c89fa1 vmlinux_read
size vmlinux_read_nocheck
text data bss dec hex filename
40938418 16451958 23289856 80680232 4cf1528 vmlinux_read_nocheck

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mark Rutland

reviewer

2017-03-20 17:17 UTC #4 lore raw

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

In reply to Dmitry Vyukov

Hi,

Quoted 7 lines · Dmitry Vyukov

/**
- * atomic_read - read atomic variable
+ * arch_atomic_read - read atomic variable
* @v: pointer of type atomic_t
*
* Atomically reads the value of @v.
*/

arch/x86/include/asm/atomic.h line 31

Quoted 10 lines

arch/x86/include/asm/atomic.h +6 -2

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

Just to check, we do this to avoid duplicate reports, right?

If so, double instrumentation isn't solely "unnecessary"; it has a functional difference, and we should explicitly describe that in the comment.

... or are duplicate reports supressed somehow?

[...]

arch/x86/include/asm/atomic.h line 49

Quoted 10 lines

arch/x86/include/asm/atomic.h +7 -0

+static __always_inline void arch_atomic_set(atomic_t *v, int i)
 {
+	/*
+	 * We could use WRITE_ONCE_NOCHECK() if it exists, similar to
+	 * READ_ONCE_NOCHECK() in arch_atomic_read(). But there is no such
+	 * thing at the moment, and introducing it for this case does not
+	 * worth it.
+	 */
 	WRITE_ONCE(v->counter, i);
 }

If we are trying to avoid duplicate reports, we should do the same here.

[...]

include/asm-generic/atomic-instrumented.h line 61

Quoted 4 lines

include/asm-generic/atomic-instrumented.h new file +4 -0
+static __always_inline short int atomic_inc_short(short int *v)
+{
+	return arch_atomic_inc_short(v);
+}

This is x86-specific, and AFAICT, not used anywhere.

Given that it is arch-specific, I don't think it should be instrumented here. If it isn't used, we could get rid of it entirely...

Thanks,
Mark.

Andrey Ryabinin

2017-03-21 09:23 UTC #5 lore raw

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

Quoted 23 lines · Mark Rutland

Hi,
Quoted 7 lines · Dmitry Vyukov
/**
- * atomic_read - read atomic variable
+ * arch_atomic_read - read atomic variable
* @v: pointer of type atomic_t
*
* Atomically reads the value of @v.
*/
arch/x86/include/asm/atomic.h line 31
Quoted 10 lines
arch/x86/include/asm/atomic.h +6 -2
-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);
 }
Just to check, we do this to avoid duplicate reports, right?

Quoted 6 lines

If so, double instrumentation isn't solely "unnecessary"; it has a functional difference, and we should explicitly describe that in the comment.
... or are duplicate reports supressed somehow?

They are not suppressed yet. But I think we should just switch kasan to single shot mode, i.e. report only the first error. Single bug quite often has multiple invalid memory accesses causing storm in dmesg. Also write OOB might corrupt metadata so the next report will print bogus alloc/free stacktraces.
In most cases we need to look only at the first report, so reporting anything after the first is just counterproductive.

Mark Rutland

reviewer

2017-03-21 10:42 UTC #6 lore raw

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

Quoted 30 lines · Andrey Ryabinin

Quoted 23 lines · Mark Rutland
Hi,
Quoted 7 lines · Dmitry Vyukov
/**
- * atomic_read - read atomic variable
+ * arch_atomic_read - read atomic variable
* @v: pointer of type atomic_t
*
* Atomically reads the value of @v.
*/
arch/x86/include/asm/atomic.h line 31
Quoted 10 lines
arch/x86/include/asm/atomic.h +6 -2
-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);
 }
Just to check, we do this to avoid duplicate reports, right?
Quoted 5 lines
If so, double instrumentation isn't solely "unnecessary"; it has a functional difference, and we should explicitly describe that in the comment.
... or are duplicate reports supressed somehow?

Quoted 7 lines

They are not suppressed yet. But I think we should just switch kasan to single shot mode, i.e. report only the first error. Single bug quite often has multiple invalid memory accesses causing storm in dmesg. Also write OOB might corrupt metadata so the next report will print bogus alloc/free stacktraces.
In most cases we need to look only at the first report, so reporting anything after the first is just counterproductive.

FWIW, that sounds sane to me.

Given that, I agree with your comment regarding READ_ONCE{,_NOCHECK}().

If anyone really wants all the reports, we could have a boot-time option to do that.

Thanks,
Mark.

Dmitry Vyukov

author reviewer

2017-03-21 18:06 UTC #7 lore raw

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

Quoted 39 lines · Mark Rutland

Quoted 30 lines · Andrey Ryabinin
Quoted 23 lines · Mark Rutland
Hi,
Quoted 7 lines · Dmitry Vyukov
/**
- * atomic_read - read atomic variable
+ * arch_atomic_read - read atomic variable
* @v: pointer of type atomic_t
*
* Atomically reads the value of @v.
*/
arch/x86/include/asm/atomic.h line 31
Quoted 10 lines
arch/x86/include/asm/atomic.h +6 -2
-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);
 }
Just to check, we do this to avoid duplicate reports, right?
Quoted 5 lines
If so, double instrumentation isn't solely "unnecessary"; it has a functional difference, and we should explicitly describe that in the comment.
... or are duplicate reports supressed somehow?
Quoted 7 lines
They are not suppressed yet. But I think we should just switch kasan to single shot mode, i.e. report only the first error. Single bug quite often has multiple invalid memory accesses causing storm in dmesg. Also write OOB might corrupt metadata so the next report will print bogus alloc/free stacktraces.
In most cases we need to look only at the first report, so reporting anything after the first is just counterproductive.

Quoted 6 lines

FWIW, that sounds sane to me.
Given that, I agree with your comment regarding READ_ONCE{,_NOCHECK}().
If anyone really wants all the reports, we could have a boot-time option to do that.

I don't mind changing READ_ONCE_NOCHECK to READ_ONCE. But I don't have strong preference either way.

We could do:
#define arch_atomic_read_is_already_instrumented 1
and then skip instrumentation in asm-generic if it's defined. But I don't think it's worth it.

There is no functional difference, it's only an optimization (now somewhat questionable). As Andrey said, one can get a splash of reports anyway, and it's the first one that is important. We use KASAN with panic_on_warn=1 so we don't even see the rest.

Arnd Bergmann

maintainer

2017-03-21 21:20 UTC #8 lore raw

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

Quoted 11 lines · Dmitry Vyukov

Quoted 1 line · Mark Rutland
On Tue, Mar 21, 2017 at 12:25:06PM +0300, Andrey Ryabinin wrote:
I don't mind changing READ_ONCE_NOCHECK to READ_ONCE. But I don't have strong preference either way.
We could do:
#define arch_atomic_read_is_already_instrumented 1
and then skip instrumentation in asm-generic if it's defined. But I don't think it's worth it.

Quoted 4 lines

There is no functional difference, it's only an optimization (now somewhat questionable). As Andrey said, one can get a splash of reports anyway, and it's the first one that is important. We use KASAN with panic_on_warn=1 so we don't even see the rest.

I'm getting couple of new stack size warnings that are all the result of the _NOCHECK.

/git/arm-soc/mm/page_alloc.c: In function 'show_free_areas':
/git/arm-soc/mm/page_alloc.c:4685:1: error: the frame size of 3368 bytes is larger than 3072 bytes [-Werror=frame-larger-than=]
}
/git/arm-soc/lib/atomic64_test.c: In function 'test_atomic':
/git/arm-soc/lib/atomic64_test.c:148:1: error: the frame size of 6528 bytes is larger than 3072 bytes [-Werror=frame-larger-than=]
}
^
/git/arm-soc/lib/atomic64_test.c: In function 'test_atomic64':
/git/arm-soc/lib/atomic64_test.c:243:1: error: the frame size of 7112 bytes is larger than 3072 bytes [-Werror=frame-larger-than=]

This is with my previous set of patches already applied, so READ_ONCE should not cause problems. Reverting
the READ_ONCE_NOCHECK() in atomic_read() and atomic64_read() back to READ_ONCE()

I also get a build failure as a result of your patch, but this one is not addressed by using READ_ONCE():

In file included from /git/arm-soc/arch/x86/include/asm/atomic.h:7:0,

                 from /git/arm-soc/include/linux/atomic.h:4,
                 from /git/arm-soc/arch/x86/include/asm/thread_info.h:53,
                 from /git/arm-soc/include/linux/thread_info.h:25,
                 from /git/arm-soc/arch/x86/include/asm/preempt.h:6,
                 from /git/arm-soc/include/linux/preempt.h:80,
                 from /git/arm-soc/include/linux/spinlock.h:50,
                 from /git/arm-soc/include/linux/mmzone.h:7,
                 from /git/arm-soc/include/linux/gfp.h:5,
                 from /git/arm-soc/include/linux/mm.h:9,
                 from /git/arm-soc/mm/slub.c:12:

/git/arm-soc/mm/slub.c: In function '__slab_free':
/git/arm-soc/arch/x86/include/asm/cmpxchg.h:174:2: error: 'asm' operand has impossible constraints
asm volatile(pfx "cmpxchg%c4b %2; sete %0" \
^
/git/arm-soc/arch/x86/include/asm/cmpxchg.h:183:2: note: in expansion of macro '__cmpxchg_double'
__cmpxchg_double(LOCK_PREFIX, p1, p2, o1, o2, n1, n2) ^~~~~~~~~~~~~~~~
/git/arm-soc/include/asm-generic/atomic-instrumented.h:236:2: note: in expansion of macro 'arch_cmpxchg_double'
arch_cmpxchg_double(____p1, (p2), (o1), (o2), (n1), (n2)); \
^~~~~~~~~~~~~~~~~~~
/git/arm-soc/mm/slub.c:385:7: note: in expansion of macro 'cmpxchg_double'
if (cmpxchg_double(&page->freelist, &page->counters,

       ^~~~~~~~~~~~~~

/git/arm-soc/scripts/Makefile.build:308: recipe for target 'mm/slub.o' failed

http://pastebin.com/raw/qXVpi9Ev has the defconfig file I used, and I get the error with any gcc version I tried (4.9 through 7.0.1).

Arnd

Dmitry Vyukov

author reviewer

2017-03-22 10:43 UTC #9 lore raw

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

Quoted 67 lines · Arnd Bergmann

Quoted 11 lines · Dmitry Vyukov
Quoted 1 line · Mark Rutland
On Tue, Mar 21, 2017 at 12:25:06PM +0300, Andrey Ryabinin wrote:
I don't mind changing READ_ONCE_NOCHECK to READ_ONCE. But I don't have strong preference either way.
We could do:
#define arch_atomic_read_is_already_instrumented 1
and then skip instrumentation in asm-generic if it's defined. But I don't think it's worth it.
Quoted 4 lines
There is no functional difference, it's only an optimization (now somewhat questionable). As Andrey said, one can get a splash of reports anyway, and it's the first one that is important. We use KASAN with panic_on_warn=1 so we don't even see the rest.
I'm getting couple of new stack size warnings that are all the result of the _NOCHECK.
/git/arm-soc/mm/page_alloc.c: In function 'show_free_areas':
/git/arm-soc/mm/page_alloc.c:4685:1: error: the frame size of 3368 bytes is larger than 3072 bytes [-Werror=frame-larger-than=]
}
/git/arm-soc/lib/atomic64_test.c: In function 'test_atomic':
/git/arm-soc/lib/atomic64_test.c:148:1: error: the frame size of 6528 bytes is larger than 3072 bytes [-Werror=frame-larger-than=]
}
^
/git/arm-soc/lib/atomic64_test.c: In function 'test_atomic64':
/git/arm-soc/lib/atomic64_test.c:243:1: error: the frame size of 7112 bytes is larger than 3072 bytes [-Werror=frame-larger-than=]
This is with my previous set of patches already applied, so READ_ONCE should not cause problems. Reverting
the READ_ONCE_NOCHECK() in atomic_read() and atomic64_read() back to READ_ONCE()
I also get a build failure as a result of your patch, but this one is not addressed by using READ_ONCE():
In file included from /git/arm-soc/arch/x86/include/asm/atomic.h:7:0,
                 from /git/arm-soc/include/linux/atomic.h:4,
                 from /git/arm-soc/arch/x86/include/asm/thread_info.h:53,
                 from /git/arm-soc/include/linux/thread_info.h:25,
                 from /git/arm-soc/arch/x86/include/asm/preempt.h:6,
                 from /git/arm-soc/include/linux/preempt.h:80,
                 from /git/arm-soc/include/linux/spinlock.h:50,
                 from /git/arm-soc/include/linux/mmzone.h:7,
                 from /git/arm-soc/include/linux/gfp.h:5,
                 from /git/arm-soc/include/linux/mm.h:9,
                 from /git/arm-soc/mm/slub.c:12:
/git/arm-soc/mm/slub.c: In function '__slab_free':
/git/arm-soc/arch/x86/include/asm/cmpxchg.h:174:2: error: 'asm' operand has impossible constraints
asm volatile(pfx "cmpxchg%c4b %2; sete %0" \
^
/git/arm-soc/arch/x86/include/asm/cmpxchg.h:183:2: note: in expansion of macro '__cmpxchg_double'
__cmpxchg_double(LOCK_PREFIX, p1, p2, o1, o2, n1, n2) ^~~~~~~~~~~~~~~~
/git/arm-soc/include/asm-generic/atomic-instrumented.h:236:2: note: in expansion of macro 'arch_cmpxchg_double'
arch_cmpxchg_double(____p1, (p2), (o1), (o2), (n1), (n2)); \
^~~~~~~~~~~~~~~~~~~
/git/arm-soc/mm/slub.c:385:7: note: in expansion of macro 'cmpxchg_double'
if (cmpxchg_double(&page->freelist, &page->counters,

Quoted 5 lines

       ^~~~~~~~~~~~~~
/git/arm-soc/scripts/Makefile.build:308: recipe for target 'mm/slub.o' failed
http://pastebin.com/raw/qXVpi9Ev has the defconfig file I used, and I get the error with any gcc version I tried (4.9 through 7.0.1).

Initially I've tested with my stock gcc 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.3) and amusingly it works. But I can reproduce the bug with 7.0.1.
Filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80148
Will think about kernel fix.
Thanks!

Arnd Bergmann

maintainer

2017-03-22 11:30 UTC #10 lore raw

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

Quoted 6 lines · Dmitry Vyukov

Quoted 1 line · Arnd Bergmann
On Tue, Mar 21, 2017 at 7:06 PM, Dmitry Vyukov <[email protected]> wrote:
Initially I've tested with my stock gcc 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.3) and amusingly it works. But I can reproduce the bug with 7.0.1.

It's probably because gcc-4.8 didn't support KASAN yet, so the added check had no effect.

Arnd

Dmitry Vyukov

author reviewer

2017-03-22 12:15 UTC #11 lore raw

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

Quoted 10 lines · Arnd Bergmann

Quoted 6 lines · Dmitry Vyukov
Quoted 1 line · Arnd Bergmann
On Tue, Mar 21, 2017 at 7:06 PM, Dmitry Vyukov <[email protected]> wrote:
Initially I've tested with my stock gcc 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.3) and amusingly it works. But I can reproduce the bug with 7.0.1.
It's probably because gcc-4.8 didn't support KASAN yet, so the added check had no effect.

I've tested without KASAN with both compilers.

Arnd Bergmann

maintainer

2017-03-22 12:48 UTC #12 lore raw

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

Quoted 5 lines · Dmitry Vyukov

Quoted 4 lines · Arnd Bergmann
Quoted 3 lines · Dmitry Vyukov
Quoted 1 line · Arnd Bergmann
On Tue, Mar 21, 2017 at 7:06 PM, Dmitry Vyukov <[email protected]> wrote:

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Initially I've tested with my stock gcc 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.3) and amusingly it works. But I can reproduce the bug with 7.0.1.
It's probably because gcc-4.8 didn't support KASAN yet, so the added check had no effect.
I've tested without KASAN with both compilers.

Ah ok. I had not realized that this happened even without KASAN. I only saw this problem in one out of hundreds of defconfig builds and assumed it was related since this came from a kasan change.

Arnd

Ingo Molnar

2017-03-24 06:52 UTC #13 lore raw

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

In reply to Dmitry Vyukov

Quoted 27 lines · Dmitry Vyukov

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 <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Will Deacon <[email protected]>,
Cc Andrew Morton <[email protected]>,
Cc Andrey Ryabinin <[email protected]>,
Cc Ingo Molnar <[email protected]>,
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]

Quoted 9 lines

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

Quoted 15 lines

arch/x86/include/asm/atomic.h +1 -1

index 14635c5ea025..95dd167eb3af 100644
@@ -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.
  */

arch/x86/include/asm/atomic.h line 31

Quoted 10 lines

arch/x86/include/asm/atomic.h +6 -2

-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:

arch/x86/include/asm/atomic.h line 40

Quoted 11 lines

arch/x86/include/asm/atomic.h +2 -2

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

Quoted 79 lines

arch/x86/include/asm/atomic.h +79 -0

+#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);
+

arch/x86/include/asm/atomic.h

Quoted 10 lines

arch/x86/include/asm/atomic.h +10 -0

+#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

Dmitry Vyukov

author reviewer

2017-03-24 07:15 UTC #14 lore raw

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

Quoted 59 lines · Ingo Molnar

Quoted 27 lines · Dmitry Vyukov
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 <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Will Deacon <[email protected]>,
Cc Andrew Morton <[email protected]>,
Cc Andrey Ryabinin <[email protected]>,
Cc Ingo Molnar <[email protected]>,
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Quoted 9 lines
---
 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...
Quoted 17 lines
arch/x86/include/asm/atomic.h +2 -2
index 14635c5ea025..95dd167eb3af 100644
@@ -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)

Quoted 8 lines

arch/x86/include/asm/atomic.h line 31

Quoted 8 lines

arch/x86/include/asm/atomic.h +5 -1

 {
-     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,

Quoted 5 lines

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?

Quoted 107 lines

arch/x86/include/asm/atomic.h line 40

Quoted 10 lines

arch/x86/include/asm/atomic.h +2 -2

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

Quoted 79 lines

arch/x86/include/asm/atomic.h +79 -0

+#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);
+

arch/x86/include/asm/atomic.h

Quoted 10 lines

arch/x86/include/asm/atomic.h +10 -0

+#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.

Quoted 2 lines

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

Dmitry Vyukov

author reviewer

2017-03-24 08:39 UTC #15 lore raw

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

Quoted 189 lines · Dmitry Vyukov

Quoted 59 lines · Ingo Molnar
Quoted 27 lines · Dmitry Vyukov
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 <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Will Deacon <[email protected]>,
Cc Andrew Morton <[email protected]>,
Cc Andrey Ryabinin <[email protected]>,
Cc Ingo Molnar <[email protected]>,
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Quoted 9 lines
---
 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...
Quoted 17 lines
arch/x86/include/asm/atomic.h +2 -2
index 14635c5ea025..95dd167eb3af 100644
@@ -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)
Quoted 8 lines
arch/x86/include/asm/atomic.h line 31
Quoted 8 lines
arch/x86/include/asm/atomic.h +5 -1
 {
-     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,
Quoted 5 lines
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?
Quoted 107 lines
arch/x86/include/asm/atomic.h line 40
Quoted 10 lines
arch/x86/include/asm/atomic.h +2 -2
 /**
- * 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:
Quoted 79 lines
arch/x86/include/asm/atomic.h +79 -0
+#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);
+
arch/x86/include/asm/atomic.h
Quoted 10 lines
arch/x86/include/asm/atomic.h +10 -0
+#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.

Quoted 7 lines

Quoted 2 lines
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...

Stephen Rothwell reported that this patch conflicts with:
a9ebf306f52c ("locking/atomic: Introduce atomic_try_cmpxchg()") e6790e4b5d5e ("locking/atomic/x86: Use atomic_try_cmpxchg()")
does it make sense to base my patch on the tree where these patches were added and then submit to that tree?

I've also sent 2 fixes for this patch, if I resent this I also squash these fixes, right?

Ingo Molnar

2017-03-24 10:57 UTC #16 lore raw

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

In reply to Dmitry Vyukov

Quoted 8 lines · Dmitry Vyukov

Quoted 5 lines
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 think writing the prototypes out as-is, properly organized, beats any of these macro based solutions.

Quoted 3 lines

[...] 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...

Mind fixing those too while at it?

And please squash any bug fixes and re-send a clean series against latest upstream or so.

Thanks,

Ingo

Dmitry Vyukov

author reviewer

2017-03-24 12:46 UTC #17 lore raw

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

Quoted 6 lines · Ingo Molnar

Quoted 3 lines · Dmitry Vyukov
Quoted 3 lines
Are just utterly disgusting that turn perfectly readable code into an unreadable, unmaintainable mess.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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 think writing the prototypes out as-is, properly organized, beats any of these macro based solutions.

You mean write out the prototypes, but use what for definitions? Macros again?

Quoted 5 lines

Quoted 3 lines
[...] 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...
Mind fixing those too while at it?

I don't mind once I understand how exactly you want it to look.

Quoted 6 lines

And please squash any bug fixes and re-send a clean series against latest upstream or so.
Thanks,
Ingo

Ingo Molnar

2017-03-28 07:52 UTC #18 lore raw

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

Quoted 10 lines · Dmitry Vyukov

Quoted 9 lines · Ingo Molnar
Quoted 6 lines · Dmitry Vyukov
Quoted 5 lines
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.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Well, I can just write all functions as is. Does it better confirm to kernel style?
I think writing the prototypes out as-is, properly organized, beats any of these macro based solutions.
You mean write out the prototypes, but use what for definitions? Macros again?

No, regular C code.

I don't see the point of generating all this code via CPP - it's certainly not making it more readable to me. I.e. this patch I commented on is a step backwards for readability.

I'd prefer repetition and a higher overall line count over complex CPP constructs.

Thanks,

Ingo

Peter Zijlstra

maintainer

2017-03-28 09:27 UTC #19 lore raw

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

Quoted 5 lines · Ingo Molnar

No, regular C code.
I don't see the point of generating all this code via CPP - it's certainly not making it more readable to me. I.e. this patch I commented on is a step backwards for readability.

Note that much of the atomic stuff we have today is all CPP already.

x86 is the exception because its 'weird', but most other archs are almost pure CPP -- check Alpha for example, or asm-generic/atomic.h.

Also, look at linux/atomic.h, its a giant maze of CPP.

The CPP help us generate functions, reduces endless copy/paste (which induces random differences -- read bugs) and construct variants depending on the architecture input.

Yes, the CPP is a pain, but writing all that out explicitly is more of a pain.

I've not yet looked too hard at these patches under consideration; and I really wish we could get the compiler to do the right thing here, but reducing the endless copy/paste that's otherwise the result of this, is something I've found to be very valuable.

Not to mention that adding additional atomic ops got trivial (the set is now near complete, so that's not much of an argument anymore -- but it was, its what kept me sane sanitizing the atomic ops across all our 25+ architectures).

Dmitry Vyukov

author reviewer

2017-03-28 09:46 UTC #20 lore raw

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

Quoted 29 lines · Peter Zijlstra

Quoted 5 lines · Ingo Molnar
No, regular C code.
I don't see the point of generating all this code via CPP - it's certainly not making it more readable to me. I.e. this patch I commented on is a step backwards for readability.
Note that much of the atomic stuff we have today is all CPP already.
x86 is the exception because its 'weird', but most other archs are almost pure CPP -- check Alpha for example, or asm-generic/atomic.h.
Also, look at linux/atomic.h, its a giant maze of CPP.
The CPP help us generate functions, reduces endless copy/paste (which induces random differences -- read bugs) and construct variants depending on the architecture input.
Yes, the CPP is a pain, but writing all that out explicitly is more of a pain.
I've not yet looked too hard at these patches under consideration; and I really wish we could get the compiler to do the right thing here, but reducing the endless copy/paste that's otherwise the result of this, is something I've found to be very valuable.

Quoted 4 lines

Not to mention that adding additional atomic ops got trivial (the set is now near complete, so that's not much of an argument anymore -- but it was, its what kept me sane sanitizing the atomic ops across all our 25+ architectures).

I am almost done with Ingo's proposal, including de-macro-ifying x86 atomic ops code.
I am ready to do either of them, I think both have pros and cons and there is no perfect solution. But please agree on something.

While we are here, one thing that I noticed is that 32-bit atomic code uses 'long long' for 64-bit operands, while 64-bit code uses 'long' for 64-bit operands. This sorta worked more of less before, but ultimately it makes it impossible to write any portable code (e.g. you don't know what format specifier to use to print return value of atomic64_read, nor what local variable type to use to avoid compiler warnings). With the try_cmpxchg it become worse, because 'long*' is not convertible to 'long long*' so it is not possible to write any portable code that uses it. If you declare 'old' variable as 'long' 32-bit code won't compile, if you declare it as 'long long' 64-bit code won't compiler. I think we need to switch to a single type for 64-bit operands/return values, e.g. 'long long'.

Ingo Molnar

2017-03-28 09:51 UTC #21 lore raw

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

In reply to Peter Zijlstra

Quoted 9 lines · Peter Zijlstra

Quoted 5 lines · Ingo Molnar
No, regular C code.
I don't see the point of generating all this code via CPP - it's certainly not making it more readable to me. I.e. this patch I commented on is a step backwards for readability.
Note that much of the atomic stuff we have today is all CPP already.

Yeah, but there it's implementational: we pick up arch primitives depending on whether they are defined, such as:

#ifndef atomic_read_acquire

# define atomic_read_acquire(v)		smp_load_acquire(&(v)->counter)
#endif

Quoted 2 lines

x86 is the exception because its 'weird', but most other archs are almost pure CPP -- check Alpha for example, or asm-generic/atomic.h.

include/asm-generic/atomic.h looks pretty clean and readable overall.

Quoted 1 line

Also, look at linux/atomic.h, its a giant maze of CPP.

Nah, that's OK, much of is is essentially __weak inlines implemented via CPP - i.e. CPP is filling in a missing compiler feature.

But this patch I replied to appears to add instrumentation wrappery via CPP which looks like excessive and avoidable obfuscation to me.

If it's much more readable and much more compact than the C version then maybe, but I'd like to see the C version first and see ...

Quoted 6 lines

The CPP help us generate functions, reduces endless copy/paste (which induces random differences -- read bugs) and construct variants depending on the architecture input.
Yes, the CPP is a pain, but writing all that out explicitly is more of a pain.

So I'm not convinced that it's true in this case.

Could we see the C version and compare? I could be wrong about it all.

Thanks,

Ingo

Dmitry Vyukov

author reviewer

2017-03-28 09:56 UTC #22 lore raw

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

Quoted 40 lines · Ingo Molnar

Quoted 9 lines · Peter Zijlstra
Quoted 5 lines · Ingo Molnar
No, regular C code.
I don't see the point of generating all this code via CPP - it's certainly not making it more readable to me. I.e. this patch I commented on is a step backwards for readability.
Note that much of the atomic stuff we have today is all CPP already.
Yeah, but there it's implementational: we pick up arch primitives depending on whether they are defined, such as:
#ifndef atomic_read_acquire
# define atomic_read_acquire(v)         smp_load_acquire(&(v)->counter)
#endif
Quoted 2 lines
x86 is the exception because its 'weird', but most other archs are almost pure CPP -- check Alpha for example, or asm-generic/atomic.h.
include/asm-generic/atomic.h looks pretty clean and readable overall.
Quoted 1 line
Also, look at linux/atomic.h, its a giant maze of CPP.
Nah, that's OK, much of is is essentially __weak inlines implemented via CPP - i.e. CPP is filling in a missing compiler feature.
But this patch I replied to appears to add instrumentation wrappery via CPP which looks like excessive and avoidable obfuscation to me.
If it's much more readable and much more compact than the C version then maybe, but I'd like to see the C version first and see ...
Quoted 4 lines
The CPP help us generate functions, reduces endless copy/paste (which induces random differences -- read bugs) and construct variants depending on the architecture input.

Quoted 6 lines

Quoted 2 lines
Yes, the CPP is a pain, but writing all that out explicitly is more of a pain.
So I'm not convinced that it's true in this case.
Could we see the C version and compare? I could be wrong about it all.

Here it is (without instrumentation):
https://gist.github.com/dvyukov/e33d580f701019e0cd99429054ff1f9a

Instrumentation will add for each function:

static __always_inline void atomic64_set(atomic64_t *v, long long i) {

+       kasan_check_write(v, sizeof(*v));
        arch_atomic64_set(v, i);

}

Ingo Molnar

2017-03-28 10:15 UTC #23 lore raw

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

Quoted 6 lines · Dmitry Vyukov

Quoted 3 lines
So I'm not convinced that it's true in this case.
Could we see the C version and compare? I could be wrong about it all.
Here it is (without instrumentation):
https://gist.github.com/dvyukov/e33d580f701019e0cd99429054ff1f9a

Could you please include the full patch so that it can be discussed via email and such?

Quoted 7 lines

Instrumentation will add for each function:
static __always_inline void atomic64_set(atomic64_t *v, long long i) {
+       kasan_check_write(v, sizeof(*v));
        arch_atomic64_set(v, i);
}

That in itself looks sensible and readable.

Thanks,

Ingo

Dmitry Vyukov

author reviewer

2017-03-28 16:30 UTC #24 lore raw

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

Quoted 5 lines · Ingo Molnar

Quoted 2 lines · Dmitry Vyukov
Quoted 2 lines
So I'm not convinced that it's true in this case.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Could we see the C version and compare? I could be wrong about it all.
Here it is (without instrumentation):
https://gist.github.com/dvyukov/e33d580f701019e0cd99429054ff1f9a
Could you please include the full patch so that it can be discussed via email and such?

Mailed the whole series.

Quoted 8 lines

Quoted 7 lines
Instrumentation will add for each function:
static __always_inline void atomic64_set(atomic64_t *v, long long i) {
+       kasan_check_write(v, sizeof(*v));
        arch_atomic64_set(v, i);
}

Quoted 5 lines

That in itself looks sensible and readable.
Thanks,
Ingo

Dmitry Vyukov

author reviewer

2017-03-14 19:24 UTC #25 lore raw

PATCH 3/3 asm-generic: add KASAN instrumentation to atomic operations

In reply to Dmitry Vyukov

Add manual KASAN checks to atomic operations.

Signed-off-by Dmitry Vyukov <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Will Deacon <[email protected]>,

Cc Andrew Morton <[email protected]>,

Cc Andrey Ryabinin <[email protected]>,

Cc Ingo Molnar <[email protected]>,

Cc [email protected]

---
 include/asm-generic/atomic-instrumented.h | 36 +++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

include/asm-generic/atomic-instrumented.h +36 -0

index 41e5e6ffdfc8..951bcd083925 100644
@@ -1,50 +1,72 @@
+/*
+ * This file provides wrappers with KASAN instrumentation for atomic operations.
+ * To use this functionality an arch's atomic.h file needs to define all
+ * atomic operations with arch_ prefix (e.g. arch_atomic_read()) and include
+ * this file at the end. This file provides atomic_read() that forwards to
+ * arch_atomic_read() for actual atomic operation.
+ * Note: if an arch atomic operation is implemented by means of other atomic
+ * operations (e.g. atomic_read()/atomic_cmpxchg() loop), then it needs to use
+ * arch_ variants (i.e. arch_atomic_read()/arch_atomic_cmpxchg()) to avoid
+ * double instrumentation.
+ */
 #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 long long 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, long long 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 long long atomic64_xchg(atomic64_t *v, long long 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 long long atomic64_cmpxchg(atomic64_t *v, long long old,
 						  long long new)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_cmpxchg(v, old, new);
 }
 
 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);
 }

@@ -52,17 +74,20 @@ static __always_inline int __atomic_add_unless(atomic_t *v, int a, int u)
 static __always_inline bool atomic64_add_unless(atomic64_t *v, long long a,
 						long long u)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic64_add_unless(v, a, u);
 }
 
 static __always_inline short int atomic_inc_short(short int *v)
 {
+	kasan_check_write(v, sizeof(*v));
 	return arch_atomic_inc_short(v);
 }
 
 #define __INSTR_VOID1(op, sz)						\
 static __always_inline void atomic##sz##_##op(atomic##sz##_t *v)	\
 {									\
+	kasan_check_write(v, sizeof(*v));				\
 	arch_atomic##sz##_##op(v);					\
 }

@@ -79,6 +104,7 @@ INSTR_VOID1(dec);
 #define __INSTR_VOID2(op, sz, type)					\
 static __always_inline void atomic##sz##_##op(type i, atomic##sz##_t *v)\
 {									\
+	kasan_check_write(v, sizeof(*v));				\
 	arch_atomic##sz##_##op(i, v);					\
 }

@@ -98,6 +124,7 @@ INSTR_VOID2(xor);
 #define __INSTR_RET1(op, sz, type, rtype)				\
 static __always_inline rtype atomic##sz##_##op(atomic##sz##_t *v)	\
 {									\
+	kasan_check_write(v, sizeof(*v));				\
 	return arch_atomic##sz##_##op(v);				\
 }

@@ -124,6 +151,7 @@ INSTR_RET_BOOL1(inc_and_test);
 #define __INSTR_RET2(op, sz, type, rtype)				\
 static __always_inline rtype atomic##sz##_##op(type i, atomic##sz##_t *v) \
 {									\
+	kasan_check_write(v, sizeof(*v));				\
 	return arch_atomic##sz##_##op(i, v);				\
 }

@@ -162,48 +190,56 @@ INSTR_RET_BOOL2(add_negative);
 #define xchg(ptr, v)					\
 ({							\
 	__typeof__(ptr) ____ptr = (ptr);		\
+	kasan_check_write(____ptr, sizeof(*____ptr));	\
 	arch_xchg(____ptr, (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)			\
 ({							\
 	__typeof__(ptr) ___ptr = (ptr);			\
+	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));	\
 })
 
 #define 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)			\
 ({							\
 	__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)				\
 ({									\
 	__typeof__(p1) ____p1 = (p1);					\
+	kasan_check_write(____p1, 2 * sizeof(*____p1));			\
 	arch_cmpxchg_double(____p1, (p2), (o1), (o2), (n1), (n2));	\
 })
 
 #define cmpxchg_double_local(p1, p2, o1, o2, n1, n2)			\
 ({									\
 	__typeof__(p1) ____p1 = (p1);					\
+	kasan_check_write(____p1, 2 * sizeof(*____p1));			\
 	arch_cmpxchg_double_local(____p1, (p2), (o1), (o2), (n1), (n2));\
 })

Signature · 7 lines

2.12.0.367.g23dc2f6d3c-goog

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2017-03-30 22:30 UTC #26 lore raw

In reply to Dmitry Vyukov

Quoted 5 lines · On Tue, 14 Mar 2017 20:24:11 +0100 Dmitry Vyukov <[email protected]> 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.

I'm getting a pile of build errors from this patchset (and related patches). Due to messed up merge fixing, probably. Please the review process has been a bit bumpy.

So I'll drop

kasan-allow-kasan_check_read-write-to-accept-pointers-to-volatiles.patch asm-generic-x86-wrap-atomic-operations.patch asm-generic-x86-wrap-atomic-operations-fix.patch asm-generic-add-kasan-instrumentation-to-atomic-operations.patch asm-generic-fix-compilation-failure-in-cmpxchg_double.patch x86-remove-unused-atomic_inc_short.patch x86-asm-generic-add-kasan-instrumentation-to-bitops.patch

for now. Please resend (against -mm or linux-next) when the dust has settled.