lkml.org 
[lkml]   [2014]   [Aug]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] include/asm-generic/cmpxchg-local.h: perform comparison in cmpxchg using appropriate size of data
Date
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


\
 
 \ /
  Last update: 2014-08-26 15:01    [W:0.041 / U:0.384 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site