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

[Darius Rad <darius@bluespec.com>]

On 08/26/2014 08:33 AM, Arnd Bergmann wrote:
> 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?
> 

Yes, the problem is when the caller uses a signed type (that is also smaller
than unsigned long).  For example, set_last_pid() in kernel/pid.c passes an
int.

And yes, the same code works without using the generic code.  In my testing,
simply casting the u32 case comparison makes the code work, whereas
with the generic code everything in user space gets killed.


>>  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.
> 

Understood.  I noticed that some architectures (e.g., MIPS, Xtensa) only
implement the 4 and 8 byte varieties, which is why I made that initial
assumption.


>> 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?
> 

Correct.


>>
>>  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.
> 

Indeed, I agree that casting 'prev' is not necessary.


> 	Arnd
> 

// darius