Re: [PATCH 2/2] ARM: futex: make futex_detect_cmpxchg more reliable

[Arnd Bergmann]

On Fri, Mar 8, 2019 at 12:56 PM Ard Biesheuvel
<ard.biesheuvel@xxxxxxxxxx> wrote:
> On Fri, 8 Mar 2019 at 11:58, Russell King - ARM Linux admin <linux@xxxxxxxxxxxxxxx> wrote:
> > On Fri, Mar 08, 2019 at 11:45:21AM +0100, Ard Biesheuvel wrote:

>
> Perhaps. So let me summarize what I do understand.
>
> 1) if futex_atomic_cmpxchg_inatomic() is instantiated *and executed*
> with the same compile time constant value of 0x0 for newval and uaddr,
> we end up with an opcode for the STRT instruction that is CONSTRAINED
> UNPREDICTABLE, but we will never execute it since the preceding load
> will fault and enter the fixup handler.
> 2) such occurrences of futex_atomic_cmpxchg_inatomic() are unlikely to
> occur in the code, but may be instantiated by the compiler when doing
> constant propagation (like in the ilog2() case I quoted), but these
> instantiations will never be called
> 3) both clang and gcc may map different inline asm input operands onto
> the same register if the value is guaranteed to be the same (i.e.,
> they are both compile time constants)
>
> My statement about uaddr was slightly misguided, in the sense that our
> invocation of STRT does use the post-index variant, but with an
> increment of zero, so the value written back to the register equals
> the original value. But it does explain why this opcode is CONSTRAINED
> UNPREDICTABLE in the first place.
>
> Given 2) above, I wonder if anyone could confirm whether adding
> 'BUG_ON(__builtin_constant_p(uaddr))' silences the warning as well.

Like this?

diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h
index 0a46676b4245..e6e9b403cd61 100644
--- a/arch/arm/include/asm/futex.h
+++ b/arch/arm/include/asm/futex.h
@@ -57,6 +57,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
        /* Prefetching cannot fault */
        prefetchw(uaddr);
        __ua_flags = uaccess_save_and_enable();
+       BUG_ON(__builtin_constant_p(uaddr) || !uaddr);
        __asm__ __volatile__("@futex_atomic_cmpxchg_inatomic\n"
        "1:     ldrex   %1, [%4]\n"
        "       teq     %1, %2\n"

This had no effect here.

My first attempt (before finding the original patch from Mikael Pettersson)
was to change the probe to pass '1' as the value instead of '0', that
worked fine.

     Arnd