Messages in this thread | | | Date | Mon, 14 Mar 2011 13:07:10 -0700 | From | Darren Hart <> | Subject | Re: [PATCH 1/3] futex: do not pagefault_disable in futex_atomic_cmpxchg_inatomic() |
| |
On 03/14/2011 06:56 AM, Thomas Gleixner wrote: > 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<walken@google.com> 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<tglx@linutronix.de> > 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<tglx@linutronix.de>
Acked-by: Darren Hart <dvhart@linux.intel.com>
> --- > 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/
-- Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel
| |