Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753603Ab1CNN5b (ORCPT ); Mon, 14 Mar 2011 09:57:31 -0400 Received: from www.tglx.de ([62.245.132.106]:50258 "EHLO www.tglx.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753242Ab1CNN53 (ORCPT ); Mon, 14 Mar 2011 09:57:29 -0400 Date: Mon, 14 Mar 2011 14:56:19 +0100 (CET) From: Thomas Gleixner To: Linus Torvalds cc: Michel Lespinasse , Darren Hart , Ingo Molnar , Peter Zijlstra , Matt Turner , Russell King , David Howells , Tony Luck , Michal Simek , Ralf Baechle , "James E.J. Bottomley" , Benjamin Herrenschmidt , Martin Schwidefsky , Paul Mundt , "David S. Miller" , Chris Metcalf , Andrew Morton , LKML Subject: Re: [PATCH 1/3] futex: do not pagefault_disable in futex_atomic_cmpxchg_inatomic() In-Reply-To: Message-ID: References: <20110307021127.GB31188@google.com> <20110309112550.GA3050@google.com> <20110311021654.GA26122@google.com> <20110311024731.GB26122@google.com> User-Agent: Alpine 2.00 (LFD 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3477 Lines: 90 On Mon, 14 Mar 2011, Thomas Gleixner wrote: > On Sun, 13 Mar 2011, Linus Torvalds wrote: > > On Thu, Mar 10, 2011 at 6:47 PM, Michel Lespinasse wrote: > That's my fault. > > I really checked the call sites of futex_atomic_cmpxchg_inatomic() and > totally failed to see the one in handle_futex_death() which does not > use the helper function cmpxchg_futex_value_locked(). That helper > function is safe and does the right thing: > > pagefault_disable(); > curval = futex_atomic_cmpxchg_inatomic(uaddr, uval, newval); > pagefault_enable(); > > So, that means we have all call sites covered except one, which needs > to be fixed _AND_ also pushed into stable as all arch implementations > except ARM rely on the caller doing the pagefault_disable(). After applying some coffee to my brain, I noticed that the ability to fault in handle_futex_death() is desired. The get_user() before that call covers the case where the futex is paged out, but it does not handle the case where the futex is in a non writeable mapping. That lacks a big fat comment at least. So the removal of the pagefault_disable() in ARM is correct, just the changelog and the comment there sucks. Sorry for not catching it. Thinking more about it. Adding a comment is to handle_futex_death() is good, but changing the code to make it entirely clear what is going on is even better. --------> Subject: futex: Deobfuscate handle_futex_death() From: Thomas Gleixner Date: Mon, 14 Mar 2011 10:34:35 +0100 handle_futex_death() uses futex_atomic_cmpxchg_inatomic() without disabling page faults. That's ok, but totally non obvious. We don't hold locks so we actually can and want to fault here, because the get_user() before futex_atomic_cmpxchg_inatomic() does not guarantee a R/W mapping. We could just add a big fat comment to explain this, but actually changing the code so that the functionality is entirely clear is better. Use the helper function which disables page faults around the futex_atomic_cmpxchg_inatomic() and handle a fault with a call to fault_in_user_writeable() as all other places in the futex code do as well. Signed-off-by: Thomas Gleixner --- kernel/futex.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) Index: linux-2.6-tip/kernel/futex.c =================================================================== --- linux-2.6-tip.orig/kernel/futex.c +++ linux-2.6-tip/kernel/futex.c @@ -2458,9 +2458,20 @@ retry: * userspace. */ mval = (uval & FUTEX_WAITERS) | FUTEX_OWNER_DIED; - if (futex_atomic_cmpxchg_inatomic(&nval, uaddr, uval, mval)) - return -1; - + /* + * We are not holding a lock here, but we want to have + * the pagefault_disable/enable() protection because + * we want to handle the fault gracefully. If the + * access fails we try to fault in the futex with R/W + * verification via get_user_pages. get_user() above + * does not guarantee R/W access. If that fails we + * give up and leave the futex locked. + */ + if (cmpxchg_futex_value_locked(&nval, uaddr, uval, mval)) { + if (fault_in_user_writeable(uaddr)) + return -1; + goto retry; + } if (nval != uval) goto retry; -- 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/