lkml.org 
[lkml]   [2011]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH RFC] futex: Fix regression with read only mappings
    Hi Shawn,

    Thanks for taking this up. Would you share your testcases as well?

    On 06/22/2011 12:19 PM, Shawn Bohrer wrote:
    > commit 7485d0d3758e8e6491a5c9468114e74dc050785d (futexes: Remove rw
    > parameter from get_futex_key()) in 2.6.33 introduced a user-mode
    > regression in that it additionally prevented futex operations on a
    > region within a read only memory mapped file. For example this breaks
    > workloads that have one or more reader processes doing a FUTEX_WAIT on a
    > futex within a read only shared mapping, and a writer processes that has
    > a writable mapping issuing the FUTEX_WAKE.
    >
    > This fixes the regression for futex operations that should be valid on
    > RO mappings by trying a RO get_user_pages_fast() when the RW
    > get_user_pages_fast() fails so as not to slow down the common path of
    > writable anonymous maps and bailing when we used the RO path on
    > anonymous memory.
    >
    > Patch based on Peter Zijlstra's initial patch with modifications to only
    > allow RO mappings for futex operations that need VERIFY_READ access.
    >
    > Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
    > ---
    >
    > Interestingly this patch also allows doing a FUTEX_WAIT on a RO private
    > mapping.

    I don't see any harm in this.

    > Where my tests on 2.6.18 show that this used to wait
    > indefinitely. Performing a FUTEX_WAIT on a RW private mapping waits
    > indefinitely in 2.6.18, 3.0.0, and with this patch applied. It is
    > unclear to me if this is a good thing or a bug.
    >
    > kernel/futex.c | 38 ++++++++++++++++++++++++++------------
    > 1 files changed, 26 insertions(+), 12 deletions(-)
    >
    > diff --git a/kernel/futex.c b/kernel/futex.c
    > index fe28dc2..e8cd532 100644
    > --- a/kernel/futex.c
    > +++ b/kernel/futex.c
    > @@ -218,6 +218,8 @@ static void drop_futex_key_refs(union futex_key *key)
    > * @uaddr: virtual address of the futex
    > * @fshared: 0 for a PROCESS_PRIVATE futex, 1 for PROCESS_SHARED
    > * @key: address where result is stored.
    > + * @rw: mapping needs to be read/write (values: VERIFY_READ,
    > + * VERIFY_WRITE)
    > *


    I'm OK with this, but ideally I I'd prefer fshared and rw be replaced
    with flags. We already have a FLAGS_SHARED, adding a FLAGS_WRITE would
    be simple, and most call sites could be updated to send the bare flags
    rather than a logical and with FLAGS_SHARED as they do now.


    > * Returns a negative error code or 0
    > * The key words are stored in *key on success.
    > @@ -229,12 +231,12 @@ static void drop_futex_key_refs(union futex_key *key)
    > * lock_page() might sleep, the caller should not hold a spinlock.
    > */
    > static int
    > -get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key)
    > +get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
    > {
    > unsigned long address = (unsigned long)uaddr;
    > struct mm_struct *mm = current->mm;
    > struct page *page, *page_head;
    > - int err;
    > + int err, ro = 0;
    >
    > /*
    > * The futex address must be "naturally" aligned.
    > @@ -262,6 +264,10 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key)
    >
    > again:
    > err = get_user_pages_fast(address, 1, 1, &page);
    > + if (err == -EFAULT && rw == VERIFY_READ) {
    > + err = get_user_pages_fast(address, 1, 0, &page);

    I verified this is correct .... we ran into a little trouble a while
    back mixing up the parameters of get_user_pages_fast. This is correct :)

    > + ro = 1;
    > + }
    > if (err < 0)
    > return err;
    >
    > @@ -316,6 +322,11 @@ again:
    > * the object not the particular process.
    > */
    > if (PageAnon(page_head)) {
    > + if (ro) {
    > + err = -EFAULT;
    > + goto out;
    > + }

    This if block needs a comment as to why RO anonymous pages trigger a
    failure. In fact... I thought you said with this patch FUTEX_WAIT waits
    indefinitely on RO private mappings? This test suggests that it
    shouldn't. Are you testing this via glibc pthread_mutexes? If so, and if
    I remember correctly, glibc loops forever on -EFAULT here, unfortunately.

    > +
    > key->both.offset |= FUT_OFF_MMSHARED; /* ref taken on mm */
    > key->private.mm = mm;
    > key->private.address = address;
    > @@ -327,9 +338,11 @@ again:
    >
    > get_futex_key_refs(key);
    >
    > + err = 0;

    Shouldn't this be 0 anyway? If it was non-zero, you would have jumped to
    out: earlier, right?

    <snip/>

    Thanks,

    --
    Darren Hart
    Intel Open Source Technology Center
    Yocto Project - Linux Kernel


    \
     
     \ /
      Last update: 2011-06-22 22:17    [W:3.802 / U:0.108 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site