x86, kasan: add KASAN checks to atomic operations

Add arch_ prefix to all atomic operations and include <asm-generic/atomic-instrumented.h>. This will allow to add KASAN instrumentation to all atomic ops.

Quoted 3 lines · On Thu, 22 Jun 2017 16:14:16 +0200 Dmitry Vyukov <[email protected]> wrote:

Add arch_ prefix to all atomic operations and include <asm-generic/atomic-instrumented.h>. This will allow to add KASAN instrumentation to all atomic ops.

This gets a large number of (simple) rejects when applied to linux-next. Can you please redo against -next?

Quoted 6 lines · Andrew Morton

Quoted 3 lines · On Thu, 22 Jun 2017 16:14:16 +0200 Dmitry Vyukov <[email protected]> wrote:

Add arch_ prefix to all atomic operations and include <asm-generic/atomic-instrumented.h>. This will allow to add KASAN instrumentation to all atomic ops.

Quoted 2 lines

This gets a large number of (simple) rejects when applied to linux-next. Can you please redo against -next?

This is based on tip/locking tree. Ingo already took a part of these series. The plan is that he takes the rest, and this applies on tip/locking without conflicts.

Quoted 8 lines · Dmitry Vyukov

Quoted 6 lines · Andrew Morton

Quoted 3 lines · On Thu, 22 Jun 2017 16:14:16 +0200 Dmitry Vyukov <[email protected]> wrote:

Add arch_ prefix to all atomic operations and include <asm-generic/atomic-instrumented.h>. This will allow to add KASAN instrumentation to all atomic ops.

Quoted 7 lines

Quoted 2 lines

This gets a large number of (simple) rejects when applied to linux-next. Can you please redo against -next?

This is based on tip/locking tree. Ingo already took a part of these series. The plan is that he takes the rest, and this applies on tip/locking without conflicts.

Yeah, so I've taken the rest as well, it all looks very clean now. Should show up in the next -next, if it passes my (arguably limited) testing.

Andrew, is this workflow fine with you? You usually take KASAN patches, but I was unhappy with the atomics instrumention of the earlier patches, and ended up reviewing the followup variants, and felt that if I hinder a patchset I might as well test and apply it once I'm happy with them! ;-)

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

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

include/asm-generic/atomic-instrumented.h +3 -3

index a0f5b7525bb2..ac6155362b39 100644
@@ -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)                 \

Quoted 13 lines · Sebastian Andrzej Siewior

(quoted diff) +3 -3

@@ -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)                 \

Review static __always_inline bool atomic64_add_negative(s64 i, atomic64_t *v) · line 374

Quoted 10 lines

(quoted diff) +3 -3

 ({                                                     \
-       __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?..

Quoted 41 lines · Dmitry Vyukov

Quoted 13 lines · Sebastian Andrzej Siewior

include/asm-generic/atomic-instrumented.h +3 -3

@@ -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)                 \

include/asm-generic/atomic-instrumented.h static __always_inline bool atomic64_add_negative(s64 i, atomic64_t *v) · line 374

Quoted 10 lines

include/asm-generic/atomic-instrumented.h +3 -3

 ({                                                     \
-       __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:

Quoted 8 lines

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

include/asm-generic/atomic-instrumented.h +2 -5

@@ -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)));       \

include/asm-generic/atomic-instrumented.h +2 -3

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

Quoted 37 lines · Thomas Gleixner

Quoted 10 lines

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

include/asm-generic/atomic-instrumented.h +2 -5

@@ -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)));       \

include/asm-generic/atomic-instrumented.h static __always_inline bool atomic64_add · line 375

Quoted 10 lines

include/asm-generic/atomic-instrumented.h +3 -3

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

Quoted 52 lines · Dmitry Vyukov

Quoted 37 lines · Thomas Gleixner

Quoted 10 lines

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

include/asm-generic/atomic-instrumented.h +2 -5

@@ -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)));       \

include/asm-generic/atomic-instrumented.h static __always_inline bool atomic64_add · line 375

Quoted 10 lines

include/asm-generic/atomic-instrumented.h +3 -3

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

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

Quoted 2 lines

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

include/asm-generic/atomic-instrumented.h static __always_inline bool atomic64_add · line 375

Quoted 7 lines · Thomas Gleixner

#define cmpxchg_local(ptr, old, new) \
({ \

include/asm-generic/atomic-instrumented.h +2 -3

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

Quoted 10 lines · Dmitry Vyukov

Quoted 7 lines · Thomas Gleixner

#define cmpxchg_local(ptr, old, new) \
({ \

include/asm-generic/atomic-instrumented.h +2 -3

-       __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));        \

Quoted 4 lines

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

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.

Quoted 11 lines · Thomas Gleixner

Quoted 10 lines · Dmitry Vyukov

Quoted 7 lines · Thomas Gleixner

#define cmpxchg_local(ptr, old, new) \
({ \

include/asm-generic/atomic-instrumented.h +2 -3

-       __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));        \

Quoted 7 lines

Quoted 4 lines

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

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.

Quoted 68 lines · Sebastian Andrzej Siewior

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

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

include/asm-generic/atomic-instrumented.h +3 -3

index a0f5b7525bb2..ac6155362b39 100644
@@ -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)                 \

include/asm-generic/atomic-instrumented.h static __always_inline bool atomic64_add_negative(s64 i, atomic64_t *v) · line 374

Quoted 10 lines

include/asm-generic/atomic-instrumented.h +3 -3

 ({                                                     \
-       __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?..

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.

---
 arch/x86/include/asm/cmpxchg_64.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

index fafaebacca2d..7046a3cc2493 100644
@@ -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));					\

Quoted 4 lines · Andrey Ryabinin

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?

Quoted 7 lines · Sebastian Andrzej Siewior

Quoted 4 lines · Andrey Ryabinin

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.

Quoted 4 lines · Thomas Gleixner

Quoted 3 lines · Sebastian Andrzej Siewior

Quoted 2 lines · Andrey Ryabinin

The main problem here is that arch_cmpxchg64_local() calls cmpxhg_local() instead of using arch_cmpxchg_local().

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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

Quoted 20 lines

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:

Quoted 8 lines

		if (sizeof(unsigned long) == 8)
			break;
	default:
		BUILD_BUG_ON(1);
	}
	kasan_check(ptr, size);
	return arch_cmpxchg(ptr, old, new);
}

Quoted 8 lines

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

Quoted 4 lines

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.

Quoted 9 lines · Thomas Gleixner

Quoted 9 lines

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:

Quoted 8 lines

Quoted 8 lines

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

Quoted 19 lines · Mark Rutland

Quoted 9 lines · Thomas Gleixner

Quoted 9 lines

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:

Quoted 8 lines

Quoted 8 lines

		if (sizeof(unsigned long) == 8)
			break;
	default:
		BUILD_BUG_ON(1);
	}
	kasan_check(ptr, size);
	return arch_cmpxchg(ptr, old, new);
}

Quoted 2 lines

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

Quoted 22 lines

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

Quoted 1 line

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

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>

Quoted 22 lines · Mark Rutland

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

Quoted 5 lines

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

Quoted 3 lines · Sebastian Andrzej Siewior

Quoted 2 lines · Andrey Ryabinin

The main problem here is that arch_cmpxchg64_local() calls cmpxhg_local() instead of using arch_cmpxchg_local().

Quoted 4 lines

Quoted 2 lines

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