On Thu, Nov 1, 2018 at 6:18 PM, Peter Zijlstra [off-list ref] wrote:

quoted

quoted

quoted

quoted

quoted

quoted

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

Even the C standard defines 2s complement for atomics.

Ooh good to know.

quoted

Just not for
normal arithmetic, where yes, signed overflow is UB.  And yes, I do
know about -fwrapv, but I would like to avoid at least some copy-pasta
UB from my kernel code to who knows what user-mode environment.  :-/

At least where it is reasonably easy to do so.

Fair enough I suppose; I just always make sure to include the same
-fknobs for the userspace thing when I lift code.

quoted

And there is a push to define C++ signed arithmetic as 2s complement,
but there are still 1s complement systems with C compilers.  Just not
C++ compilers.  Legacy...

*groan*; how about those ancient hardwares keep using ancient compilers
and we all move on to the 70s :-)

quoted

quoted

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 gave me a signed-overflow warning a few days ago.  Which I have
fixed, even though 2s complement did the right thing.  I am also taking
advantage of the change to use better naming.

Oh too many *SANs I suppose; and yes, if you can make the code better,
why not.

If there is a warning that we don't want to see at all, then we can
disable it. It supposed to be a useful tool, rather than a thing in
itself that lives own life. We already I think removed 1 particularly
noisy warning and made another optional via a config.
But the thing with overflows is that, even if it's defined, it's not
necessary the intended behavior. For example, take allocation size
calculation done via unsigned size_t. If it overflows it does not help
if C defines result or not, it still gives a user controlled write
primitive. We've seen similar cases with timeout/deadline calculation
in kernel, we really don't want it to just wrap modulo-2, right. Some
user-space projects even test with unsigned overflow warnings or
implicit truncation warnings, which are formally legal, but frequently
bugs.