lkml.org 
[lkml]   [2011]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
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    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site