| Message ID | 20009.45690.158286.161591@pilspetsen.it.uu.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 22, 2011 at 6:25 PM, Mikael Pettersson <mikpe@it.uu.se> wrote: > Compiling kernel 3.0 for UP ARM (ARMv5) I see: > > kernel/futex.c: In function 'fixup_pi_state_owner': > kernel/futex.c:1549: warning: 'curval' may be used uninitialized in this function > kernel/futex.c: In function 'handle_futex_death': > kernel/futex.c:2454: warning: 'nval' may be used uninitialized in this function > /tmp/ccVDjh0q.s: Assembler messages: > /tmp/ccVDjh0q.s:113: Warning: source register same as write-back base > > The corresponding instruction is: > > 2: streqt r3, [r3] > > (same as strteq) which the ARMv5 ARM ARM describes as being UNPREDICTABLE. > > This code originates from futex_atomic_cmpxchg_inatomic(): > > static inline int > futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, > u32 oldval, u32 newval) > { > int ret = 0; > u32 val; > > if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))) > return -EFAULT; > > __asm__ __volatile__("@futex_atomic_cmpxchg_inatomic\n" > "1: " T(ldr) " %1, [%4]\n" > " teq %1, %2\n" > " it eq @ explicit IT needed for the 2b label\n" > "2: " T(streq) " %3, [%4]\n" > __futex_atomic_ex_table("%5") > : "+r" (ret), "=&r" (val) > : "r" (oldval), "r" (newval), "r" (uaddr), "Ir" (-EFAULT) > : "cc", "memory"); > > *uval = val; > return ret; > } > > The problem is that in the T(streq) insn, %3 and %4 MUST be different registers, > but nothing in the asm() constrains them to be different. kernel/futex.c:init_futex() > calls this with uaddr, oldval, and newval all zero, and the compiler may allocate > all three to the same register. That's exactly what happens for me with a gcc-4.4.6 > based compiler and the 2.6.39 and 3.0 kernels. (It didn't happen with 2.6.38.) > > One way of fixing this is to make uaddr an input/output register, since that prevents > it from overlapping any other input or output. The patch below does exactly that; > on the problematic fragment in futex.c it causes the following change: > > @@ -104,13 +104,14 @@ futex_init: > mvnne r3, #13 > bne .L6 > mvn r2, #13 > + mov r0, r3 > #APP > @ 97 "/tmp/linux-3.0/arch/arm/include/asm/futex.h" 1 > @futex_atomic_cmpxchg_inatomic > -1: ldrt r1, [r3] > +1: ldrt r1, [r0] > teq r1, r3 > it eq @ explicit IT needed for the 2b label > -2: streqt r3, [r3] > +2: streqt r3, [r0] > 3: > .pushsection __ex_table,"a" > .align 3 > > which is pretty much what we want. > > Lightly tested on an n2100 (UP, ARMv5). > > Does this seem like a reasonable solution, or is there a better way to force > distinct input registers to an asm()? > > /Mikael > > --- linux-3.0/arch/arm/include/asm/futex.h.~1~ 2011-07-22 12:01:07.000000000 +0200 > +++ linux-3.0/arch/arm/include/asm/futex.h 2011-07-22 18:31:08.000000000 +0200 > @@ -95,13 +95,13 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, > return -EFAULT; > > __asm__ __volatile__("@futex_atomic_cmpxchg_inatomic\n" > - "1: " T(ldr) " %1, [%4]\n" > - " teq %1, %2\n" > + "1: " T(ldr) " %1, [%2]\n" > + " teq %1, %3\n" > " it eq @ explicit IT needed for the 2b label\n" > - "2: " T(streq) " %3, [%4]\n" > + "2: " T(streq) " %4, [%2]\n" > __futex_atomic_ex_table("%5") > - : "+r" (ret), "=&r" (val) > - : "r" (oldval), "r" (newval), "r" (uaddr), "Ir" (-EFAULT) > + : "+r" (ret), "=&r" (val), "+r" (uaddr) > + : "r" (oldval), "r" (newval), "Ir" (-EFAULT) > : "cc", "memory"); This seems reasonable. For new code, I personally prefer named arguments for inline asm rather than numbered arguments, since that makes patches like this much more readable if the arguments need to be shuffled. Not sure if it's actually worth changing just for this, though... The two alternative ways of forcing different registers to be used are both a bit painful; either: a) declare explicit register int variables with asm("rX"); or b) use specific registers instead of % substitutions, and mark those registers as clobbered or save/restore them (ugh) Do we have the same potential bug for the __put_user functions in arch/arm/include/asm/uaccess.h? Although cases where the two relevant arguments could be statically assigned to the same register by the compiler will be rare, those macros are used all over the place. Cheers ---Dave
On Fri, Jul 22, 2011 at 07:25:14PM +0200, Mikael Pettersson wrote: > Compiling kernel 3.0 for UP ARM (ARMv5) I see: > > kernel/futex.c: In function 'fixup_pi_state_owner': > kernel/futex.c:1549: warning: 'curval' may be used uninitialized in this function > kernel/futex.c: In function 'handle_futex_death': > kernel/futex.c:2454: warning: 'nval' may be used uninitialized in this function > /tmp/ccVDjh0q.s: Assembler messages: > /tmp/ccVDjh0q.s:113: Warning: source register same as write-back base It's actually not a problem - the warning has been around for quite a long time and I've just been ignoring it having investigated it already. Why? The compare-and-exchange is guaranteed to fail at this point without this instruction even being executed. > @@ -104,13 +104,14 @@ futex_init: > mvnne r3, #13 > bne .L6 > mvn r2, #13 > #APP > @ 97 "/tmp/linux-3.0/arch/arm/include/asm/futex.h" 1 > @futex_atomic_cmpxchg_inatomic > -1: ldrt r1, [r3] > teq r1, r3 > it eq @ explicit IT needed for the 2b label > -2: streqt r3, [r3] > 3: > .pushsection __ex_table,"a" > .align 3 r3 = 0, so the ldrt causes a data abort, and invokes the fixup, which will skip over the above. So it's really not a problem, and this is the only place where that 'optimization' by the compiler will happen. So no need to fix it. Really.
On Fri, Jul 22, 2011 at 07:12:30PM +0100, Dave Martin wrote: > Do we have the same potential bug for the __put_user functions in > arch/arm/include/asm/uaccess.h? Although cases where the two > relevant arguments could be statically assigned to the same register > by the compiler will be rare, those macros are used all over the place. The only time it will happen is if the compiler sees that we're doing this: __put_user(x, x); In other words, you pass the same argument in for the value and pointer. That should never happen for two reasons: 1. It will fail the type checking and issue compiler warnings about mismatched types. 2. This is silly code. That goes for the futex case too - it can only happen if you pass in the same pointer twice (or an alias of it.) And that's rubbish code. The only other case it could happen is: __put_user(NULL, NULL); and I mean the constant 'NULL' not some NULL pointer. Again, this has to be written explicitly, and writing to address 0 is... very silly like this. So, nothing to fix there either.
Russell King - ARM Linux writes: > On Fri, Jul 22, 2011 at 07:25:14PM +0200, Mikael Pettersson wrote: > > Compiling kernel 3.0 for UP ARM (ARMv5) I see: > > > > kernel/futex.c: In function 'fixup_pi_state_owner': > > kernel/futex.c:1549: warning: 'curval' may be used uninitialized in this function > > kernel/futex.c: In function 'handle_futex_death': > > kernel/futex.c:2454: warning: 'nval' may be used uninitialized in this function > > /tmp/ccVDjh0q.s: Assembler messages: > > /tmp/ccVDjh0q.s:113: Warning: source register same as write-back base > > It's actually not a problem - the warning has been around for quite a long > time and I've just been ignoring it having investigated it already. Why? > The compare-and-exchange is guaranteed to fail at this point without this > instruction even being executed. > > > @@ -104,13 +104,14 @@ futex_init: > > mvnne r3, #13 > > bne .L6 > > mvn r2, #13 > > #APP > > @ 97 "/tmp/linux-3.0/arch/arm/include/asm/futex.h" 1 > > @futex_atomic_cmpxchg_inatomic > > -1: ldrt r1, [r3] > > teq r1, r3 > > it eq @ explicit IT needed for the 2b label > > -2: streqt r3, [r3] > > 3: > > .pushsection __ex_table,"a" > > .align 3 > > r3 = 0, so the ldrt causes a data abort, and invokes the fixup, which > will skip over the above. > > So it's really not a problem, and this is the only place where that > 'optimization' by the compiler will happen. So no need to fix it. > Really. OK. Patch withdrawn.
--- linux-3.0/arch/arm/include/asm/futex.h.~1~ 2011-07-22 12:01:07.000000000 +0200 +++ linux-3.0/arch/arm/include/asm/futex.h 2011-07-22 18:31:08.000000000 +0200 @@ -95,13 +95,13 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, return -EFAULT; __asm__ __volatile__("@futex_atomic_cmpxchg_inatomic\n" - "1: " T(ldr) " %1, [%4]\n" - " teq %1, %2\n" + "1: " T(ldr) " %1, [%2]\n" + " teq %1, %3\n" " it eq @ explicit IT needed for the 2b label\n" - "2: " T(streq) " %3, [%4]\n" + "2: " T(streq) " %4, [%2]\n" __futex_atomic_ex_table("%5") - : "+r" (ret), "=&r" (val) - : "r" (oldval), "r" (newval), "r" (uaddr), "Ir" (-EFAULT) + : "+r" (ret), "=&r" (val), "+r" (uaddr) + : "r" (oldval), "r" (newval), "Ir" (-EFAULT) : "cc", "memory"); *uval = val;
Compiling kernel 3.0 for UP ARM (ARMv5) I see: kernel/futex.c: In function 'fixup_pi_state_owner': kernel/futex.c:1549: warning: 'curval' may be used uninitialized in this function kernel/futex.c: In function 'handle_futex_death': kernel/futex.c:2454: warning: 'nval' may be used uninitialized in this function /tmp/ccVDjh0q.s: Assembler messages: /tmp/ccVDjh0q.s:113: Warning: source register same as write-back base The corresponding instruction is: 2: streqt r3, [r3] (same as strteq) which the ARMv5 ARM ARM describes as being UNPREDICTABLE. This code originates from futex_atomic_cmpxchg_inatomic(): static inline int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval, u32 newval) { int ret = 0; u32 val; if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))) return -EFAULT; __asm__ __volatile__("@futex_atomic_cmpxchg_inatomic\n" "1: " T(ldr) " %1, [%4]\n" " teq %1, %2\n" " it eq @ explicit IT needed for the 2b label\n" "2: " T(streq) " %3, [%4]\n" __futex_atomic_ex_table("%5") : "+r" (ret), "=&r" (val) : "r" (oldval), "r" (newval), "r" (uaddr), "Ir" (-EFAULT) : "cc", "memory"); *uval = val; return ret; } The problem is that in the T(streq) insn, %3 and %4 MUST be different registers, but nothing in the asm() constrains them to be different. kernel/futex.c:init_futex() calls this with uaddr, oldval, and newval all zero, and the compiler may allocate all three to the same register. That's exactly what happens for me with a gcc-4.4.6 based compiler and the 2.6.39 and 3.0 kernels. (It didn't happen with 2.6.38.) One way of fixing this is to make uaddr an input/output register, since that prevents it from overlapping any other input or output. The patch below does exactly that; on the problematic fragment in futex.c it causes the following change: @@ -104,13 +104,14 @@ futex_init: mvnne r3, #13 bne .L6 mvn r2, #13 + mov r0, r3 #APP @ 97 "/tmp/linux-3.0/arch/arm/include/asm/futex.h" 1 @futex_atomic_cmpxchg_inatomic -1: ldrt r1, [r3] +1: ldrt r1, [r0] teq r1, r3 it eq @ explicit IT needed for the 2b label -2: streqt r3, [r3] +2: streqt r3, [r0] 3: .pushsection __ex_table,"a" .align 3 which is pretty much what we want. Lightly tested on an n2100 (UP, ARMv5). Does this seem like a reasonable solution, or is there a better way to force distinct input registers to an asm()? /Mikael