[PATCH] x86/atomic: Fix UB on various atomic helpers

[PATCH] x86/atomic: Fix UB on various atomic helpers

[Alejandro Vallejo]

The atomic interface is signed. There's UB if a caller adds over INT_MAX
or subtracts below INT_MIN where the addition/subtraction is done in
C, just like in a regular int. Make the outward facing interface respect
two's complement on wraparound by round-tripping through unsigned for
arithmetic.

While at it, fix the while loop to have Xen style.

Fixes: e0efc0738163 ("atomic: replace atomic_compareandswap() with atomic_cmpxchg()")
Fixes: c98c80084d77 ("asm/atomic.h: implement missing and add common prototypes")
Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
---
I'd prefer to turn this into an unsigned interface, but then some
callers would need changing in non-trivial ways.

OTOH, one could argue the atomics merely highlight the UB already
present in signed arithmetic, and thus this doesn't need fixing. I'd
prefer to remove UB where possible.
---
 xen/arch/x86/include/asm/atomic.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/include/asm/atomic.h b/xen/arch/x86/include/asm/atomic.h
index ed4e09a503..8f227e0e6f 100644
--- a/xen/arch/x86/include/asm/atomic.h
+++ b/xen/arch/x86/include/asm/atomic.h
@@ -122,7 +122,7 @@ static inline void atomic_add(int i, atomic_t *v)
 
 static inline int atomic_add_return(int i, atomic_t *v)
 {
-    return i + arch_fetch_and_add(&v->counter, i);
+    return (unsigned int)i + arch_fetch_and_add(&v->counter, i);
 }
 
 static inline void atomic_sub(int i, atomic_t *v)
@@ -135,7 +135,9 @@ static inline void atomic_sub(int i, atomic_t *v)
 
 static inline int atomic_sub_return(int i, atomic_t *v)
 {
-    return arch_fetch_and_add(&v->counter, -i) - i;
+    unsigned int ui = i;
+
+    return arch_fetch_and_add(&v->counter, -ui) - ui;
 }
 
 static inline int atomic_sub_and_test(int i, atomic_t *v)
@@ -221,8 +223,9 @@ static inline int atomic_add_unless(atomic_t *v, int a, int u)
     int c, old;
 
     c = atomic_read(v);
-    while (c != u && (old = atomic_cmpxchg(v, c, c + a)) != c)
+    while ( c != u && (old = atomic_cmpxchg(v, c, (unsigned int)c + a)) != c )
         c = old;
+
     return c;
 }
 

base-commit: 7168cc02f86fb069da6e7405afbf5c7a1c215b0d
-- 
2.43.0

Re: [PATCH] x86/atomic: Fix UB on various atomic helpers

[Jan Beulich]

On 14.10.2026 15:35, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/include/asm/atomic.h
> +++ b/xen/arch/x86/include/asm/atomic.h
> @@ -122,7 +122,7 @@ static inline void atomic_add(int i, atomic_t *v)
>  
>  static inline int atomic_add_return(int i, atomic_t *v)
>  {
> -    return i + arch_fetch_and_add(&v->counter, i);
> +    return (unsigned int)i + arch_fetch_and_add(&v->counter, i);
>  }

I'm concerned seeing this kind of change: It's merely papering over an
issue in the caller. There's not going to be UB if the caller passes
suitable values. And the atomic types hardly are meant to be used in
contexts where overflow could occur.

Furthermore different parts of the C spec say different things here [1],
[2]. Hence whether overflow in signed calculations is undefined or
implementation-defined doesn't become entirely clear, according to my
reading.

Yes, [2] is from "Conversions" when here we talk about expression
results, but unsigned overflow not being UB also only follows from
what's said under "Conversions".

Jan

[1] From "Expressions":

"If an exceptional condition occurs during the evaluation of an expression
 (that is, if the result is not mathematically defined or not in the range
 of representable values for its type), the behavior is undefined."

[2] From "Conversions" / "Arithmetic operands" / "Signed and unsigned
    Integers":

"Otherwise, the new type is signed and the value cannot be represented in
 it; either the result is implementation-defined or an implementation-
 defined signal is raised."

Re: [PATCH] x86/atomic: Fix UB on various atomic helpers

[Alejandro Vallejo]

On Wed Oct 14, 2026 at 4:33 PM CEST, Jan Beulich wrote:
> On 14.10.2026 15:35, Alejandro Vallejo wrote:
> > --- a/xen/arch/x86/include/asm/atomic.h
> > +++ b/xen/arch/x86/include/asm/atomic.h
> > @@ -122,7 +122,7 @@ static inline void atomic_add(int i, atomic_t *v)
> >  
> >  static inline int atomic_add_return(int i, atomic_t *v)
> >  {
> > -    return i + arch_fetch_and_add(&v->counter, i);
> > +    return (unsigned int)i + arch_fetch_and_add(&v->counter, i);
> >  }
>
> I'm concerned seeing this kind of change: It's merely papering over an
> issue in the caller. There's not going to be UB if the caller passes
> suitable values. And the atomic types hardly are meant to be used in
> contexts where overflow could occur.
>
> Furthermore different parts of the C spec say different things here [1],
> [2]. Hence whether overflow in signed calculations is undefined or
> implementation-defined doesn't become entirely clear, according to my
> reading.

I won't dwell on interpreting the standard, because I only care how
GCC and Clang developers interpreted it; and that's a hard trap on
signed overflow with sanitisers enabled.

I can live without this patch (it's just paranoia), but thinking about
it some more, perhaps we ought to pass -fno-strict-overflow, like Linux
does, to the compiler. This brings forward post-C23 overflow semantics
and removes this whole class of UB wholesale. Here and elsewhere.

Thoughts?

Cheers,
Alejandro