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

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

[Darius Rad]

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 *).  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 cmpxchg.

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.

Patch generated against 3.17-rc1.

 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:

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

[Arnd Bergmann]

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

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

[Darius Rad]

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