lkml.org 
[lkml]   [2012]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
Subject[regression] Re: [PATCH 2/3] futex: Sanitize cmpxchg_futex_value_locked API
(reset cc list)
Hi,

Michel Lespinasse wrote:

> This change makes the cmpxchg_futex_value_locked API more similar to the
> get_futex_value_locked one, returning an error code and updating the
> original value through a reference argument.
[...]
> Acked-by: Tony Luck <tony.luck@intel.com> [ia64]

Émeric Maschino (cc-ed) is experiencing random crashes, X restarts,
and so on on Itanium. Bisects to this patch[1].

Patrick Baggett, investigating, wrote[2]:

> It doesn't look like the return value (r8) is actually being set beyond
> initialized to 0. If there is some ia64 instruction that modifies it, GCC
> doesn't know about it from the inline assembly (r8 doesn't appear in the
> inputs/outputs list). From looking at the x86 version (agh, inline asm is
> hard to parse), it does modify the return value based on whether the
> comparison was a success or not, and the return value is certainly used by
> the callers.

And indeed, pinning that variable to that register (why not "prev"
instead?) looks suspicious.

Anywhere, here's the potentially problematic patch hunk. Ideas?

> --- a/arch/ia64/include/asm/futex.h
> +++ b/arch/ia64/include/asm/futex.h
> @@ -100,23 +100,26 @@ futex_atomic_op_inuser (int encoded_op, int __user *uaddr)
> }
>
> static inline int
> -futex_atomic_cmpxchg_inatomic(int __user *uaddr, int oldval, int newval)
> +futex_atomic_cmpxchg_inatomic(int *uval, int __user *uaddr,
> + int oldval, int newval)
> {
> if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
> return -EFAULT;
>
> {
> - register unsigned long r8 __asm ("r8");
> + register unsigned long r8 __asm ("r8") = 0;
> + unsigned long prev;
> __asm__ __volatile__(
> " mf;; \n"
> " mov ar.ccv=%3;; \n"
> "[1:] cmpxchg4.acq %0=[%1],%2,ar.ccv \n"
> " .xdata4 \"__ex_table\", 1b-., 2f-. \n"
> "[2:]"
> - : "=r" (r8)
> + : "=r" (prev)
> : "r" (uaddr), "r" (newval),
> "rO" ((long) (unsigned) oldval)
> : "memory");
> + *uval = prev;
> return r8;
> }
> }

Jonathan

[1] https://bugzilla.kernel.org/show_bug.cgi?id=42757
[2] http://thread.gmane.org/gmane.linux.debian.ports.ia64/3121/focus=3123
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2012-03-05 01:03    [from the cache]
©2003-2011 Jasper Spaans