[PATCH] locking/atomic: cmpxchg: Make __generic_cmpxchg_local compare against zero-extended 'old' value

[PATCH] locking/atomic: cmpxchg: Make __generic_cmpxchg_local compare against zero-extended 'old' value

[Matt Evans]

__generic_cmpxchg_local takes unsigned long old/new arguments which
might end up being up-cast from smaller signed types (which will
sign-extend).  The loaded compare value must be compared against a
truncated smaller type, so down-cast appropriately for each size.

The issue is apparent on 64-bit machines with code, such as
atomic_dec_unless_positive(), that sign-extends from int.

64-bit machines generally don't use the generic cmpxchg but
development/early ports might make use of it, so make it correct.

Signed-off-by: Matt Evans <mev@rivosinc.com>
---
 include/asm-generic/cmpxchg-local.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/asm-generic/cmpxchg-local.h b/include/asm-generic/cmpxchg-local.h
index 380cdc824e4b..c3e7315b7c1d 100644
--- a/include/asm-generic/cmpxchg-local.h
+++ b/include/asm-generic/cmpxchg-local.h
@@ -26,15 +26,15 @@ static inline unsigned long __generic_cmpxchg_local(volatile void *ptr,
 	raw_local_irq_save(flags);
 	switch (size) {
 	case 1: prev = *(u8 *)ptr;
-		if (prev == old)
+		if (prev == (u8)old)
 			*(u8 *)ptr = (u8)new;
 		break;
 	case 2: prev = *(u16 *)ptr;
-		if (prev == old)
+		if (prev == (u16)old)
 			*(u16 *)ptr = (u16)new;
 		break;
 	case 4: prev = *(u32 *)ptr;
-		if (prev == old)
+		if (prev == (u32)old)
 			*(u32 *)ptr = (u32)new;
 		break;
 	case 8: prev = *(u64 *)ptr;
-- 
2.30.2

Re: [PATCH] locking/atomic: cmpxchg: Make __generic_cmpxchg_local compare against zero-extended 'old' value

[Arnd Bergmann]

On Wed, Feb 1, 2023, at 19:39, Matt Evans wrote:
> __generic_cmpxchg_local takes unsigned long old/new arguments which
> might end up being up-cast from smaller signed types (which will
> sign-extend).  The loaded compare value must be compared against a
> truncated smaller type, so down-cast appropriately for each size.
>
> The issue is apparent on 64-bit machines with code, such as
> atomic_dec_unless_positive(), that sign-extends from int.
>
> 64-bit machines generally don't use the generic cmpxchg but
> development/early ports might make use of it, so make it correct.
>
> Signed-off-by: Matt Evans <mev@rivosinc.com>
> ---

Applied to the asm-generic tree for 6.3, thanks

    Arnd

Re: [PATCH] locking/atomic: cmpxchg: Make __generic_cmpxchg_local compare against zero-extended 'old' value

[Arnd Bergmann]

On Wed, Feb 1, 2023, at 19:39, Matt Evans wrote:
> __generic_cmpxchg_local takes unsigned long old/new arguments which
> might end up being up-cast from smaller signed types (which will
> sign-extend).  The loaded compare value must be compared against a
> truncated smaller type, so down-cast appropriately for each size.
>
> The issue is apparent on 64-bit machines with code, such as
> atomic_dec_unless_positive(), that sign-extends from int.
>
> 64-bit machines generally don't use the generic cmpxchg but
> development/early ports might make use of it, so make it correct.
>
> Signed-off-by: Matt Evans <mev@rivosinc.com>

Hi Matt,

I'm getting emails about nios2 sparse warnings from the
kernel test robot about your patch. I can also reproduce
this on armv5:


fs/erofs/zdata.c: note: in included file (through /home/arnd/arm-soc/arch/arm/include/asm/cmpxchg.h, /home/arnd/arm-soc/arch/arm/include/asm/atomic.h, /home/arnd/arm-soc/include/linux/atomic.h, ...):
include/asm-generic/cmpxchg-local.h:29:33: warning: cast truncates bits from constant value (5f0ecafe becomes fe)
include/asm-generic/cmpxchg-local.h:33:34: warning: cast truncates bits from constant value (5f0ecafe becomes cafe)
include/asm-generic/cmpxchg-local.h:29:33: warning: cast truncates bits from constant value (5f0ecafe becomes fe)
include/asm-generic/cmpxchg-local.h:30:42: warning: cast truncates bits from constant value (5f0edead becomes ad)
include/asm-generic/cmpxchg-local.h:33:34: warning: cast truncates bits from constant value (5f0ecafe becomes cafe)
include/asm-generic/cmpxchg-local.h:34:44: warning: cast truncates bits from constant value (5f0edead becomes dead)

This was already warning for the 'new' cast, but now also warns
for the 'old' cast, so the bot thinks this is a new problem.

I managed to shut up the warning by using a binary '&' operator
instead of the cast, but I wonder if it would be better to do
also mask this in the caller, when arch_atomic_cmpxchg() with its
signed argument calls into arch_cmpxchg() with its unsigned argument:

diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index 04b8be9f1a77..e271d6708c87 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -130,7 +130,7 @@ ATOMIC_OP(xor, ^)
 #define arch_atomic_read(v)                    READ_ONCE((v)->counter)
 #define arch_atomic_set(v, i)                  WRITE_ONCE(((v)->counter), (i))
 
-#define arch_atomic_xchg(ptr, v)               (arch_xchg(&(ptr)->counter, (v)))
-#define arch_atomic_cmpxchg(v, old, new)       (arch_cmpxchg(&((v)->counter), (old), (new)))
+#define arch_atomic_xchg(ptr, v)               (arch_xchg(&(ptr)->counter, (u32)(v)))
+#define arch_atomic_cmpxchg(v, old, new)       (arch_cmpxchg(&((v)->counter), (u32)(old), (u32)(new)))
 
 #endif /* __ASM_GENERIC_ATOMIC_H */
diff --git a/include/asm-generic/cmpxchg-local.h b/include/asm-generic/cmpxchg-local.h
index c3e7315b7c1d..f9d52d1f0472 100644
--- a/include/asm-generic/cmpxchg-local.h
+++ b/include/asm-generic/cmpxchg-local.h
@@ -26,20 +26,20 @@ static inline unsigned long __generic_cmpxchg_local(volatile void *ptr,
        raw_local_irq_save(flags);
        switch (size) {
        case 1: prev = *(u8 *)ptr;
-               if (prev == (u8)old)
-                       *(u8 *)ptr = (u8)new;
+               if (prev == (old & 0xff))
+                       *(u8 *)ptr = (new & 0xffu);
                break;
        case 2: prev = *(u16 *)ptr;
-               if (prev == (u16)old)
-                       *(u16 *)ptr = (u16)new;
+               if (prev == (old & 0xffffu))
+                       *(u16 *)ptr = (new & 0xffffu);
                break;
        case 4: prev = *(u32 *)ptr;
-               if (prev == (u32)old)
-                       *(u32 *)ptr = (u32)new;
+               if (prev == (old & 0xffffffffu))
+                       *(u32 *)ptr = (new & 0xffffffffu);
                break;
        case 8: prev = *(u64 *)ptr;
                if (prev == old)
-                       *(u64 *)ptr = (u64)new;
+                       *(u64 *)ptr = new;
                break;
        default:
                wrong_size_cmpxchg(ptr);


     Arnd

Re: [PATCH] locking/atomic: cmpxchg: Make __generic_cmpxchg_local compare against zero-extended 'old' value

[Matt Evans]

Hi Arnd,

> On 26 Feb 2023, at 10:13, Arnd Bergmann <arnd@arndb.de> wrote:
> 
> On Wed, Feb 1, 2023, at 19:39, Matt Evans wrote:
>> __generic_cmpxchg_local takes unsigned long old/new arguments which
>> might end up being up-cast from smaller signed types (which will
>> sign-extend).  The loaded compare value must be compared against a
>> truncated smaller type, so down-cast appropriately for each size.
>> 
>> The issue is apparent on 64-bit machines with code, such as
>> atomic_dec_unless_positive(), that sign-extends from int.
>> 
>> 64-bit machines generally don't use the generic cmpxchg but
>> development/early ports might make use of it, so make it correct.
>> 
>> Signed-off-by: Matt Evans <mev@rivosinc.com>
> 
> Hi Matt,
> 
> I'm getting emails about nios2 sparse warnings from the
> kernel test robot about your patch. I can also reproduce
> this on armv5:
> 
> 
> fs/erofs/zdata.c: note: in included file (through /home/arnd/arm-soc/arch/arm/include/asm/cmpxchg.h, /home/arnd/arm-soc/arch/arm/include/asm/atomic.h, /home/arnd/arm-soc/include/linux/atomic.h, ...):
> include/asm-generic/cmpxchg-local.h:29:33: warning: cast truncates bits from constant value (5f0ecafe becomes fe)
> include/asm-generic/cmpxchg-local.h:33:34: warning: cast truncates bits from constant value (5f0ecafe becomes cafe)
> include/asm-generic/cmpxchg-local.h:29:33: warning: cast truncates bits from constant value (5f0ecafe becomes fe)
> include/asm-generic/cmpxchg-local.h:30:42: warning: cast truncates bits from constant value (5f0edead becomes ad)
> include/asm-generic/cmpxchg-local.h:33:34: warning: cast truncates bits from constant value (5f0ecafe becomes cafe)
> include/asm-generic/cmpxchg-local.h:34:44: warning: cast truncates bits from constant value (5f0edead becomes dead)
> 
> This was already warning for the 'new' cast, but now also warns
> for the 'old' cast, so the bot thinks this is a new problem.

Thank you!  Hmm, indeed, it’s “more of the same” warning-wise but your alternative is nicer.

> I managed to shut up the warning by using a binary '&' operator
> instead of the cast, but I wonder if it would be better to do
> also mask this in the caller, when arch_atomic_cmpxchg() with its
> signed argument calls into arch_cmpxchg() with its unsigned argument:

Proposed patch LGTM, but one query:  are the casts in arch_[cmp]xchg()’s args necessary?  The new masks should deal with the issue (and consistency would imply same for all other users of arch_cmpxchg(), not ideal).

> diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
> index 04b8be9f1a77..e271d6708c87 100644
> --- a/include/asm-generic/atomic.h
> +++ b/include/asm-generic/atomic.h
> @@ -130,7 +130,7 @@ ATOMIC_OP(xor, ^)
> #define arch_atomic_read(v)                    READ_ONCE((v)->counter)
> #define arch_atomic_set(v, i)                  WRITE_ONCE(((v)->counter), (i))
> 
> -#define arch_atomic_xchg(ptr, v)               (arch_xchg(&(ptr)->counter, (v)))
> -#define arch_atomic_cmpxchg(v, old, new)       (arch_cmpxchg(&((v)->counter), (old), (new)))
> +#define arch_atomic_xchg(ptr, v)               (arch_xchg(&(ptr)->counter, (u32)(v)))
> +#define arch_atomic_cmpxchg(v, old, new)       (arch_cmpxchg(&((v)->counter), (u32)(old), (u32)(new)))
> 
> #endif /* __ASM_GENERIC_ATOMIC_H */
> diff --git a/include/asm-generic/cmpxchg-local.h b/include/asm-generic/cmpxchg-local.h
> index c3e7315b7c1d..f9d52d1f0472 100644
> --- a/include/asm-generic/cmpxchg-local.h
> +++ b/include/asm-generic/cmpxchg-local.h
> @@ -26,20 +26,20 @@ static inline unsigned long __generic_cmpxchg_local(volatile void *ptr,
>        raw_local_irq_save(flags);
>        switch (size) {
>        case 1: prev = *(u8 *)ptr;
> -               if (prev == (u8)old)
> -                       *(u8 *)ptr = (u8)new;
> +               if (prev == (old & 0xff))
> +                       *(u8 *)ptr = (new & 0xffu);
>                break;
>        case 2: prev = *(u16 *)ptr;
> -               if (prev == (u16)old)
> -                       *(u16 *)ptr = (u16)new;
> +               if (prev == (old & 0xffffu))
> +                       *(u16 *)ptr = (new & 0xffffu);
>                break;
>        case 4: prev = *(u32 *)ptr;
> -               if (prev == (u32)old)
> -                       *(u32 *)ptr = (u32)new;
> +               if (prev == (old & 0xffffffffu))
> +                       *(u32 *)ptr = (new & 0xffffffffu);
>                break;
>        case 8: prev = *(u64 *)ptr;
>                if (prev == old)
> -                       *(u64 *)ptr = (u64)new;
> +                       *(u64 *)ptr = new;
>                break;
>        default:
>                wrong_size_cmpxchg(ptr);

FWIW (including the casts in atomic.h, if you prefer):

Reviewed-by: Matt Evans <mev@rivosinc.com>


Thanks,


Matt


> 
> 
>     Arnd

Re: [PATCH] locking/atomic: cmpxchg: Make __generic_cmpxchg_local compare against zero-extended 'old' value

[Arnd Bergmann]

On Wed, Mar 1, 2023, at 16:19, Matt Evans wrote:
>> On 26 Feb 2023, at 10:13, Arnd Bergmann <arnd@arndb.de> wrote:
>
> Proposed patch LGTM, but one query:  are the casts in 
> arch_[cmp]xchg()’s args necessary?  The new masks should deal with the 
> issue (and consistency would imply same for all other users of 
> arch_cmpxchg(), not ideal).

I don't think they are necessary, but I'd like to stay on the
safe side here since there are so many different implementations of
arch_cmpxchg() across architectures that it's likely that I missed
one that has a similar but, or that another one will get added
in the future.

> FWIW (including the casts in atomic.h, if you prefer):
>
> Reviewed-by: Matt Evans <mev@rivosinc.com>

Thanks!

     Arnd