include/asm-generic/cmpxchg-local.h: perform comparison in cmpxchg using appropriate size of data
PATCH 1/1 include/asm-generic/cmpxchg-local.h: perform comparison in cmpxchg using appropriate size of data
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.
---
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(-)
include/asm-generic/cmpxchg-local.h +4 -4
@@ -25,19 +25,19 @@ static inline unsigned long __cmpxchg_loraw_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 :
Arnd Bergmann
maintainerQuoted 8 lines · 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?
Quoted 4 lines
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.
Quoted 7 lines
Signed-off-by Darius Rad <[email protected]>---
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?
Quoted 20 lines
include/asm-generic/cmpxchg-local.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)include/asm-generic/cmpxchg-local.h +2
-2@@ -25,19 +25,19 @@ static inline unsigned long __cmpxchg_loraw_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;
include/asm-generic/cmpxchg-local.h static inline unsigned long __cmpxchg_lo · line 43
Quoted 10 lines
include/asm-generic/cmpxchg-local.h +2
-2-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
Quoted 10 lines · Arnd Bergmann
Quoted 8 lines · 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 *).
Quoted 6 lines
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.
Quoted 9 lines
Quoted 4 lines
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.
Quoted 10 lines
Quoted 7 lines
Signed-off-by Darius Rad <[email protected]>---
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.
Quoted 25 lines
Quoted 25 lines
include/asm-generic/cmpxchg-local.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)include/asm-generic/cmpxchg-local.h +3
-3@@ -25,19 +25,19 @@ static inline unsigned long __cmpxchg_loraw_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;
Quoted 9 lines
include/asm-generic/cmpxchg-local.h static inline unsigned long __cmpxchg_lo · line 43
Quoted 5 lines
include/asm-generic/cmpxchg-local.h +1
-1-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.
Quoted 2 lines
Arnd
// darius