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@xxxxxxxxxxxx>
>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
