Re: [PATCH] include/asm-generic/cmpxchg-local.h: perform comparison in cmpxchg using appropriate size of data

[Arnd Bergmann <arnd@arndb.de>]

On Monday 25 August 2014 11:33:07 Darius Rad wrote:
> In the generic implementation of cmpxchg, cast the parameters to the size
> of the data prior to comparison.  Otherwise, it is possible for the
> comparison to be done incorrectly in the case where the data size is
> smaller than the architecture register size.
> 
> For example, on a 64-bit architecture, a 32-bit value is sign extended
> differently if it is typecast from an int to an unsigned long as compared
> to being loaded from memory via an unsigned pointer (u32 *).

I don't see the scenario where this applies yet. The function itself
only deals with unsigned values, so there should never be any sign extension.

Is this a problem with the caller using a signed type? Does the same
code work on architectures that don't use the generic code?

>  Given that
> the primary, or possibly only, use of cmpxchg is with 4 and 8 byte values,
> the incorrect comparison could only occur on 64-bit architectures that make
> use of the generic cmxchg.

cmpxchg is definitely meant to handle 1 and 2 byte values as well, but it's
relatively rare. ARMv6 for instance does not handle 2-byte atomics and only
one driver (xen) has had problems because of this.

> Signed-off-by: Darius Rad <darius@bluespec.com>
> 
> ---
> It does not appear that this is relevant to architectures that are in the
> kernel tree, since the generic cmpxchg is only ever used by some 32-bit
> architectures.  This does impact the RISC-V architecture that is currently
> in development.

I guess that means that RISC-V has no mandatory atomic instructions, right?

> 
>  include/asm-generic/cmpxchg-local.h |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- linux-3.17-rc1.orig/include/asm-generic/cmpxchg-local.h	2014-08-16 12:40:26.000000000 -0400
> +++ linux-3.17-rc1/include/asm-generic/cmpxchg-local.h	2014-08-22 14:26:59.280746090 -0400
> @@ -25,19 +25,19 @@ static inline unsigned long __cmpxchg_lo
>  	raw_local_irq_save(flags);
>  	switch (size) {
>  	case 1: prev = *(u8 *)ptr;
> -		if (prev == old)
> +		if ((u8)prev == (u8)old)
>  			*(u8 *)ptr = (u8)new;
>  		break;
>  	case 2: prev = *(u16 *)ptr;
> -		if (prev == old)
> +		if ((u16)prev == (u16)old)
>  			*(u16 *)ptr = (u16)new;
>  		break;
>  	case 4: prev = *(u32 *)ptr;
> -		if (prev == old)
> +		if ((u32)prev == (u32)old)
>  			*(u32 *)ptr = (u32)new;
>  		break;
>  	case 8: prev = *(u64 *)ptr;
> -		if (prev == old)
> +		if ((u64)prev == (u64)old)
>  			*(u64 *)ptr = (u64)new;
>  		break;
>  	default:

I can see how the cast of 'old' to a narrower type makes sense, but
not 'prev', which we just loaded and zero-extended one line earlier.

	Arnd