On 11/01/2018 07:32 PM, Peter Zijlstra wrote: > On Thu, Nov 01, 2018 at 03:22:15PM +0000, Trond Myklebust wrote: >> On Thu, 2018-11-01 at 15:59 +0100, Peter Zijlstra wrote: >>> On Thu, Nov 01, 2018 at 01:18:46PM +0000, Mark Rutland wrote: > >>>>> My one question (and the reason why I went with cmpxchg() in the >>>>> first place) would be about the overflow behaviour for >>>>> atomic_fetch_inc() and friends. I believe those functions should >>>>> be OK on x86, so that when we overflow the counter, it behaves >>>>> like an unsigned value and wraps back around. Is that the case >>>>> for all architectures? >>>>> >>>>> i.e. are atomic_t/atomic64_t always guaranteed to behave like >>>>> u32/u64 on increment? >>>>> >>>>> I could not find any documentation that explicitly stated that >>>>> they should. >>>> >>>> Peter, Will, I understand that the atomic_t/atomic64_t ops are >>>> required to wrap per 2's-complement. IIUC the refcount code relies >>>> on this. >>>> >>>> Can you confirm? >>> >>> There is quite a bit of core code that hard assumes 2s-complement. >>> Not only for atomics but for any signed integer type. Also see the >>> kernel using -fno-strict-overflow which implies -fwrapv, which >>> defines signed overflow to behave like 2s-complement (and rids us of >>> that particular UB). >> >> Fair enough, but there have also been bugfixes to explicitly fix unsafe >> C standards assumptions for signed integers. See, for instance commit >> 5a581b367b5d "jiffies: Avoid undefined behavior from signed overflow" >> from Paul McKenney. > > Yes, I feel Paul has been to too many C/C++ committee meetings and got > properly paranoid. Which isn't always a bad thing :-) > > But for us using -fno-strict-overflow which actually defines signed > overflow, I myself am really not worried. I'm also not sure if KASAN has > been taught about this, or if it will still (incorrectly) warn about UB > for signed types. > UBSAN warns about signed overflows despite -fno-strict-overflow if gcc version is < 8. I have learned recently that UBSAN in GCC 8 ignores signed overflows if -fno-strict-overflow of fwrapv is used. $ cat signed_overflow.c #include <stdio.h> __attribute__((noinline)) int foo(int a, int b) { return a+b; } int main(void) { int a = 0x7fffffff; int b = 2; printf("%d\n", foo(a,b)); return 0; } $ gcc-8.2.0 -fsanitize=signed-integer-overflow signed_overflow.c && ./a.out signed_overflow.c:6:10: runtime error: signed integer overflow: 2147483647 + 2 cannot be represented in type 'int' -2147483647 $ gcc-8.2.0 -fno-strict-overflow -fsanitize=signed-integer-overflow signed_overflow.c && ./a.out -2147483647 $ gcc-7.3.0 -fsanitize=signed-integer-overflow signed_overflow.c && ./a.out signed_overflow.c:6:10: runtime error: signed integer overflow: 2147483647 + 2 cannot be represented in type 'int' -2147483647 $ gcc-7.3.0 -fno-strict-overflow -fsanitize=signed-integer-overflow signed_overflow.c && ./a.out signed_overflow.c:6:10: runtime error: signed integer overflow: 2147483647 + 2 cannot be represented in type 'int' -2147483647 clang behaves the same way as GCC 8: $ clang -fsanitize=signed-integer-overflow signed_overflow.c && ./a.out signed_overflow.c:6:10: runtime error: signed integer overflow: 2147483647 + 2 cannot be represented in type 'int' -2147483647 $ clang -fno-strict-overflow -fsanitize=signed-integer-overflow signed_overflow.c && ./a.out -2147483647 We can always just drop -fsanitize=signed-integer-overflow if it considered too noisy. Although it did catch some real bugs.