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


On 06/28/2011 10:38 AM, Shawn Bohrer wrote:
> On Tue, Jun 28, 2011 at 07:52:06AM -0700, Darren Hart wrote:
>>
>>
>> On 06/28/2011 03:54 AM, Peter Zijlstra wrote:
>>> On Mon, 2011-06-27 at 17:22 -0500, 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.
>>>>
>>>> While fixing the regression this patch opens up two possible bad
>>>> scenarios as identified by KOSAKI Motohiro:
>>>>
>>>> 1) This patch also allows FUTEX_WAIT on RO private mappings which have
>>>> the following corner case.
>>>>
>>>> Thread-A: call futex(FUTEX_WAIT, memory-region-A).
>>>> get_futex_key() return inode based key.
>>>> sleep on the key
>>>> Thread-B: call mprotect(PROT_READ|PROT_WRITE, memory-region-A)
>>>> Thread-B: write memory-region-A.
>>>> COW happen. This process's memory-region-A become related
>>>> to new COWed private (ie PageAnon=1) page.
>>>> Thread-B: call futex(FUETX_WAKE, memory-region-A).
>>>> get_futex_key() return mm based key.
>>>> IOW, we fail to wake up Thread-A.
>>>>
>>>> 2) Current futex code doesn't handle zero page properly.
>>>>
>>>> Read mode get_user_pages() can return zero page, but current futex
>>>> code doesn't handle it at all. Then, zero page makes infinite loop
>>>> internally.
>>>>
>>>> This Patch is based on Peter Zijlstra's initial patch with modifications to
>>>> only allow RO mappings for futex operations that need VERIFY_READ access.
>>>>
>>>> Reported-by: David Oliver <david@rgmadvisors.com>
>>>> Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
>>>
>>> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
>>
>> I think we need to address #2 above first.
>
> I believe the following contrived case triggers the zero page problem:
>
> #include <errno.h>
> #include <fcntl.h>
> #include <stdint.h>
> #include <linux/futex.h>
> #include <sys/mman.h>
> #include <sys/syscall.h>
> #include <unistd.h>
> #include <stdio.h>
>
>
> int main(int argc, char *argv[])
> {
> int fd, *futex, rc, val = 42;
> struct timespec ts = {.tv_sec = 2, .tv_nsec = 0 };
>
> fd = open("/tmp/futex_test", O_RDWR|O_CREAT, 0644);
> write(fd, &val, 4);
> futex = (int *)mmap(0, sizeof(int), PROT_READ, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> rc = syscall(SYS_futex, futex, FUTEX_WAIT, val, &ts, 0, 0);
> printf("rc=%d errno=%d\n", rc, errno);
> }
>
> Running the program above with my patch applied will spin in the
> kernel at 100% CPU usage.
>
>> 1) Is the loop killable?
>
> Yes, SIGINT causes the program to return.

OK, so while it is killable... I'd really rather not introduce a busy loop in the
kernel. Especially not after we removed it with:

http://lkml.org/lkml/2010/1/13/136

How about adding something like this. I _think_ the only way to get a ZERO_PAGE
here now is with the contrived testcase below.

$ git diff
diff --git a/kernel/futex.c b/kernel/futex.c
index 1737a66..a5417c2 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -316,6 +316,13 @@ again:
if (!page_head->mapping) {
unlock_page(page_head);
put_page(page_head);
+ /*
+ * ZERO_PAGE pages don't have a mapping. Avoid a busy loop
+ * trying to find one. RW mapping would have COW'd (and thus
+ * have a mapping) so this page is RO and won't ever change.
+ */
+ if ((page_head == ZERO_PAGE(address)))
+ return -EFAULT;
goto again;
}

This returns EFAULT for the following testcase which spun in get_futex_key with
your V3 patch.


#include <errno.h>
#include <fcntl.h>
#include <stdint.h>
#include <linux/futex.h>
#include <sys/mman.h>
#include <sys/syscall.h>
#include <unistd.h>
#include <stdio.h>


int main(int argc, char *argv[])
{
int fd, *futex, rc;
struct timespec ts = {.tv_sec = 2, .tv_nsec = 0 };
futex = (int *)mmap(0, sizeof(int), PROT_READ, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
printf("futex @ %p\n", futex);
rc = syscall(SYS_futex, futex, FUTEX_WAIT, 0, &ts, 0, 0);
printf("rc=%d errno=%d\n", rc, errno);
}
Thanks,

Darren

>
>> 2) If not, and we try to catch zero_page scenario, we need to ensure RO
>> sparse file mapping work. Peter notes that these are probably fine,
>> filemap.c doesn't seem to use the zero page - but we need to test.
>>
>> It's agreed, I believe, between myself, Peter, and Shawn that we will
>> document Private RW mappings as unsupported.
>>
>> --
>> Darren Hart
>> Intel Open Source Technology Center
>> Yocto Project - Linux Kernel
>
>
> ---------------------------------------------------------------
> This email, along with any attachments, is confidential. If you
> believe you received this message in error, please contact the
> sender immediately and delete all copies of the message.
> Thank you.
>

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


\
 
 \ /
  Last update: 2011-06-29 01:59    [from the cache]
©2003-2011 Jasper Spaans