Re: Change in functionality of futex() system call.

Re: Change in functionality of futex() system call.

[George Spelvin]

I'm not sure if it's best, but the risk of RO waiters interfering could
be solved by giving them a lower prioirty for wakeup and always waking
RW-mapped waiters first.

In most cases, there would only be one type of waiter, so this would
have no effect, but if there were, a RO-mapped reader couldn't "steal"
wakeup from someone who had it RW-mapped.

It might conflict with a fairness goal, but it does solve the problem
without any extension to the API.

Re: Change in functionality of futex() system call.

[Darren Hart]

On 06/08/2011 05:44 PM, George Spelvin wrote:
> I'm not sure if it's best, but the risk of RO waiters interfering could
> be solved by giving them a lower prioirty for wakeup and always waking
> RW-mapped waiters first.

This strikes me as bending over backwards and jumping through hoops
inside the kernel to avoid having to do proper permissions management in
userspace.


> In most cases, there would only be one type of waiter, so this would
> have no effect, but if there were, a RO-mapped reader couldn't "steal"
> wakeup from someone who had it RW-mapped.
> 
> It might conflict with a fairness goal, but it does solve the problem
> without any extension to the API.

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

Re: Change in functionality of futex() system call.

[Andrew Lutomirski]

On Wed, Jun 8, 2011 at 11:02 PM, Darren Hart <dvhart@linux.intel.com> wrote:
> On 06/08/2011 05:44 PM, George Spelvin wrote:
>> I'm not sure if it's best, but the risk of RO waiters interfering could
>> be solved by giving them a lower prioirty for wakeup and always waking
>> RW-mapped waiters first.
>
> This strikes me as bending over backwards and jumping through hoops
> inside the kernel to avoid having to do proper permissions management in
> userspace.

Huh?

I still don't understand why userspace ought to need to deny read
access to a file to prevent DoS.  I think it's entirely reasonable for
userspace to make the assumption that users with read access cannot
make changes visible to writers unless explicitly documented (i.e.
file locking, which is so thoroughly broken that it shouldn't be taken
as an example of how to design anything).

Given that current kernels make this use safe and the proposal is to
make it unsafe, I think it's worth designing the interface to avoid
introducing new security problems.

--Andy

Re: Change in functionality of futex() system call.

[Eric Dumazet]

Le mercredi 08 juin 2011 à 23:38 -0400, Andrew Lutomirski a écrit :

> Huh?
> 
> I still don't understand why userspace ought to need to deny read
> access to a file to prevent DoS.  I think it's entirely reasonable for
> userspace to make the assumption that users with read access cannot
> make changes visible to writers unless explicitly documented (i.e.
> file locking, which is so thoroughly broken that it shouldn't be taken
> as an example of how to design anything).
> 
> Given that current kernels make this use safe and the proposal is to
> make it unsafe, I think it's worth designing the interface to avoid
> introducing new security problems.

I am very tired of this discussion, you repeat the same arguments over
and over.

You can not prevent DOS on a machine if you allow a process to RO map
your critical files (where you put futexes), because you allow this
process to interfere with critical cache lines bouncing between cpus.

Really, please forget about this crazy idea of allowing foreigners to
_read_ or memory _map_ your files. Dont do it.

Re: Change in functionality of futex() system call.

[Andrew Lutomirski]

On Wed, Jun 8, 2011 at 11:54 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> You can not prevent DOS on a machine if you allow a process to RO map
> your critical files (where you put futexes), because you allow this
> process to interfere with critical cache lines bouncing between cpus.

The cacheline bounce DoS slows things down and they go back to normal
when you kill the DoS-ing task.

The wakeup-eating DoS is permanent.  Seems a good deal worse to me.

If you make this change, please at least document it in the man page.

>
> Really, please forget about this crazy idea of allowing foreigners to
> _read_ or memory _map_ your files. Dont do it.
>

Then how am I supposed to efficiently broadcast information to
untrusted processes?  I'll have to put any futexes involved into
different files, but one way or another the actual data will have to
be memory mapped to avoid syscall overhead.

--Andy

Re: Change in functionality of futex() system call.

[Eric Dumazet]

Le jeudi 09 juin 2011 à 00:10 -0400, Andrew Lutomirski a écrit :
> On Wed, Jun 8, 2011 at 11:54 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > You can not prevent DOS on a machine if you allow a process to RO map
> > your critical files (where you put futexes), because you allow this
> > process to interfere with critical cache lines bouncing between cpus.
> 
> The cacheline bounce DoS slows things down and they go back to normal
> when you kill the DoS-ing task.
> 
> The wakeup-eating DoS is permanent.  Seems a good deal worse to me.
> 
> If you make this change, please at least document it in the man page.
> 


This is how futexes had working for years.

It was very obvious from the beginning. Please submit a man page change
since you raised the point. You own the credit to open a CVE and
immediately release a fix to all 2.6 versions !

How come a critical fix (according to you) went without being noticed
and documented ?

> Then how am I supposed to efficiently broadcast information to
> untrusted processes?  I'll have to put any futexes involved into
> different files, but one way or another the actual data will have to
> be memory mapped to avoid syscall overhead.

futexes are a linux extension over standard VM games.

If you dont know how to share a memory segment between a group of
processes, disallowing others to come spy on you, maybe its better to
use another IPC ?

Instead of 'fixing' futexes, what about educating people how to
correctly use memory segments ?

Re: Change in functionality of futex() system call.

[Andrew Lutomirski]

On Thu, Jun 9, 2011 at 1:11 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le jeudi 09 juin 2011 à 00:10 -0400, Andrew Lutomirski a écrit :
>> On Wed, Jun 8, 2011 at 11:54 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> >
>> > You can not prevent DOS on a machine if you allow a process to RO map
>> > your critical files (where you put futexes), because you allow this
>> > process to interfere with critical cache lines bouncing between cpus.
>>
>> The cacheline bounce DoS slows things down and they go back to normal
>> when you kill the DoS-ing task.
>>
>> The wakeup-eating DoS is permanent.  Seems a good deal worse to me.
>>
>> If you make this change, please at least document it in the man page.
>>
>
>
> This is how futexes had working for years.
>
> It was very obvious from the beginning. Please submit a man page change
> since you raised the point. You own the credit to open a CVE and
> immediately release a fix to all 2.6 versions !
>
> How come a critical fix (according to you) went without being noticed
> and documented ?
>

<cynical answer>Because Linux system calls aren't really documented.

It's reasonable to ask people to read a specification of how a system
call works to see if it's well-thought-out, usable, and has good
security properties.

It's also reasonable to ask people to read an implementation of a
system call and check to see that it conforms to the spec.

It's IMO a little less reasonable to ask people to review complicated
mm code to see if the interface it implements is well-designed.

The futex(2) and futex(7) manpages are very incomplete, even
today.</cynical answer>

>
> If its useful, then it needs a futex extension (and this must be
> emulated on old kernels without this extension)

I'm arguing for the extension.  I don't think the kernel has any
obligation to make sure that new use-cases are possible on old
versions, though.

--Andy

Re: Change in functionality of futex() system call.

[George Spelvin]

> You can not prevent DOS on a machine if you allow a process to RO map
> your critical files (where you put futexes), because you allow this
> process to interfere with critical cache lines bouncing between cpus.
> 
> Really, please forget about this crazy idea of allowing foreigners to
> _read_ or memory _map_ your files. Don't do it.

Hold on.  There's a significant difference between making a system
run slowly (there are a hundred ways to achieve that, starting with
the classic fork bomb), and a permanent hard lockup caused by losing
a FUTEX_WAIT.

Among other things, you can notice a DoS after a while and kill it.
Things should then return to normal.

There may well be a reason to share a file containing futexes read-only.

Just for example, consider a circular buffer that a few (trusted)
processes can write to, but many (less trusted) can read.  Obviously,
being able to sleep on the head pointer is useful.

We are already dealing with one problem caused by a too-narrow idea of
how futexes would be used.  Let's not add another "nobody would ever
want to do that!" unnecessarily.

Re: Change in functionality of futex() system call.

[Eric Dumazet]

Le jeudi 09 juin 2011 à 00:43 -0400, George Spelvin a écrit :

> Just for example, consider a circular buffer that a few (trusted)
> processes can write to, but many (less trusted) can read.  Obviously,
> being able to sleep on the head pointer is useful.


If its useful, then it needs a futex extension (and this must be
emulated on old kernels without this extension)

Remember futex_wake() call would just have to wakeup _all_ threads
instead of one, if kernel lacks this function.

If you dont trust futex_wait() users, just use futex_wake(ALL)

Its should not be a consequence of a previous (unrelated) patch, it
should be an added functionality, dully documented.

Re: Change in functionality of futex() system call.

[Kyle Moffett]

On Wed, Jun 8, 2011 at 23:54, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mercredi 08 juin 2011 à 23:38 -0400, Andrew Lutomirski a écrit :
>> Huh?
>>
>> I still don't understand why userspace ought to need to deny read
>> access to a file to prevent DoS.  I think it's entirely reasonable for
>> userspace to make the assumption that users with read access cannot
>> make changes visible to writers unless explicitly documented (i.e.
>> file locking, which is so thoroughly broken that it shouldn't be taken
>> as an example of how to design anything).
>>
>> Given that current kernels make this use safe and the proposal is to
>> make it unsafe, I think it's worth designing the interface to avoid
>> introducing new security problems.
>
> I am very tired of this discussion, you repeat the same arguments over
> and over.
>
> You can not prevent DOS on a machine if you allow a process to RO map
> your critical files (where you put futexes), because you allow this
> process to interfere with critical cache lines bouncing between cpus.
>
> Really, please forget about this crazy idea of allowing foreigners to
> _read_ or memory _map_ your files. Dont do it.

The issue is NOT that things get "slow".  There are lots of ways to do that
in an untrusted process on a normal Linux system.  Chewing CPU time
and reading random small files from all over the disk are the easiest ones,
and most Linux distributions usually ship lots of such files in directories
such as /usr/share/doc, /usr/share/zoneinfo, various locales directories, etc.

The issue is that this allows you to eat wakeups and make processes hang.

One relatively trivial example would be a database library like libdb or
similar.  The library could very reasonably use futexes to communicate
between multiple simultaneous threads writing to the same database
file.  Since the library wants to be well-behaved and avoid thundering
herd problems, it only issues a single wakeup for each lock release.

Now you have another program which uses the same database library
to do lockless queries of the of the DB file.  This is all well and good
except that it can now permanently hang an unlimited number of writer
threads in FUTEX_WAIT with trivial effort and virtually zero CPU.

All the attacker process needs to do is mmap() one page containing
one lock that the victim threads take occasionally and do this in a loop:
    int *victimfutex = [...];
    while(1)
        futex(victimfutex, *victimfutex, FUTEX_WAIT, NULL, NULL, 0);

Suddenly read-only access to *ANY* database file that happens to
use an in-file futex means that you can hang the database... period.
If you write it in ASM, you could even probably start a whole bunch
of threads in parallel by sharing the same stack.

Even better from a DoS standpoint, this does not trigger any resource
limits the way other attacks would, because you are sleeping 99.999%
of the time and are using no memory.  On top of that, once all of the
program's threads are stuck you can exit and it will just stay stuck.

This kind of thing is incredibly common in web-applications and other
similar environments, where "www-data" should be allowed to query
various file databases which are maintained by another daemon.

If the C library happens to use an in-file futex for arbitrating processes
writing to /var/log/utmp or /var/log/lastlog, it suddenly becomes trivial
to lock up every login process.

That is why FUTEX_WAIT needs separate handling for read-only files.

Cheers,
Kyle Moffett

Change in functionality of futex() system call.

[David Oliver]

Hello,

The functionality of the futex() system call appears to have changed
between versions 2.6.18 and 2.6.32.28.

Specifically, performing a FUTEX_WAIT on a read-only mapped location
results in an EFAULT. Although other operations, such as FUTEX_WAKE,
are only meaningful for writable locations, FUTEX_WAIT is useful for
processes with read-only access to a memory-mapped file.

The code below illustrates the changed behavior (each of the EXPECT
operations succeed on the older kernel, the ASSERTs pass in each
case), assuming the file /tmp/futex_test exists and contains int(42).

With the older kernel, the syscall() suspends until another process
changes the file and issues a FUTEX_WAKE, whereas the new behavior is
for an EFAULT error, independent of the file contents.

Let me know if you need further clarification.

Cheers!

David Oliver.


#include <errno.h>
#include <fcntl.h>
#include <stdint.h>
typedef uint32_t u32;   // for futex.h
#include <linux/futex.h>
#include <sys/mman.h>
#include <sys/syscall.h>
#include <unistd.h>
#include "gtest/gtest.h" // test framework to illustrate issue.


TEST(Futex, futex_in_read_only_file_is_ok) {
  int fd = open("/tmp/futex_test", O_RDONLY);
  ASSERT_GE(fd, 0);
  int* futex = static_cast<int *>(mmap(0, sizeof(int), PROT_READ,
MAP_SHARED, fd, 0));
  ASSERT_NE((int *)(0), futex);

  int rc = syscall(SYS_futex, futex, FUTEX_WAIT, 42, 0, 0, 0);

  EXPECT_NE(-1, rc);              // fails.
  if (rc == -1) {
      EXPECT_NE(errno, EFAULT);   // fails.
  }
}


---------------------------------------------------------------
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.

Re: Change in functionality of futex() system call.

[Eric Dumazet]

Le lundi 06 juin 2011 à 09:28 -0500, David Oliver a écrit :
> Hello,
> 
> The functionality of the futex() system call appears to have changed
> between versions 2.6.18 and 2.6.32.28.
> 
> Specifically, performing a FUTEX_WAIT on a read-only mapped location
> results in an EFAULT. Although other operations, such as FUTEX_WAKE,
> are only meaningful for writable locations, FUTEX_WAIT is useful for
> processes with read-only access to a memory-mapped file.
> 
> The code below illustrates the changed behavior (each of the EXPECT
> operations succeed on the older kernel, the ASSERTs pass in each
> case), assuming the file /tmp/futex_test exists and contains int(42).
> 
> With the older kernel, the syscall() suspends until another process
> changes the file and issues a FUTEX_WAKE, whereas the new behavior is
> for an EFAULT error, independent of the file contents.
> 
> Let me know if you need further clarification.
> 
> Cheers!
> 
> David Oliver.
> 
> 
> #include <errno.h>
> #include <fcntl.h>
> #include <stdint.h>
> typedef uint32_t u32;   // for futex.h
> #include <linux/futex.h>
> #include <sys/mman.h>
> #include <sys/syscall.h>
> #include <unistd.h>
> #include "gtest/gtest.h" // test framework to illustrate issue.
> 
> 
> TEST(Futex, futex_in_read_only_file_is_ok) {
>   int fd = open("/tmp/futex_test", O_RDONLY);
>   ASSERT_GE(fd, 0);
>   int* futex = static_cast<int *>(mmap(0, sizeof(int), PROT_READ,
> MAP_SHARED, fd, 0));
>   ASSERT_NE((int *)(0), futex);
> 
>   int rc = syscall(SYS_futex, futex, FUTEX_WAIT, 42, 0, 0, 0);
> 
>   EXPECT_NE(-1, rc);              // fails.
>   if (rc == -1) {
>       EXPECT_NE(errno, EFAULT);   // fails.
>   }
> }
> 

Right you are, this came from commit 7485d0d3758e8e6491a5 (futexes:
Remove rw parameter from get_futex_key()) in 2.6.33

commit 7485d0d3758e8e6491a5c9468114e74dc050785d
Author: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Date:   Tue Jan 5 16:32:43 2010 +0900

    futexes: Remove rw parameter from get_futex_key()
    
    Currently, futexes have two problem:
    
    A) The current futex code doesn't handle private file mappings properly.
    
    get_futex_key() uses PageAnon() to distinguish file and
    anon, which can cause the following bad scenario:
    
      1) thread-A call futex(private-mapping, FUTEX_WAIT), it
         sleeps on file mapping object.
      2) thread-B writes a variable and it makes it cow.
      3) thread-B calls futex(private-mapping, FUTEX_WAKE), it
         wakes up blocked thread on the anonymous page. (but it's nothing)
    
    B) 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.
    
    The solution is to use write mode get_user_page() always for
    page lookup. It prevents the lookup of both file page of private
    mappings and zero page.
    
    Performance concerns:
    
    Probaly very little, because glibc always initialize variables
    for futex before to call futex(). It means glibc users never see
    the overhead of this patch.
    
    Compatibility concerns:
    
    This patch has few compatibility issues. After this patch,
    FUTEX_WAIT require writable access to futex variables (read-only
    mappings makes EFAULT). But practically it's not a problem,
    glibc always initalizes variables for futexes explicitly - nobody
    uses read-only mappings.
    
    Reported-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
    Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
    Acked-by: Peter Zijlstra <peterz@infradead.org>
    Acked-by: Darren Hart <dvhltc@us.ibm.com>
    Cc: <stable@kernel.org>
    Cc: Linus Torvalds <torvalds@linux-foundation.org>
    Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
    Cc: Nick Piggin <npiggin@suse.de>
    Cc: Ulrich Drepper <drepper@gmail.com>
    LKML-Reference: <20100105162633.45A2.A69D9226@jp.fujitsu.com>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

Re: Change in functionality of futex() system call.

[Shawn Bohrer]

On Mon, Jun 06, 2011 at 05:23:39PM +0200, Eric Dumazet wrote:
> Le lundi 06 juin 2011 à 09:28 -0500, David Oliver a écrit :
> > Hello,
> > 
> > The functionality of the futex() system call appears to have changed
> > between versions 2.6.18 and 2.6.32.28.
> > 
> > Specifically, performing a FUTEX_WAIT on a read-only mapped location
> > results in an EFAULT. Although other operations, such as FUTEX_WAKE,
> > are only meaningful for writable locations, FUTEX_WAIT is useful for
> > processes with read-only access to a memory-mapped file.
> > 
> > The code below illustrates the changed behavior (each of the EXPECT
> > operations succeed on the older kernel, the ASSERTs pass in each
> > case), assuming the file /tmp/futex_test exists and contains int(42).
> > 
> > With the older kernel, the syscall() suspends until another process
> > changes the file and issues a FUTEX_WAKE, whereas the new behavior is
> > for an EFAULT error, independent of the file contents.
> > 
> > Let me know if you need further clarification.
> > 
> > Cheers!
> > 
> > David Oliver.
> > 
> > 
> > #include <errno.h>
> > #include <fcntl.h>
> > #include <stdint.h>
> > typedef uint32_t u32;   // for futex.h
> > #include <linux/futex.h>
> > #include <sys/mman.h>
> > #include <sys/syscall.h>
> > #include <unistd.h>
> > #include "gtest/gtest.h" // test framework to illustrate issue.
> > 
> > 
> > TEST(Futex, futex_in_read_only_file_is_ok) {
> >   int fd = open("/tmp/futex_test", O_RDONLY);
> >   ASSERT_GE(fd, 0);
> >   int* futex = static_cast<int *>(mmap(0, sizeof(int), PROT_READ,
> > MAP_SHARED, fd, 0));
> >   ASSERT_NE((int *)(0), futex);
> > 
> >   int rc = syscall(SYS_futex, futex, FUTEX_WAIT, 42, 0, 0, 0);
> > 
> >   EXPECT_NE(-1, rc);              // fails.
> >   if (rc == -1) {
> >       EXPECT_NE(errno, EFAULT);   // fails.
> >   }
> > }
> > 
> 
> Right you are, this came from commit 7485d0d3758e8e6491a5 (futexes:
> Remove rw parameter from get_futex_key()) in 2.6.33
> 
> commit 7485d0d3758e8e6491a5c9468114e74dc050785d
> Author: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Date:   Tue Jan 5 16:32:43 2010 +0900
> 
>     futexes: Remove rw parameter from get_futex_key()
>     
>     Currently, futexes have two problem:
>     
>     A) The current futex code doesn't handle private file mappings properly.
>     
>     get_futex_key() uses PageAnon() to distinguish file and
>     anon, which can cause the following bad scenario:
>     
>       1) thread-A call futex(private-mapping, FUTEX_WAIT), it
>          sleeps on file mapping object.
>       2) thread-B writes a variable and it makes it cow.
>       3) thread-B calls futex(private-mapping, FUTEX_WAKE), it
>          wakes up blocked thread on the anonymous page. (but it's nothing)
>     
>     B) 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.
>     
>     The solution is to use write mode get_user_page() always for
>     page lookup. It prevents the lookup of both file page of private
>     mappings and zero page.
>     
>     Performance concerns:
>     
>     Probaly very little, because glibc always initialize variables
>     for futex before to call futex(). It means glibc users never see
>     the overhead of this patch.
>     
>     Compatibility concerns:
>     
>     This patch has few compatibility issues. After this patch,
>     FUTEX_WAIT require writable access to futex variables (read-only
>     mappings makes EFAULT). But practically it's not a problem,
>     glibc always initalizes variables for futexes explicitly - nobody
>     uses read-only mappings.

We use read-only mappings.  In our case we have Process A writing to
a memory-mapped file, and Processes B, C, D, etc reading that memory
mapped file.  Using a futex in the file header is a convenient way to
notify the readers of updates.

glibc is not the only user of futexes.

--
Shawn


---------------------------------------------------------------
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.

Re: Change in functionality of futex() system call.

[Peter Zijlstra]

On Mon, 2011-06-06 at 17:23 +0200, Eric Dumazet wrote:
> Le lundi 06 juin 2011 à 09:28 -0500, David Oliver a écrit :
> > Hello,
> > 
> > The functionality of the futex() system call appears to have changed
> > between versions 2.6.18 and 2.6.32.28.
> > 
> > Specifically, performing a FUTEX_WAIT on a read-only mapped location
> > results in an EFAULT. Although other operations, such as FUTEX_WAKE,
> > are only meaningful for writable locations, FUTEX_WAIT is useful for
> > processes with read-only access to a memory-mapped file.
> > 
> > The code below illustrates the changed behavior (each of the EXPECT
> > operations succeed on the older kernel, the ASSERTs pass in each
> > case), assuming the file /tmp/futex_test exists and contains int(42).
> > 
> > With the older kernel, the syscall() suspends until another process
> > changes the file and issues a FUTEX_WAKE, whereas the new behavior is
> > for an EFAULT error, independent of the file contents.
> > 
> > Let me know if you need further clarification.
> > 
> > Cheers!
> > 
> > David Oliver.
> > 
> > 
> > #include <errno.h>
> > #include <fcntl.h>
> > #include <stdint.h>
> > typedef uint32_t u32;   // for futex.h
> > #include <linux/futex.h>
> > #include <sys/mman.h>
> > #include <sys/syscall.h>
> > #include <unistd.h>
> > #include "gtest/gtest.h" // test framework to illustrate issue.
> > 
> > 
> > TEST(Futex, futex_in_read_only_file_is_ok) {
> >   int fd = open("/tmp/futex_test", O_RDONLY);
> >   ASSERT_GE(fd, 0);
> >   int* futex = static_cast<int *>(mmap(0, sizeof(int), PROT_READ,
> > MAP_SHARED, fd, 0));
> >   ASSERT_NE((int *)(0), futex);
> > 
> >   int rc = syscall(SYS_futex, futex, FUTEX_WAIT, 42, 0, 0, 0);
> > 
> >   EXPECT_NE(-1, rc);              // fails.
> >   if (rc == -1) {
> >       EXPECT_NE(errno, EFAULT);   // fails.
> >   }
> > }
> > 
> 
> Right you are, this came from commit 7485d0d3758e8e6491a5 (futexes:
> Remove rw parameter from get_futex_key()) in 2.6.33
> 
> commit 7485d0d3758e8e6491a5c9468114e74dc050785d
> Author: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Date:   Tue Jan 5 16:32:43 2010 +0900
> 
>     futexes: Remove rw parameter from get_futex_key()
>     
>     Currently, futexes have two problem:
>     
>     A) The current futex code doesn't handle private file mappings properly.
>     
>     get_futex_key() uses PageAnon() to distinguish file and
>     anon, which can cause the following bad scenario:
>     
>       1) thread-A call futex(private-mapping, FUTEX_WAIT), it
>          sleeps on file mapping object.
>       2) thread-B writes a variable and it makes it cow.
>       3) thread-B calls futex(private-mapping, FUTEX_WAKE), it
>          wakes up blocked thread on the anonymous page. (but it's nothing)
>     
>     B) 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.
>     
>     The solution is to use write mode get_user_page() always for
>     page lookup. It prevents the lookup of both file page of private
>     mappings and zero page.
>     
>     Performance concerns:
>     
>     Probaly very little, because glibc always initialize variables
>     for futex before to call futex(). It means glibc users never see
>     the overhead of this patch.
>     
>     Compatibility concerns:
>     
>     This patch has few compatibility issues. After this patch,
>     FUTEX_WAIT require writable access to futex variables (read-only
>     mappings makes EFAULT). But practically it's not a problem,
>     glibc always initalizes variables for futexes explicitly - nobody
>     uses read-only mappings.

Urgh,. maybe something like the below but with more conditionals that
enable the extra logic only for FUTEX_WAIT..

The idea is to try a RO gup() when the RW gup() fails so as not to slow
down the common path of writable anonymous maps and bail when we used
the RO path on anonymous memory.

---
diff --git a/kernel/futex.c b/kernel/futex.c
index fe28dc2..11f2ad1 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -234,7 +234,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key)
 	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 +262,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) {
+		err = get_user_pages_fast(address, 1, 0, &page);
+		ro = 1;
+	}
 	if (err < 0)
 		return err;
 
@@ -316,6 +320,11 @@ again:
 	 * the object not the particular process.
 	 */
 	if (PageAnon(page_head)) {
+		if (ro) {
+			err = -EFAULT;
+			goto out;
+		}
+
 		key->both.offset |= FUT_OFF_MMSHARED; /* ref taken on mm */
 		key->private.mm = mm;
 		key->private.address = address;
@@ -327,9 +336,10 @@ again:
 
 	get_futex_key_refs(key);
 
+out:
 	unlock_page(page_head);
 	put_page(page_head);
-	return 0;
+	return err;
 }
 
 static inline void put_futex_key(union futex_key *key)

Re: Change in functionality of futex() system call.

[Peter Zijlstra]

On Mon, 2011-06-06 at 18:11 +0200, Peter Zijlstra wrote:
> On Mon, 2011-06-06 at 17:23 +0200, Eric Dumazet wrote:
> > Le lundi 06 juin 2011 à 09:28 -0500, David Oliver a écrit :
> > > Hello,
> > > 
> > > The functionality of the futex() system call appears to have changed
> > > between versions 2.6.18 and 2.6.32.28.
> > > 
> > > Specifically, performing a FUTEX_WAIT on a read-only mapped location
> > > results in an EFAULT. Although other operations, such as FUTEX_WAKE,
> > > are only meaningful for writable locations, FUTEX_WAIT is useful for
> > > processes with read-only access to a memory-mapped file.
> > > 
> > > The code below illustrates the changed behavior (each of the EXPECT
> > > operations succeed on the older kernel, the ASSERTs pass in each
> > > case), assuming the file /tmp/futex_test exists and contains int(42).
> > > 
> > > With the older kernel, the syscall() suspends until another process
> > > changes the file and issues a FUTEX_WAKE, whereas the new behavior is
> > > for an EFAULT error, independent of the file contents.
> > > 
> > > Let me know if you need further clarification.
> > > 
> > > Cheers!
> > > 
> > > David Oliver.
> > > 
> > > 
> > > #include <errno.h>
> > > #include <fcntl.h>
> > > #include <stdint.h>
> > > typedef uint32_t u32;   // for futex.h
> > > #include <linux/futex.h>
> > > #include <sys/mman.h>
> > > #include <sys/syscall.h>
> > > #include <unistd.h>
> > > #include "gtest/gtest.h" // test framework to illustrate issue.
> > > 
> > > 
> > > TEST(Futex, futex_in_read_only_file_is_ok) {
> > >   int fd = open("/tmp/futex_test", O_RDONLY);
> > >   ASSERT_GE(fd, 0);
> > >   int* futex = static_cast<int *>(mmap(0, sizeof(int), PROT_READ,
> > > MAP_SHARED, fd, 0));
> > >   ASSERT_NE((int *)(0), futex);
> > > 
> > >   int rc = syscall(SYS_futex, futex, FUTEX_WAIT, 42, 0, 0, 0);
> > > 
> > >   EXPECT_NE(-1, rc);              // fails.
> > >   if (rc == -1) {
> > >       EXPECT_NE(errno, EFAULT);   // fails.
> > >   }
> > > }
> > > 
> > 
> > Right you are, this came from commit 7485d0d3758e8e6491a5 (futexes:
> > Remove rw parameter from get_futex_key()) in 2.6.33
> > 
> > commit 7485d0d3758e8e6491a5c9468114e74dc050785d
> > Author: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > Date:   Tue Jan 5 16:32:43 2010 +0900
> > 
> >     futexes: Remove rw parameter from get_futex_key()
> >     
> >     Currently, futexes have two problem:
> >     
> >     A) The current futex code doesn't handle private file mappings properly.
> >     
> >     get_futex_key() uses PageAnon() to distinguish file and
> >     anon, which can cause the following bad scenario:
> >     
> >       1) thread-A call futex(private-mapping, FUTEX_WAIT), it
> >          sleeps on file mapping object.
> >       2) thread-B writes a variable and it makes it cow.
> >       3) thread-B calls futex(private-mapping, FUTEX_WAKE), it
> >          wakes up blocked thread on the anonymous page. (but it's nothing)
> >     
> >     B) 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.
> >     
> >     The solution is to use write mode get_user_page() always for
> >     page lookup. It prevents the lookup of both file page of private
> >     mappings and zero page.
> >     
> >     Performance concerns:
> >     
> >     Probaly very little, because glibc always initialize variables
> >     for futex before to call futex(). It means glibc users never see
> >     the overhead of this patch.
> >     
> >     Compatibility concerns:
> >     
> >     This patch has few compatibility issues. After this patch,
> >     FUTEX_WAIT require writable access to futex variables (read-only
> >     mappings makes EFAULT). But practically it's not a problem,
> >     glibc always initalizes variables for futexes explicitly - nobody
> >     uses read-only mappings.
> 
> Urgh,. maybe something like the below but with more conditionals that
> enable the extra logic only for FUTEX_WAIT..
> 
> The idea is to try a RO gup() when the RW gup() fails so as not to slow
> down the common path of writable anonymous maps and bail when we used
> the RO path on anonymous memory.
> 
> ---
> diff --git a/kernel/futex.c b/kernel/futex.c
> index fe28dc2..11f2ad1 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -234,7 +234,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key)
>  	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 +262,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) {
> +		err = get_user_pages_fast(address, 1, 0, &page);
> +		ro = 1;
> +	}
>  	if (err < 0)
>  		return err;
>  
> @@ -316,6 +320,11 @@ again:
>  	 * the object not the particular process.
>  	 */
>  	if (PageAnon(page_head)) {
> +		if (ro) {
> +			err = -EFAULT;
> +			goto out;
> +		}
> +
>  		key->both.offset |= FUT_OFF_MMSHARED; /* ref taken on mm */
>  		key->private.mm = mm;
>  		key->private.address = address;
> @@ -327,9 +336,10 @@ again:
>  
>  	get_futex_key_refs(key);
>  
> +out:
>  	unlock_page(page_head);
>  	put_page(page_head);
> -	return 0;
> +	return err;
>  }
>  
>  static inline void put_futex_key(union futex_key *key)
> 

Hmm, wouldn't that still be susceptible to the zero-page thing if: we
create a writable private file map of a sparse file, touch a page and
then remap the thing RO?

Re: Change in functionality of futex() system call.

[Eric Dumazet]

Le lundi 06 juin 2011 à 18:16 +0200, Peter Zijlstra a écrit :

> Hmm, wouldn't that still be susceptible to the zero-page thing if: we
> create a writable private file map of a sparse file, touch a page and
> then remap the thing RO?
> 
> 
> 

Also I am not sure how MAP_PRIVATE could be affected. If we still try a
RW gup()... It will allocate a page for us, instead of still pointing to
shared one.

On previous kernel, the application using read-only mapping could use
MAP_PRIVATE or MAP_SHARED with same 'behavior'

Re: Change in functionality of futex() system call.

[Peter Zijlstra]

On Mon, 2011-06-06 at 18:22 +0200, Eric Dumazet wrote:
> Le lundi 06 juin 2011 à 18:16 +0200, Peter Zijlstra a écrit :
> 
> > Hmm, wouldn't that still be susceptible to the zero-page thing if: we
> > create a writable private file map of a sparse file, touch a page and
> > then remap the thing RO?
> > 
> > 
> > 
> 
> Also I am not sure how MAP_PRIVATE could be affected. If we still try a
> RW gup()... It will allocate a page for us, instead of still pointing to
> shared one.
> 
> On previous kernel, the application using read-only mapping could use
> MAP_PRIVATE or MAP_SHARED with same 'behavior'

But by not forcing the COW you get different behaviour depending on when
you call FUTEX_WAIT, surely that's not correct either?

Re: Change in functionality of futex() system call.

[Eric Dumazet]

Le lundi 06 juin 2011 à 18:29 +0200, Peter Zijlstra a écrit :
> On Mon, 2011-06-06 at 18:22 +0200, Eric Dumazet wrote:
> > Le lundi 06 juin 2011 à 18:16 +0200, Peter Zijlstra a écrit :
> > 
> > > Hmm, wouldn't that still be susceptible to the zero-page thing if: we
> > > create a writable private file map of a sparse file, touch a page and
> > > then remap the thing RO?
> > > 
> > > 
> > > 
> > 
> > Also I am not sure how MAP_PRIVATE could be affected. If we still try a
> > RW gup()... It will allocate a page for us, instead of still pointing to
> > shared one.
> > 
> > On previous kernel, the application using read-only mapping could use
> > MAP_PRIVATE or MAP_SHARED with same 'behavior'
> 
> But by not forcing the COW you get different behaviour depending on when
> you call FUTEX_WAIT, surely that's not correct either?


As long as the current process never writes to the page holding the
futex, the page stay shared. Behavior should be same with PRIVATE or
SHARED ?

In David Oliver case, this is needed : He wants to catch a change in a
file/memory region written by another process.

Re: Change in functionality of futex() system call.

[Peter Zijlstra]

On Mon, 2011-06-06 at 18:42 +0200, Eric Dumazet wrote:
> Le lundi 06 juin 2011 à 18:29 +0200, Peter Zijlstra a écrit :
> > On Mon, 2011-06-06 at 18:22 +0200, Eric Dumazet wrote:
> > > Le lundi 06 juin 2011 à 18:16 +0200, Peter Zijlstra a écrit :
> > > 
> > > > Hmm, wouldn't that still be susceptible to the zero-page thing if: we
> > > > create a writable private file map of a sparse file, touch a page and
> > > > then remap the thing RO?
> > > > 
> > > > 
> > > > 
> > > 
> > > Also I am not sure how MAP_PRIVATE could be affected. If we still try a
> > > RW gup()... It will allocate a page for us, instead of still pointing to
> > > shared one.
> > > 
> > > On previous kernel, the application using read-only mapping could use
> > > MAP_PRIVATE or MAP_SHARED with same 'behavior'
> > 
> > But by not forcing the COW you get different behaviour depending on when
> > you call FUTEX_WAIT, surely that's not correct either?
> 
> 
> As long as the current process never writes to the page holding the
> futex, the page stay shared. Behavior should be same with PRIVATE or
> SHARED ?

Dunno, using futexes on private file maps is stupid imo, its just asking
for trouble, ro private file maps are even worse. Forcing the COW is the
only sane answer in that it gives consistent results and 'breaks' silly
expectations early instead of sometimes.

Anyway, that's not really the issue here, as David uses MAP_SHARED (as
one should if one is interested in the shared value).

Re: Change in functionality of futex() system call.

[Eric Dumazet]

Le lundi 06 juin 2011 à 19:05 +0200, Peter Zijlstra a écrit :

> Dunno, using futexes on private file maps is stupid imo, its just asking
> for trouble, ro private file maps are even worse. Forcing the COW is the
> only sane answer in that it gives consistent results and 'breaks' silly
> expectations early instead of sometimes.
> 
> Anyway, that's not really the issue here, as David uses MAP_SHARED (as
> one should if one is interested in the shared value).

Sure, but maybe another guy is 'stupid' and uses MAP_PRIVATE on its
read-only mappings. With old kernels this was working, and we were not
doing the COW.

(Note : the other process writes to the file, using MAP_SHARED)

Re: Change in functionality of futex() system call.

[Steven Rostedt]

On Mon, Jun 06, 2011 at 07:11:37PM +0200, Eric Dumazet wrote:
> Le lundi 06 juin 2011 à 19:05 +0200, Peter Zijlstra a écrit :
> 
> > Dunno, using futexes on private file maps is stupid imo, its just asking
> > for trouble, ro private file maps are even worse. Forcing the COW is the
> > only sane answer in that it gives consistent results and 'breaks' silly
> > expectations early instead of sometimes.
> > 
> > Anyway, that's not really the issue here, as David uses MAP_SHARED (as
> > one should if one is interested in the shared value).
> 
> Sure, but maybe another guy is 'stupid' and uses MAP_PRIVATE on its
> read-only mappings. With old kernels this was working, and we were not
> doing the COW.

That sounds like a bug in both the kernel and userspace. I would expect
a MAP_PRIVATE not be seen by any other process regardless. That's the
definition of PRIVATE.

 From: http://www.gnu.org/s/hello/manual/libc/Memory_002dmapped-I_002fO.html

MAP_PRIVATE
    This specifies that writes to the region should never be written back
    to the attached file. Instead, a copy is made for the process, and the
    region will be swapped normally if memory runs low. No other process
    will see the changes. 

-- Steve

Re: Change in functionality of futex() system call.

[Darren Hart]

On 06/06/2011 10:27 AM, Steven Rostedt wrote:
> On Mon, Jun 06, 2011 at 07:11:37PM +0200, Eric Dumazet wrote:
>> Le lundi 06 juin 2011 à 19:05 +0200, Peter Zijlstra a écrit :
>>
>>> Dunno, using futexes on private file maps is stupid imo, its just asking
>>> for trouble, ro private file maps are even worse. Forcing the COW is the
>>> only sane answer in that it gives consistent results and 'breaks' silly
>>> expectations early instead of sometimes.
>>>
>>> Anyway, that's not really the issue here, as David uses MAP_SHARED (as
>>> one should if one is interested in the shared value).
>>
>> Sure, but maybe another guy is 'stupid' and uses MAP_PRIVATE on its
>> read-only mappings. With old kernels this was working, and we were not
>> doing the COW.
> 
> That sounds like a bug in both the kernel and userspace. I would expect
> a MAP_PRIVATE not be seen by any other process regardless. That's the
> definition of PRIVATE.
> 
>  From: http://www.gnu.org/s/hello/manual/libc/Memory_002dmapped-I_002fO.html
> 
> MAP_PRIVATE
>     This specifies that writes to the region should never be written back
>     to the attached file. Instead, a copy is made for the process, and the
>     region will be swapped normally if memory runs low. No other process
>     will see the changes. 


This doesn't address what happens if changes to a MAP_SHARED mapping are
visible to a MAP_PRIVATE mapping, which is more the issue at hand I believe.

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

Re: Change in functionality of futex() system call.

[Peter Zijlstra]

On Mon, 2011-06-06 at 19:11 +0200, Eric Dumazet wrote:
> Le lundi 06 juin 2011 à 19:05 +0200, Peter Zijlstra a écrit :
> 
> > Dunno, using futexes on private file maps is stupid imo, its just asking
> > for trouble, ro private file maps are even worse. Forcing the COW is the
> > only sane answer in that it gives consistent results and 'breaks' silly
> > expectations early instead of sometimes.
> > 
> > Anyway, that's not really the issue here, as David uses MAP_SHARED (as
> > one should if one is interested in the shared value).
> 
> Sure, but maybe another guy is 'stupid' and uses MAP_PRIVATE on its
> read-only mappings. With old kernels this was working, and we were not
> doing the COW.
> 
> (Note : the other process writes to the file, using MAP_SHARED)

That's really not the point, what do we do when the COW happens during
the FUTEX_WAIT? At that point the process vaddr changes mapping and we
cannot continue the wait on the old page, since that would expose
invisible information, nor can we switch to the new page since we queued
on the old page.

Therefore we have to force the COW and queue on the private copy, it
really is the only semi sane semantic.

Re: Change in functionality of futex() system call.

[Eric Dumazet]

Le lundi 06 juin 2011 à 20:23 +0200, Peter Zijlstra a écrit :

> 
> That's really not the point, what do we do when the COW happens during
> the FUTEX_WAIT? At that point the process vaddr changes mapping and we
> cannot continue the wait on the old page, since that would expose
> invisible information, nor can we switch to the new page since we queued
> on the old page.
> 
> Therefore we have to force the COW and queue on the private copy, it
> really is the only semi sane semantic.

The point is we dont necessarly have to COW the page. If you attempt
this COW, you shoot on user that did not expect to have a COW.

Take this program : COW is not allowed, still this worked on 2.6.18 (it
waits until another process change the value in file and call
futex_wait())

Using PROT_READ | PROT_WRITE instead of PROT_READ was OK too.

(If we use PROT_READ | PROT_WRITE, then after your patch, program doesnt
work anymore since this process gets a private page after your hidden
COW : It'll wait forever)

#include <errno.h>
#include <fcntl.h>
#include <stdint.h>
typedef uint32_t u32;   // for futex.h
#include <linux/futex.h>
#include <sys/mman.h>
#include <sys/syscall.h>
#include <unistd.h>


int main(int argc, char *argv[]) {
 	int fd, *futex, rc, val = 42;

	fd = open("/tmp/futex_test", O_RDWR|O_CREAT, 0644);
	write(fd, &val, 4);
	futex = (int *)mmap(0, sizeof(int), PROT_READ, MAP_PRIVATE, fd, 0);
	rc = syscall(SYS_futex, futex, FUTEX_WAIT, val, 0, 0, 0);
	printf("rc=%d errno=%d\n", rc, errno);
}

Re: Change in functionality of futex() system call.

[Darren Hart]

Hi Eric,

I'm finally getting time to review this in depth and try to help Shawn
get his fix upstream. Trying to make sure I have all the facets of this
straight in my head... or on paper at least ;-)

On 06/06/2011 11:27 AM, Eric Dumazet wrote:
> Le lundi 06 juin 2011 à 20:23 +0200, Peter Zijlstra a écrit :
> 
>>
>> That's really not the point, what do we do when the COW happens during
>> the FUTEX_WAIT? At that point the process vaddr changes mapping and we
>> cannot continue the wait on the old page, since that would expose
>> invisible information, nor can we switch to the new page since we queued
>> on the old page.
>>
>> Therefore we have to force the COW and queue on the private copy, it
>> really is the only semi sane semantic.
> 
> The point is we dont necessarly have to COW the page. If you attempt
> this COW, you shoot on user that did not expect to have a COW.
> 
> Take this program : COW is not allowed, still this worked on 2.6.18 (it
> waits until another process change the value in file and call
> futex_wait())
> 
> Using PROT_READ | PROT_WRITE instead of PROT_READ was OK too.
> 
> (If we use PROT_READ | PROT_WRITE, then after your patch, program doesnt
> work anymore since this process gets a private page after your hidden
> COW : It'll wait forever)


As I understand MMAP(2), this is working due to undefined behavior as
Stephen pointed out earlier:

"It is unspecified whether changes made to the file after the     mmap()
call are visible in the mapped region."

I don't think we are under any obligation to keep that working.

--
Darren

> 
> #include <errno.h>
> #include <fcntl.h>
> #include <stdint.h>
> typedef uint32_t u32;   // for futex.h
> #include <linux/futex.h>
> #include <sys/mman.h>
> #include <sys/syscall.h>
> #include <unistd.h>
> 
> 
> int main(int argc, char *argv[]) {
>  	int fd, *futex, rc, val = 42;
> 
> 	fd = open("/tmp/futex_test", O_RDWR|O_CREAT, 0644);
> 	write(fd, &val, 4);
> 	futex = (int *)mmap(0, sizeof(int), PROT_READ, MAP_PRIVATE, fd, 0);
> 	rc = syscall(SYS_futex, futex, FUTEX_WAIT, val, 0, 0, 0);
> 	printf("rc=%d errno=%d\n", rc, errno);
> }
> 
> 
> 
> --
> 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

Re: Change in functionality of futex() system call.

[Shawn Bohrer]

Hey Eric,

I have one question below.

On Mon, Jun 06, 2011 at 08:27:51PM +0200, Eric Dumazet wrote:
> Le lundi 06 juin 2011 à 20:23 +0200, Peter Zijlstra a écrit :
> 
> > 
> > That's really not the point, what do we do when the COW happens during
> > the FUTEX_WAIT? At that point the process vaddr changes mapping and we
> > cannot continue the wait on the old page, since that would expose
> > invisible information, nor can we switch to the new page since we queued
> > on the old page.
> > 
> > Therefore we have to force the COW and queue on the private copy, it
> > really is the only semi sane semantic.
> 
> The point is we dont necessarly have to COW the page. If you attempt
> this COW, you shoot on user that did not expect to have a COW.
> 
> Take this program : COW is not allowed, still this worked on 2.6.18 (it
> waits until another process change the value in file and call
> futex_wait())
> 
> Using PROT_READ | PROT_WRITE instead of PROT_READ was OK too.

Did this actually work on 2.6.18?  I'm testing on RHEL 5 and both
PROT_READ, and PROT_READ|PROT_WRITE with MAP_PRIVATE wait forever.
I'm not sure if that is from Red Hat backports or not.

Peter's change on top of 3.0 actually make the PROT_READ MAP_PRIVATE
case work, those as you point out below PROT_READ|PROT_WRITE waits
forever.

> (If we use PROT_READ | PROT_WRITE, then after your patch, program doesnt
> work anymore since this process gets a private page after your hidden
> COW : It'll wait forever)
> 
> #include <errno.h>
> #include <fcntl.h>
> #include <stdint.h>
> typedef uint32_t u32;   // for futex.h
> #include <linux/futex.h>
> #include <sys/mman.h>
> #include <sys/syscall.h>
> #include <unistd.h>
> 
> 
> int main(int argc, char *argv[]) {
>  	int fd, *futex, rc, val = 42;
> 
> 	fd = open("/tmp/futex_test", O_RDWR|O_CREAT, 0644);
> 	write(fd, &val, 4);
> 	futex = (int *)mmap(0, sizeof(int), PROT_READ, MAP_PRIVATE, fd, 0);
> 	rc = syscall(SYS_futex, futex, FUTEX_WAIT, val, 0, 0, 0);
> 	printf("rc=%d errno=%d\n", rc, errno);
> }
> 
> 
> 


---------------------------------------------------------------
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.

Re: Change in functionality of futex() system call.

[Darren Hart]

On 06/06/2011 09:42 AM, Eric Dumazet wrote:
> Le lundi 06 juin 2011 à 18:29 +0200, Peter Zijlstra a écrit :
>> On Mon, 2011-06-06 at 18:22 +0200, Eric Dumazet wrote:
>>> Le lundi 06 juin 2011 à 18:16 +0200, Peter Zijlstra a écrit :
>>>
>>>> Hmm, wouldn't that still be susceptible to the zero-page thing if: we
>>>> create a writable private file map of a sparse file, touch a page and
>>>> then remap the thing RO?
>>>>
>>>>
>>>>
>>>
>>> Also I am not sure how MAP_PRIVATE could be affected. If we still try a
>>> RW gup()... It will allocate a page for us, instead of still pointing to
>>> shared one.
>>>
>>> On previous kernel, the application using read-only mapping could use
>>> MAP_PRIVATE or MAP_SHARED with same 'behavior'
>>
>> But by not forcing the COW you get different behaviour depending on when
>> you call FUTEX_WAIT, surely that's not correct either?
> 
> 
> As long as the current process never writes to the page holding the
> futex, the page stay shared. Behavior should be same with PRIVATE or
> SHARED ?

If I understand the problem correctly, RO private mapping really doesn't
make any sense and we should probably explicitly not support it, while
special casing the RO shared mapping in support of David's scenario.

> 
> In David Oliver case, this is needed : He wants to catch a change in a
> file/memory region written by another process.

But with shared mapping and shared futexes. He just needs the ability to
FUTEX_WAIT on a RO mapping. Or is that what you were saying?

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

Re: Change in functionality of futex() system call.

[Eric Dumazet]

Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
> 

> If I understand the problem correctly, RO private mapping really doesn't
> make any sense and we should probably explicitly not support it, while
> special casing the RO shared mapping in support of David's scenario.
> 

We supported them in 2.6.18 kernels, apparently. This might sounds
stupid but who knows ?

> > 
> > In David Oliver case, this is needed : He wants to catch a change in a
> > file/memory region written by another process.
> 
> But with shared mapping and shared futexes. He just needs the ability to
> FUTEX_WAIT on a RO mapping. Or is that what you were saying?
> 

I am saying that in David Oliver case, he sure uses a MAP_SHARED ro
mapping.

Now, what if other software uses a MAP_PRIVATE ro mapping ?

It was working in previous kernels as well.

We can say its stupid, but IMHO its not.

In other words, this program should work, if process never touches
(writes) into first page.

This program on previous kernels gave :
rc=-1 errno=11
(allowing to wait for a value change and a futex_WAKE)

With new kernel :
rc=-1 errno=14  [ no sleep allowed ]

#include <errno.h>
#include <fcntl.h>
#include <stdint.h>
typedef uint32_t u32;   // for futex.h
#include <linux/futex.h>
#include <sys/mman.h>
#include <sys/syscall.h>
#include <unistd.h>


int main() {
 	int fd, *futex, rc;

	fd = open("/tmp/futex_test", O_RDWR|O_CREAT, 0644);
	write(fd, "\1\1\1\1", 4);
	futex = (int *)mmap(0, sizeof(int), PROT_READ, MAP_PRIVATE, fd, 0);
	rc = syscall(SYS_futex, futex, FUTEX_WAIT, 42, 0, 0, 0);
	printf("rc=%d errno=%d\n", rc, errno);
}

Re: Change in functionality of futex() system call.

[Darren Hart]

On 06/06/2011 11:11 AM, Eric Dumazet wrote:
> Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
>>
> 
>> If I understand the problem correctly, RO private mapping really doesn't
>> make any sense and we should probably explicitly not support it, while
>> special casing the RO shared mapping in support of David's scenario.
>>
> 
> We supported them in 2.6.18 kernels, apparently. This might sounds
> stupid but who knows ?


I guess this is actually the key point we need to agree on to provide a
solution. This particular case "worked" in 2.6.18 kernels, but that
doesn't necessarily mean it was supported, or even intentional.

It sounds to me that we agree that we should support RO shared mappings.
The question remains about whether we should introduce deliberate
support of RO private mappings, and if so, if the forced COW approach is
appropriate or not.

Does anyone with a longer history working with futexes than I have an
opinion on this? Is support for RO private mappings part of our futex
API, or was it an unintentional side effect of the futex simply being a
userspace address.

--
Darren


> 
>>>
>>> In David Oliver case, this is needed : He wants to catch a change in a
>>> file/memory region written by another process.
>>
>> But with shared mapping and shared futexes. He just needs the ability to
>> FUTEX_WAIT on a RO mapping. Or is that what you were saying?
>>
> 
> I am saying that in David Oliver case, he sure uses a MAP_SHARED ro
> mapping.
> 
> Now, what if other software uses a MAP_PRIVATE ro mapping ?
> 
> It was working in previous kernels as well.
> 
> We can say its stupid, but IMHO its not.
> 
> In other words, this program should work, if process never touches
> (writes) into first page.
> 
> This program on previous kernels gave :
> rc=-1 errno=11
> (allowing to wait for a value change and a futex_WAKE)
> 
> With new kernel :
> rc=-1 errno=14  [ no sleep allowed ]
> 
> #include <errno.h>
> #include <fcntl.h>
> #include <stdint.h>
> typedef uint32_t u32;   // for futex.h
> #include <linux/futex.h>
> #include <sys/mman.h>
> #include <sys/syscall.h>
> #include <unistd.h>
> 
> 
> int main() {
>  	int fd, *futex, rc;
> 
> 	fd = open("/tmp/futex_test", O_RDWR|O_CREAT, 0644);
> 	write(fd, "\1\1\1\1", 4);
> 	futex = (int *)mmap(0, sizeof(int), PROT_READ, MAP_PRIVATE, fd, 0);
> 	rc = syscall(SYS_futex, futex, FUTEX_WAIT, 42, 0, 0, 0);
> 	printf("rc=%d errno=%d\n", rc, errno);
> }
> 
> 
> --
> 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

Re: Change in functionality of futex() system call.

[Eric Dumazet]

Le lundi 06 juin 2011 à 20:13 -0700, Darren Hart a écrit :
> 
> On 06/06/2011 11:11 AM, Eric Dumazet wrote:
> > Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
> >>
> > 
> >> If I understand the problem correctly, RO private mapping really doesn't
> >> make any sense and we should probably explicitly not support it, while
> >> special casing the RO shared mapping in support of David's scenario.
> >>
> > 
> > We supported them in 2.6.18 kernels, apparently. This might sounds
> > stupid but who knows ?
> 
> 
> I guess this is actually the key point we need to agree on to provide a
> solution. This particular case "worked" in 2.6.18 kernels, but that
> doesn't necessarily mean it was supported, or even intentional.
> 
> It sounds to me that we agree that we should support RO shared mappings.
> The question remains about whether we should introduce deliberate
> support of RO private mappings, and if so, if the forced COW approach is
> appropriate or not.
> 
> Does anyone with a longer history working with futexes than I have an
> opinion on this? Is support for RO private mappings part of our futex
> API, or was it an unintentional side effect of the futex simply being a
> userspace address.
> 

I personnally dont care as I dont use ro mappings for my futexes land,
but I can feel the pain of people discovering yet another
incompatibility in their user apps after a kernel upgrade, spending so
much time to find the root of the problem (hey, not everybody is a
kernel hacker)

If we think about it, futex_wait() should not touch memory, only read
it. Some smart layer could be upset by this (valgrind ?)

Its like saying write(int fd, const void *buffer, size_t count) could
try to do a COW on buffer, because it makes kernel programmer life more
comfortable, this makes litle sense to me IMHO.

Part of the problem comes from futex() syscall being a multiplexor.
What a mess.

If we had a clean API at the beginning, then we would have :

int sys_futex_wait(const void *futex, int val, const struct timespec *t);

And really, doing COW in futex_wait() would clearly be wrong.

Re: Change in functionality of futex() system call.

[Andy Lutomirski]

On 06/06/2011 11:13 PM, Darren Hart wrote:
>
>
> On 06/06/2011 11:11 AM, Eric Dumazet wrote:
>> Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
>>>
>>
>>> If I understand the problem correctly, RO private mapping really doesn't
>>> make any sense and we should probably explicitly not support it, while
>>> special casing the RO shared mapping in support of David's scenario.
>>>
>>
>> We supported them in 2.6.18 kernels, apparently. This might sounds
>> stupid but who knows ?
>
>
> I guess this is actually the key point we need to agree on to provide a
> solution. This particular case "worked" in 2.6.18 kernels, but that
> doesn't necessarily mean it was supported, or even intentional.
>
> It sounds to me that we agree that we should support RO shared mappings.
> The question remains about whether we should introduce deliberate
> support of RO private mappings, and if so, if the forced COW approach is
> appropriate or not.
>

I disagree.

FUTEX_WAIT has side-effects.  Specifically, it eats one wakeup sent by 
FUTEX_WAKE.  So if something uses futexes on a file mapping, then a 
process with only read access could (if the semantics were changed) DoS 
the other processes by spawning a bunch of threads and FUTEX_WAITing 
from each of them.

If there were a FUTEX_WAIT_NOCONSUME that did not consume a wakeup and 
worked on RO mappings, I would drop my objection.

--Andy

Re: Change in functionality of futex() system call.

[Darren Hart]

On 06/07/2011 07:44 AM, Andy Lutomirski wrote:
> On 06/06/2011 11:13 PM, Darren Hart wrote:
>>
>>
>> On 06/06/2011 11:11 AM, Eric Dumazet wrote:
>>> Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
>>>>
>>>
>>>> If I understand the problem correctly, RO private mapping really doesn't
>>>> make any sense and we should probably explicitly not support it, while
>>>> special casing the RO shared mapping in support of David's scenario.
>>>>
>>>
>>> We supported them in 2.6.18 kernels, apparently. This might sounds
>>> stupid but who knows ?
>>
>>
>> I guess this is actually the key point we need to agree on to provide a
>> solution. This particular case "worked" in 2.6.18 kernels, but that
>> doesn't necessarily mean it was supported, or even intentional.
>>
>> It sounds to me that we agree that we should support RO shared mappings.
>> The question remains about whether we should introduce deliberate
>> support of RO private mappings, and if so, if the forced COW approach is
>> appropriate or not.
>>
> 
> I disagree.
> 
> FUTEX_WAIT has side-effects.  Specifically, it eats one wakeup sent by 
> FUTEX_WAKE.  So if something uses futexes on a file mapping, then a 
> process with only read access could (if the semantics were changed) DoS 
> the other processes by spawning a bunch of threads and FUTEX_WAITing 
> from each of them.
> 
> If there were a FUTEX_WAIT_NOCONSUME that did not consume a wakeup and 
> worked on RO mappings, I would drop my objection.


This sounds like an argument for properly managing file permissions and
carefully selecting the mapping backing your futex word - but I don't
see this as compelling rationale to disable RO support entirely and
certainly not to add yet another futex op code.


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

Re: Change in functionality of futex() system call.

[Eric Dumazet]

Le mardi 07 juin 2011 à 10:44 -0400, Andy Lutomirski a écrit :
> On 06/06/2011 11:13 PM, Darren Hart wrote:
> >
> >
> > On 06/06/2011 11:11 AM, Eric Dumazet wrote:
> >> Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
> >>>
> >>
> >>> If I understand the problem correctly, RO private mapping really doesn't
> >>> make any sense and we should probably explicitly not support it, while
> >>> special casing the RO shared mapping in support of David's scenario.
> >>>
> >>
> >> We supported them in 2.6.18 kernels, apparently. This might sounds
> >> stupid but who knows ?
> >
> >
> > I guess this is actually the key point we need to agree on to provide a
> > solution. This particular case "worked" in 2.6.18 kernels, but that
> > doesn't necessarily mean it was supported, or even intentional.
> >
> > It sounds to me that we agree that we should support RO shared mappings.
> > The question remains about whether we should introduce deliberate
> > support of RO private mappings, and if so, if the forced COW approach is
> > appropriate or not.
> >
> 
> I disagree.
> 
> FUTEX_WAIT has side-effects.  Specifically, it eats one wakeup sent by 
> FUTEX_WAKE.  So if something uses futexes on a file mapping, then a 
> process with only read access could (if the semantics were changed) DoS 
> the other processes by spawning a bunch of threads and FUTEX_WAITing 
> from each of them.
> 
> If there were a FUTEX_WAIT_NOCONSUME that did not consume a wakeup and 
> worked on RO mappings, I would drop my objection.

If a group of cooperating processes uses a memory segment to exchange
critical information, do you really think this memory segment will be
readable by other unrelated processes on the machine ?

How is this related to futex code ? 

Same problem for legacy IPC (shm, msg, sem) : Appropriate protections
are needed, obviously.

BTW, kernel/futex.c uses a global hash table (futex_queues[256]) and a
very predictable hash_futex(), so its easy to slow down futex users...

Re: Change in functionality of futex() system call.

[Andrew Lutomirski]

On Tue, Jun 7, 2011 at 11:58 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mardi 07 juin 2011 à 10:44 -0400, Andy Lutomirski a écrit :
>> On 06/06/2011 11:13 PM, Darren Hart wrote:
>> >
>> >
>> > On 06/06/2011 11:11 AM, Eric Dumazet wrote:
>> >> Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
>> >>>
>> >>
>> >>> If I understand the problem correctly, RO private mapping really doesn't
>> >>> make any sense and we should probably explicitly not support it, while
>> >>> special casing the RO shared mapping in support of David's scenario.
>> >>>
>> >>
>> >> We supported them in 2.6.18 kernels, apparently. This might sounds
>> >> stupid but who knows ?
>> >
>> >
>> > I guess this is actually the key point we need to agree on to provide a
>> > solution. This particular case "worked" in 2.6.18 kernels, but that
>> > doesn't necessarily mean it was supported, or even intentional.
>> >
>> > It sounds to me that we agree that we should support RO shared mappings.
>> > The question remains about whether we should introduce deliberate
>> > support of RO private mappings, and if so, if the forced COW approach is
>> > appropriate or not.
>> >
>>
>> I disagree.
>>
>> FUTEX_WAIT has side-effects.  Specifically, it eats one wakeup sent by
>> FUTEX_WAKE.  So if something uses futexes on a file mapping, then a
>> process with only read access could (if the semantics were changed) DoS
>> the other processes by spawning a bunch of threads and FUTEX_WAITing
>> from each of them.
>>
>> If there were a FUTEX_WAIT_NOCONSUME that did not consume a wakeup and
>> worked on RO mappings, I would drop my objection.
>
> If a group of cooperating processes uses a memory segment to exchange
> critical information, do you really think this memory segment will be
> readable by other unrelated processes on the machine ?

Depends on the design.

I have some software I'm working on that uses shared files and could
easily use futexes.  I don't want random read-only processes to
interfere with the futex protocol.

>
> How is this related to futex code ?

Because this usage is currently safe and would become unsafe with the
proposed change.

>
> Same problem for legacy IPC (shm, msg, sem) : Appropriate protections
> are needed, obviously.
>
> BTW, kernel/futex.c uses a global hash table (futex_queues[256]) and a
> very predictable hash_futex(), so its easy to slow down futex users...

There's a difference between slowing down users by abusing a kernel
hash and deadlocking users by eating a wakeup.  (If you eat a wakeup
the wakeup won't magically come back later.  It's gone.)

--Andy

Re: Change in functionality of futex() system call.

[Darren Hart]

On 06/07/2011 11:43 AM, Andrew Lutomirski wrote:
> On Tue, Jun 7, 2011 at 11:58 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> Le mardi 07 juin 2011 à 10:44 -0400, Andy Lutomirski a écrit :
>>> On 06/06/2011 11:13 PM, Darren Hart wrote:
>>>>
>>>>
>>>> On 06/06/2011 11:11 AM, Eric Dumazet wrote:
>>>>> Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
>>>>>>
>>>>>
>>>>>> If I understand the problem correctly, RO private mapping really doesn't
>>>>>> make any sense and we should probably explicitly not support it, while
>>>>>> special casing the RO shared mapping in support of David's scenario.
>>>>>>
>>>>>
>>>>> We supported them in 2.6.18 kernels, apparently. This might sounds
>>>>> stupid but who knows ?
>>>>
>>>>
>>>> I guess this is actually the key point we need to agree on to provide a
>>>> solution. This particular case "worked" in 2.6.18 kernels, but that
>>>> doesn't necessarily mean it was supported, or even intentional.
>>>>
>>>> It sounds to me that we agree that we should support RO shared mappings.
>>>> The question remains about whether we should introduce deliberate
>>>> support of RO private mappings, and if so, if the forced COW approach is
>>>> appropriate or not.
>>>>
>>>
>>> I disagree.
>>>
>>> FUTEX_WAIT has side-effects.  Specifically, it eats one wakeup sent by
>>> FUTEX_WAKE.  So if something uses futexes on a file mapping, then a
>>> process with only read access could (if the semantics were changed) DoS
>>> the other processes by spawning a bunch of threads and FUTEX_WAITing
>>> from each of them.
>>>
>>> If there were a FUTEX_WAIT_NOCONSUME that did not consume a wakeup and
>>> worked on RO mappings, I would drop my objection.
>>
>> If a group of cooperating processes uses a memory segment to exchange
>> critical information, do you really think this memory segment will be
>> readable by other unrelated processes on the machine ?
> 
> Depends on the design.
> 
> I have some software I'm working on that uses shared files and could
> easily use futexes.  I don't want random read-only processes to
> interfere with the futex protocol.


So don't use world readable files.


>>
>> How is this related to futex code ?
> 
> Because this usage is currently safe and would become unsafe with the
> proposed change.
> 
>>
>> Same problem for legacy IPC (shm, msg, sem) : Appropriate protections
>> are needed, obviously.
>>
>> BTW, kernel/futex.c uses a global hash table (futex_queues[256]) and a
>> very predictable hash_futex(), so its easy to slow down futex users...
> 
> There's a difference between slowing down users by abusing a kernel
> hash and deadlocking users by eating a wakeup.  (If you eat a wakeup
> the wakeup won't magically come back later.  It's gone.)

That's the nature of SHARED, you have to protect the mapping independent
of the futex mechanism.

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

Re: Change in functionality of futex() system call.

[Andrew Lutomirski]

On Tue, Jun 7, 2011 at 3:01 PM, Darren Hart <dvhart@linux.intel.com> wrote:
>>> If a group of cooperating processes uses a memory segment to exchange
>>> critical information, do you really think this memory segment will be
>>> readable by other unrelated processes on the machine ?
>>
>> Depends on the design.
>>
>> I have some software I'm working on that uses shared files and could
>> easily use futexes.  I don't want random read-only processes to
>> interfere with the futex protocol.
>
>
> So don't use world readable files.

...which prevents people from *reading* them, which was the whole point.

>
>
>>>
>>> How is this related to futex code ?
>>
>> Because this usage is currently safe and would become unsafe with the
>> proposed change.
>>
>>>
>>> Same problem for legacy IPC (shm, msg, sem) : Appropriate protections
>>> are needed, obviously.
>>>
>>> BTW, kernel/futex.c uses a global hash table (futex_queues[256]) and a
>>> very predictable hash_futex(), so its easy to slow down futex users...
>>
>> There's a difference between slowing down users by abusing a kernel
>> hash and deadlocking users by eating a wakeup.  (If you eat a wakeup
>> the wakeup won't magically come back later.  It's gone.)
>
> That's the nature of SHARED, you have to protect the mapping independent
> of the futex mechanism.

Well... it used to mean you have to protect from untrusted RW users.
Now it will mean you have to protect from untrusted RO users.

AFAICT sys_futex will become the only way that a user with RO access
to a file can actually interfere with the owner of the file (as
opposed to just learning information).

Why do we need this change again?

--Andy

Re: Change in functionality of futex() system call.

[Eric Dumazet]

Le mardi 07 juin 2011 à 14:43 -0400, Andrew Lutomirski a écrit :

> I have some software I'm working on that uses shared files and could
> easily use futexes.  I don't want random read-only processes to
> interfere with the futex protocol.

Relying on kernel doing 'the right thing for you' wont work on old
kernels. Some people still want to run 2.6.18 ones, for very good
reasons. Also this 'magical protection' is not documented on man pages,
so this is would be a bad choice.

Regression being one year old, you can bet many machines run with
previous behavior (safe too, unless your application design is not
secured)

> There's a difference between slowing down users by abusing a kernel
> hash and deadlocking users by eating a wakeup.  (If you eat a wakeup
> the wakeup won't magically come back later.  It's gone.)

Sure, you could let messages queues, named pipes, being readable as
well. Or your files being writeable, who knows, and lost your data too.

I dont know why I even discuss the point. A regression was added, you
cant justify it saying futexes were insecure from 2002 to 2010.

Re: Change in functionality of futex() system call.

[David Oliver]

On Tue, Jun 7, 2011 at 1:43 PM, Andrew Lutomirski <luto@mit.edu> wrote:
> On Tue, Jun 7, 2011 at 11:58 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> Le mardi 07 juin 2011 à 10:44 -0400, Andy Lutomirski a écrit :
>>> On 06/06/2011 11:13 PM, Darren Hart wrote:
>>> >
>>> >
>>> > On 06/06/2011 11:11 AM, Eric Dumazet wrote:
>>> >> Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
>>> >>>
>>> >>
>>> >>> If I understand the problem correctly, RO private mapping really doesn't
>>> >>> make any sense and we should probably explicitly not support it, while
>>> >>> special casing the RO shared mapping in support of David's scenario.
>>> >>>
>>> >>
>>> >> We supported them in 2.6.18 kernels, apparently. This might sounds
>>> >> stupid but who knows ?
>>> >
>>> >
>>> > I guess this is actually the key point we need to agree on to provide a
>>> > solution. This particular case "worked" in 2.6.18 kernels, but that
>>> > doesn't necessarily mean it was supported, or even intentional.
>>> >
>>> > It sounds to me that we agree that we should support RO shared mappings.
>>> > The question remains about whether we should introduce deliberate
>>> > support of RO private mappings, and if so, if the forced COW approach is
>>> > appropriate or not.
>>> >
>>>
>>> I disagree.
>>>
>>> FUTEX_WAIT has side-effects.  Specifically, it eats one wakeup sent by
>>> FUTEX_WAKE.  So if something uses futexes on a file mapping, then a
>>> process with only read access could (if the semantics were changed) DoS
>>> the other processes by spawning a bunch of threads and FUTEX_WAITing
>>> from each of them.
>>>
>>> If there were a FUTEX_WAIT_NOCONSUME that did not consume a wakeup and
>>> worked on RO mappings, I would drop my objection.
>>
>> If a group of cooperating processes uses a memory segment to exchange
>> critical information, do you really think this memory segment will be
>> readable by other unrelated processes on the machine ?
>
> Depends on the design.
>
> I have some software I'm working on that uses shared files and could
> easily use futexes.
>
I have software which currently uses shared files for a one way
transfer of information, which is modeled precisely by the futex (as
contrasted to the mutex) model. In this case, the number of receivers
is undetermined, so the number of wakeups is set to maxint.

The receivers are minimally trusted: they have read access to the
files, so they cannot accidentally affect other processes use of the
data. Requiring my files to be writeable by all clients would require
a serious increase in the amount of software needing to be trusted.

> I don't want random read-only processes to
> interfere with the futex protocol.
>
If limited wakeups are needed for your application, this can be
prevented by preventing access to the files. This is analogous to
preventing read access to fifos: reading a random unprotected named
pipe would have interesting consequences.

>> How is this related to futex code ?
>
> Because this usage is currently safe and would become unsafe with the
> proposed change.
>
My usage was both safe and useful before the recent kernel change.
>>
>> Same problem for legacy IPC (shm, msg, sem) : Appropriate protections
>> are needed, obviously.
>>
>> BTW, kernel/futex.c uses a global hash table (futex_queues[256]) and a
>> very predictable hash_futex(), so its easy to slow down futex users...
>
> There's a difference between slowing down users by abusing a kernel
> hash and deadlocking users by eating a wakeup.  (If you eat a wakeup
> the wakeup won't magically come back later.  It's gone.)
>
> --Andy
>

-- 
Cheers!

David.


---------------------------------------------------------------
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.

Re: Change in functionality of futex() system call.

[Andrew Lutomirski]

On Tue, Jun 7, 2011 at 3:10 PM, David Oliver <david@rgmadvisors.com> wrote:
> On Tue, Jun 7, 2011 at 1:43 PM, Andrew Lutomirski <luto@mit.edu> wrote:
>> On Tue, Jun 7, 2011 at 11:58 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>> Le mardi 07 juin 2011 à 10:44 -0400, Andy Lutomirski a écrit :
>>>> On 06/06/2011 11:13 PM, Darren Hart wrote:
>>>> >
>>>> >
>>>> > On 06/06/2011 11:11 AM, Eric Dumazet wrote:
>>>> >> Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
>>>> >>>
>>>> >>
>>>> >>> If I understand the problem correctly, RO private mapping really doesn't
>>>> >>> make any sense and we should probably explicitly not support it, while
>>>> >>> special casing the RO shared mapping in support of David's scenario.
>>>> >>>
>>>> >>
>>>> >> We supported them in 2.6.18 kernels, apparently. This might sounds
>>>> >> stupid but who knows ?
>>>> >
>>>> >
>>>> > I guess this is actually the key point we need to agree on to provide a
>>>> > solution. This particular case "worked" in 2.6.18 kernels, but that
>>>> > doesn't necessarily mean it was supported, or even intentional.
>>>> >
>>>> > It sounds to me that we agree that we should support RO shared mappings.
>>>> > The question remains about whether we should introduce deliberate
>>>> > support of RO private mappings, and if so, if the forced COW approach is
>>>> > appropriate or not.
>>>> >
>>>>
>>>> I disagree.
>>>>
>>>> FUTEX_WAIT has side-effects.  Specifically, it eats one wakeup sent by
>>>> FUTEX_WAKE.  So if something uses futexes on a file mapping, then a
>>>> process with only read access could (if the semantics were changed) DoS
>>>> the other processes by spawning a bunch of threads and FUTEX_WAITing
>>>> from each of them.
>>>>
>>>> If there were a FUTEX_WAIT_NOCONSUME that did not consume a wakeup and
>>>> worked on RO mappings, I would drop my objection.
>>>
>>> If a group of cooperating processes uses a memory segment to exchange
>>> critical information, do you really think this memory segment will be
>>> readable by other unrelated processes on the machine ?
>>
>> Depends on the design.
>>
>> I have some software I'm working on that uses shared files and could
>> easily use futexes.
>>
> I have software which currently uses shared files for a one way
> transfer of information, which is modeled precisely by the futex (as
> contrasted to the mutex) model. In this case, the number of receivers
> is undetermined, so the number of wakeups is set to maxint.
>
> The receivers are minimally trusted: they have read access to the
> files, so they cannot accidentally affect other processes use of the
> data. Requiring my files to be writeable by all clients would require
> a serious increase in the amount of software needing to be trusted.

What's wrong with adding a FUTEX_WAIT_NOCONSUME flag then?  Your
program can use it to get exactly the semantics it wants and my
program can use it or not depending on which semantics it wants.

Then we can document in the man page that, on kernels newer than
whichever version introduced the regression, read-only mappings of a
file cannot be used to interfere with futexes on that file.

--Andy

Re: Change in functionality of futex() system call.

[David Oliver]

On Tue, Jun 7, 2011 at 2:19 PM, Andrew Lutomirski <luto@mit.edu> wrote:
> On Tue, Jun 7, 2011 at 3:10 PM, David Oliver <david@rgmadvisors.com> wrote:
>> On Tue, Jun 7, 2011 at 1:43 PM, Andrew Lutomirski <luto@mit.edu> wrote:
>>> On Tue, Jun 7, 2011 at 11:58 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>> Le mardi 07 juin 2011 à 10:44 -0400, Andy Lutomirski a écrit :
>>>>> On 06/06/2011 11:13 PM, Darren Hart wrote:
>>>>> >
>>>>> >
>>>>> > On 06/06/2011 11:11 AM, Eric Dumazet wrote:
>>>>> >> Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
>>>>> >>>
>>>>> >>
>>>>> >>> If I understand the problem correctly, RO private mapping really doesn't
>>>>> >>> make any sense and we should probably explicitly not support it, while
>>>>> >>> special casing the RO shared mapping in support of David's scenario.
>>>>> >>>
>>>>> >>
>>>>> >> We supported them in 2.6.18 kernels, apparently. This might sounds
>>>>> >> stupid but who knows ?
>>>>> >
>>>>> >
>>>>> > I guess this is actually the key point we need to agree on to provide a
>>>>> > solution. This particular case "worked" in 2.6.18 kernels, but that
>>>>> > doesn't necessarily mean it was supported, or even intentional.
>>>>> >
>>>>> > It sounds to me that we agree that we should support RO shared mappings.
>>>>> > The question remains about whether we should introduce deliberate
>>>>> > support of RO private mappings, and if so, if the forced COW approach is
>>>>> > appropriate or not.
>>>>> >
>>>>>
>>>>> I disagree.
>>>>>
>>>>> FUTEX_WAIT has side-effects.  Specifically, it eats one wakeup sent by
>>>>> FUTEX_WAKE.  So if something uses futexes on a file mapping, then a
>>>>> process with only read access could (if the semantics were changed) DoS
>>>>> the other processes by spawning a bunch of threads and FUTEX_WAITing
>>>>> from each of them.
>>>>>
>>>>> If there were a FUTEX_WAIT_NOCONSUME that did not consume a wakeup and
>>>>> worked on RO mappings, I would drop my objection.
>>>>
>>>> If a group of cooperating processes uses a memory segment to exchange
>>>> critical information, do you really think this memory segment will be
>>>> readable by other unrelated processes on the machine ?
>>>
>>> Depends on the design.
>>>
>>> I have some software I'm working on that uses shared files and could
>>> easily use futexes.
>>>
>> I have software which currently uses shared files for a one way
>> transfer of information, which is modeled precisely by the futex (as
>> contrasted to the mutex) model. In this case, the number of receivers
>> is undetermined, so the number of wakeups is set to maxint.
>>
>> The receivers are minimally trusted: they have read access to the
>> files, so they cannot accidentally affect other processes use of the
>> data. Requiring my files to be writeable by all clients would require
>> a serious increase in the amount of software needing to be trusted.
>
> What's wrong with adding a FUTEX_WAIT_NOCONSUME flag then?  Your
> program can use it to get exactly the semantics it wants and my
> program can use it or not depending on which semantics it wants.
>
1. I would prefer not to require my programs have to check for kernel
version (code named "working", "regressed", and "altered") to decide
which parameters need to be sent to the futex call.
2. Doing FUTEX_WAIT_NOCONSUME would change the semantics of
futex_wake() between the "working" and "altered" kernels, as it would
no longer return the number of processes woken.

It seems that FUTEX_WAIT_NOCONSUME would be rather like a
non-consuming read on a pipe.

> Then we can document in the man page that, on kernels newer than
> whichever version introduced the regression, read-only mappings of a
> file cannot be used to interfere with futexes on that file.
>
> --Andy
>

-- 
Cheers!

David.


---------------------------------------------------------------
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.

Re: Change in functionality of futex() system call.

[Andrew Lutomirski]

On Tue, Jun 7, 2011 at 3:33 PM, David Oliver <david@rgmadvisors.com> wrote:
> On Tue, Jun 7, 2011 at 2:19 PM, Andrew Lutomirski <luto@mit.edu> wrote:
>> On Tue, Jun 7, 2011 at 3:10 PM, David Oliver <david@rgmadvisors.com> wrote:
>>> On Tue, Jun 7, 2011 at 1:43 PM, Andrew Lutomirski <luto@mit.edu> wrote:
>>>> On Tue, Jun 7, 2011 at 11:58 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>>> Le mardi 07 juin 2011 à 10:44 -0400, Andy Lutomirski a écrit :
>>>>>> On 06/06/2011 11:13 PM, Darren Hart wrote:
>>>>>> >
>>>>>> >
>>>>>> > On 06/06/2011 11:11 AM, Eric Dumazet wrote:
>>>>>> >> Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
>>>>>> >>>
>>>>>> >>
>>>>>> >>> If I understand the problem correctly, RO private mapping really doesn't
>>>>>> >>> make any sense and we should probably explicitly not support it, while
>>>>>> >>> special casing the RO shared mapping in support of David's scenario.
>>>>>> >>>
>>>>>> >>
>>>>>> >> We supported them in 2.6.18 kernels, apparently. This might sounds
>>>>>> >> stupid but who knows ?
>>>>>> >
>>>>>> >
>>>>>> > I guess this is actually the key point we need to agree on to provide a
>>>>>> > solution. This particular case "worked" in 2.6.18 kernels, but that
>>>>>> > doesn't necessarily mean it was supported, or even intentional.
>>>>>> >
>>>>>> > It sounds to me that we agree that we should support RO shared mappings.
>>>>>> > The question remains about whether we should introduce deliberate
>>>>>> > support of RO private mappings, and if so, if the forced COW approach is
>>>>>> > appropriate or not.
>>>>>> >
>>>>>>
>>>>>> I disagree.
>>>>>>
>>>>>> FUTEX_WAIT has side-effects.  Specifically, it eats one wakeup sent by
>>>>>> FUTEX_WAKE.  So if something uses futexes on a file mapping, then a
>>>>>> process with only read access could (if the semantics were changed) DoS
>>>>>> the other processes by spawning a bunch of threads and FUTEX_WAITing
>>>>>> from each of them.
>>>>>>
>>>>>> If there were a FUTEX_WAIT_NOCONSUME that did not consume a wakeup and
>>>>>> worked on RO mappings, I would drop my objection.
>>>>>
>>>>> If a group of cooperating processes uses a memory segment to exchange
>>>>> critical information, do you really think this memory segment will be
>>>>> readable by other unrelated processes on the machine ?
>>>>
>>>> Depends on the design.
>>>>
>>>> I have some software I'm working on that uses shared files and could
>>>> easily use futexes.
>>>>
>>> I have software which currently uses shared files for a one way
>>> transfer of information, which is modeled precisely by the futex (as
>>> contrasted to the mutex) model. In this case, the number of receivers
>>> is undetermined, so the number of wakeups is set to maxint.
>>>
>>> The receivers are minimally trusted: they have read access to the
>>> files, so they cannot accidentally affect other processes use of the
>>> data. Requiring my files to be writeable by all clients would require
>>> a serious increase in the amount of software needing to be trusted.
>>
>> What's wrong with adding a FUTEX_WAIT_NOCONSUME flag then?  Your
>> program can use it to get exactly the semantics it wants and my
>> program can use it or not depending on which semantics it wants.
>>
> 1. I would prefer not to require my programs have to check for kernel
> version (code named "working", "regressed", and "altered") to decide
> which parameters need to be sent to the futex call.

You don't have to check for kernel version.  Just try
FUTEX_WAIT_NOCONSUME first and retry with FUTEX_WAIT if it returns
-EINVAL.

I think you've already lost on regressed kernels regardless :-/

> 2. Doing FUTEX_WAIT_NOCONSUME would change the semantics of
> futex_wake() between the "working" and "altered" kernels, as it would
> no longer return the number of processes woken.

True, but that change couldn't affect old code because old code
wouldn't use FUTEX_WAIT_NOCONSUME.

>
> It seems that FUTEX_WAIT_NOCONSUME would be rather like a
> non-consuming read on a pipe.

More like a nonconsuming read on an eventfd, which sounds very useful.
 (Actually, I'm porting code from Windows to Linux right now that
wants that feature...)

The reason I bring this up now is that I've been annoyed that
FUTEX_WAIT can be used on an R/O mapping to interfere with futexes in
that mapping.  Under the original semantics this would have been
pretty much impossible to fix, but the regression has been there for
long enough that we have the option right now to fix it better instead
of restoring the original behavior.


--Andy

Re: Change in functionality of futex() system call.

[David Oliver]

On Tue, Jun 7, 2011 at 2:53 PM, Andrew Lutomirski <luto@mit.edu> wrote:
> On Tue, Jun 7, 2011 at 3:33 PM, David Oliver <david@rgmadvisors.com> wrote:
>> On Tue, Jun 7, 2011 at 2:19 PM, Andrew Lutomirski <luto@mit.edu> wrote:
>>> On Tue, Jun 7, 2011 at 3:10 PM, David Oliver <david@rgmadvisors.com> wrote:
>>>> On Tue, Jun 7, 2011 at 1:43 PM, Andrew Lutomirski <luto@mit.edu> wrote:
>>>>> On Tue, Jun 7, 2011 at 11:58 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>>>> Le mardi 07 juin 2011 à 10:44 -0400, Andy Lutomirski a écrit :
>>>>>>> On 06/06/2011 11:13 PM, Darren Hart wrote:
>>>>>>> >
>>>>>>> >
>>>>>>> > On 06/06/2011 11:11 AM, Eric Dumazet wrote:
>>>>>>> >> Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
>>>>>>> >>>
>>>>>>> >>
>>>>>>> >>> If I understand the problem correctly, RO private mapping really doesn't
>>>>>>> >>> make any sense and we should probably explicitly not support it, while
>>>>>>> >>> special casing the RO shared mapping in support of David's scenario.
>>>>>>> >>>
>>>>>>> >>
>>>>>>> >> We supported them in 2.6.18 kernels, apparently. This might sounds
>>>>>>> >> stupid but who knows ?
>>>>>>> >
>>>>>>> >
>>>>>>> > I guess this is actually the key point we need to agree on to provide a
>>>>>>> > solution. This particular case "worked" in 2.6.18 kernels, but that
>>>>>>> > doesn't necessarily mean it was supported, or even intentional.
>>>>>>> >
>>>>>>> > It sounds to me that we agree that we should support RO shared mappings.
>>>>>>> > The question remains about whether we should introduce deliberate
>>>>>>> > support of RO private mappings, and if so, if the forced COW approach is
>>>>>>> > appropriate or not.
>>>>>>> >
>>>>>>>
>>>>>>> I disagree.
>>>>>>>
>>>>>>> FUTEX_WAIT has side-effects.  Specifically, it eats one wakeup sent by
>>>>>>> FUTEX_WAKE.  So if something uses futexes on a file mapping, then a
>>>>>>> process with only read access could (if the semantics were changed) DoS
>>>>>>> the other processes by spawning a bunch of threads and FUTEX_WAITing
>>>>>>> from each of them.
>>>>>>>
>>>>>>> If there were a FUTEX_WAIT_NOCONSUME that did not consume a wakeup and
>>>>>>> worked on RO mappings, I would drop my objection.
>>>>>>
>>>>>> If a group of cooperating processes uses a memory segment to exchange
>>>>>> critical information, do you really think this memory segment will be
>>>>>> readable by other unrelated processes on the machine ?
>>>>>
>>>>> Depends on the design.
>>>>>
>>>>> I have some software I'm working on that uses shared files and could
>>>>> easily use futexes.
>>>>>
>>>> I have software which currently uses shared files for a one way
>>>> transfer of information, which is modeled precisely by the futex (as
>>>> contrasted to the mutex) model. In this case, the number of receivers
>>>> is undetermined, so the number of wakeups is set to maxint.
>>>>
>>>> The receivers are minimally trusted: they have read access to the
>>>> files, so they cannot accidentally affect other processes use of the
>>>> data. Requiring my files to be writeable by all clients would require
>>>> a serious increase in the amount of software needing to be trusted.
>>>
>>> What's wrong with adding a FUTEX_WAIT_NOCONSUME flag then?  Your
>>> program can use it to get exactly the semantics it wants and my
>>> program can use it or not depending on which semantics it wants.
>>>
>> 1. I would prefer not to require my programs have to check for kernel
>> version (code named "working", "regressed", and "altered") to decide
>> which parameters need to be sent to the futex call.
>
> You don't have to check for kernel version.  Just try
> FUTEX_WAIT_NOCONSUME first and retry with FUTEX_WAIT if it returns
> -EINVAL.
>
... and punt if that gives me an EFAULT. Possible but clumsy.
Fortunately, I'm not writing code for general consumption.

> I think you've already lost on regressed kernels regardless :-/
>
>> 2. Doing FUTEX_WAIT_NOCONSUME would change the semantics of
>> futex_wake() between the "working" and "altered" kernels, as it would
>> no longer return the number of processes woken.
>
> True, but that change couldn't affect old code because old code
> wouldn't use FUTEX_WAIT_NOCONSUME.
>
So, how would I find out the number of processes awakened by the
futex_wake() - I only care for statistical purposes.

>>
>> It seems that FUTEX_WAIT_NOCONSUME would be rather like a
>> non-consuming read on a pipe.
>
> More like a nonconsuming read on an eventfd, which sounds very useful.
>  (Actually, I'm porting code from Windows to Linux right now that
> wants that feature...)
>
> The reason I bring this up now is that I've been annoyed that
> FUTEX_WAIT can be used on an R/O mapping to interfere with futexes in
> that mapping.  Under the original semantics this would have been
> pretty much impossible to fix, but the regression has been there for
> long enough that we have the option right now to fix it better instead
> of restoring the original behavior.
>
Not being a kernel developer, the change seems very recent - about
when I started finding my code failing with EFAULTs.

>From my perspective, that's a real case of my futexes being interfered with :).
>
> --Andy
>

-- 
Cheers!

David.


---------------------------------------------------------------
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.

Re: Change in functionality of futex() system call.

[Andrew Lutomirski]

On Tue, Jun 7, 2011 at 4:04 PM, David Oliver <david@rgmadvisors.com> wrote:
> On Tue, Jun 7, 2011 at 2:53 PM, Andrew Lutomirski <luto@mit.edu> wrote:
>> On Tue, Jun 7, 2011 at 3:33 PM, David Oliver <david@rgmadvisors.com> wrote:
>>> On Tue, Jun 7, 2011 at 2:19 PM, Andrew Lutomirski <luto@mit.edu> wrote:
>>>> On Tue, Jun 7, 2011 at 3:10 PM, David Oliver <david@rgmadvisors.com> wrote:
>>>>> On Tue, Jun 7, 2011 at 1:43 PM, Andrew Lutomirski <luto@mit.edu> wrote:
>>>>>> On Tue, Jun 7, 2011 at 11:58 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>>>>> Le mardi 07 juin 2011 à 10:44 -0400, Andy Lutomirski a écrit :
>>>>>>>> On 06/06/2011 11:13 PM, Darren Hart wrote:
>>>>>>>> >
>>>>>>>> >
>>>>>>>> > On 06/06/2011 11:11 AM, Eric Dumazet wrote:
>>>>>>>> >> Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
>>>>>>>> >>>
>>>>>>>> >>
>>>>>>>> >>> If I understand the problem correctly, RO private mapping really doesn't
>>>>>>>> >>> make any sense and we should probably explicitly not support it, while
>>>>>>>> >>> special casing the RO shared mapping in support of David's scenario.
>>>>>>>> >>>
>>>>>>>> >>
>>>>>>>> >> We supported them in 2.6.18 kernels, apparently. This might sounds
>>>>>>>> >> stupid but who knows ?
>>>>>>>> >
>>>>>>>> >
>>>>>>>> > I guess this is actually the key point we need to agree on to provide a
>>>>>>>> > solution. This particular case "worked" in 2.6.18 kernels, but that
>>>>>>>> > doesn't necessarily mean it was supported, or even intentional.
>>>>>>>> >
>>>>>>>> > It sounds to me that we agree that we should support RO shared mappings.
>>>>>>>> > The question remains about whether we should introduce deliberate
>>>>>>>> > support of RO private mappings, and if so, if the forced COW approach is
>>>>>>>> > appropriate or not.
>>>>>>>> >
>>>>>>>>
>>>>>>>> I disagree.
>>>>>>>>
>>>>>>>> FUTEX_WAIT has side-effects.  Specifically, it eats one wakeup sent by
>>>>>>>> FUTEX_WAKE.  So if something uses futexes on a file mapping, then a
>>>>>>>> process with only read access could (if the semantics were changed) DoS
>>>>>>>> the other processes by spawning a bunch of threads and FUTEX_WAITing
>>>>>>>> from each of them.
>>>>>>>>
>>>>>>>> If there were a FUTEX_WAIT_NOCONSUME that did not consume a wakeup and
>>>>>>>> worked on RO mappings, I would drop my objection.
>>>>>>>
>>>>>>> If a group of cooperating processes uses a memory segment to exchange
>>>>>>> critical information, do you really think this memory segment will be
>>>>>>> readable by other unrelated processes on the machine ?
>>>>>>
>>>>>> Depends on the design.
>>>>>>
>>>>>> I have some software I'm working on that uses shared files and could
>>>>>> easily use futexes.
>>>>>>
>>>>> I have software which currently uses shared files for a one way
>>>>> transfer of information, which is modeled precisely by the futex (as
>>>>> contrasted to the mutex) model. In this case, the number of receivers
>>>>> is undetermined, so the number of wakeups is set to maxint.
>>>>>
>>>>> The receivers are minimally trusted: they have read access to the
>>>>> files, so they cannot accidentally affect other processes use of the
>>>>> data. Requiring my files to be writeable by all clients would require
>>>>> a serious increase in the amount of software needing to be trusted.
>>>>
>>>> What's wrong with adding a FUTEX_WAIT_NOCONSUME flag then?  Your
>>>> program can use it to get exactly the semantics it wants and my
>>>> program can use it or not depending on which semantics it wants.
>>>>
>>> 1. I would prefer not to require my programs have to check for kernel
>>> version (code named "working", "regressed", and "altered") to decide
>>> which parameters need to be sent to the futex call.
>>
>> You don't have to check for kernel version.  Just try
>> FUTEX_WAIT_NOCONSUME first and retry with FUTEX_WAIT if it returns
>> -EINVAL.
>>
> ... and punt if that gives me an EFAULT. Possible but clumsy.
> Fortunately, I'm not writing code for general consumption.
>
>> I think you've already lost on regressed kernels regardless :-/
>>
>>> 2. Doing FUTEX_WAIT_NOCONSUME would change the semantics of
>>> futex_wake() between the "working" and "altered" kernels, as it would
>>> no longer return the number of processes woken.
>>
>> True, but that change couldn't affect old code because old code
>> wouldn't use FUTEX_WAIT_NOCONSUME.
>>
> So, how would I find out the number of processes awakened by the
> futex_wake() - I only care for statistical purposes.

Add a FUTEX_WAKE_COUNT_NOCONSUME or some such magic flag.  Yeah, not so pretty.

>
>>>
>>> It seems that FUTEX_WAIT_NOCONSUME would be rather like a
>>> non-consuming read on a pipe.
>>
>> More like a nonconsuming read on an eventfd, which sounds very useful.
>>  (Actually, I'm porting code from Windows to Linux right now that
>> wants that feature...)
>>
>> The reason I bring this up now is that I've been annoyed that
>> FUTEX_WAIT can be used on an R/O mapping to interfere with futexes in
>> that mapping.  Under the original semantics this would have been
>> pretty much impossible to fix, but the regression has been there for
>> long enough that we have the option right now to fix it better instead
>> of restoring the original behavior.
>>
> Not being a kernel developer, the change seems very recent - about
> when I started finding my code failing with EFAULTs.
>
> From my perspective, that's a real case of my futexes being interfered with :).

Fair enough.  But it's a little late to prevent the regression.

--Andy

Re: Change in functionality of futex() system call.

[Kyle Moffett]

On Tue, Jun 7, 2011 at 15:19, Andrew Lutomirski <luto@mit.edu> wrote:
> On Tue, Jun 7, 2011 at 3:10 PM, David Oliver <david@rgmadvisors.com> wrote:
>> I have software which currently uses shared files for a one way
>> transfer of information, which is modeled precisely by the futex (as
>> contrasted to the mutex) model. In this case, the number of receivers
>> is undetermined, so the number of wakeups is set to maxint.
>>
>> The receivers are minimally trusted: they have read access to the
>> files, so they cannot accidentally affect other processes use of the
>> data. Requiring my files to be writeable by all clients would require
>> a serious increase in the amount of software needing to be trusted.
>
> What's wrong with adding a FUTEX_WAIT_NOCONSUME flag then?  Your
> program can use it to get exactly the semantics it wants and my
> program can use it or not depending on which semantics it wants.
>
> Then we can document in the man page that, on kernels newer than
> whichever version introduced the regression, read-only mappings of a
> file cannot be used to interfere with futexes on that file.

Hmm, I would actually call it "FUTEX_POLL", since that better reflects the
operation being performed.

Certainly you would want to avoid allowing FUTEX_POLL to "steal"
limited wakeups from FUTEX_CMP_REQUEUE or whatever, so you
also need a new "FUTEX_NOTIFY".  Alternatively I guess you could just
special-case the FUTEX_WAKE && wakeups == INTMAX combination to
also notify FUTEX_POLL processes.

I almost wonder if long-term there might possibly be some decent way
to integrate this with eventfds to allow a thread to wait for notifications from
any number of memory addresses as well as other event sources.  This
would be a similar extension to signalfd, only for futexes.

Cheers,
Kyle Moffett

Re: Change in functionality of futex() system call.

[David Oliver]

On Tue, Jun 7, 2011 at 5:26 PM, Kyle Moffett <kyle@moffetthome.net> wrote:
> On Tue, Jun 7, 2011 at 15:19, Andrew Lutomirski <luto@mit.edu> wrote:
>> On Tue, Jun 7, 2011 at 3:10 PM, David Oliver <david@rgmadvisors.com> wrote:
>>> I have software which currently uses shared files for a one way
>>> transfer of information, which is modeled precisely by the futex (as
>>> contrasted to the mutex) model. In this case, the number of receivers
>>> is undetermined, so the number of wakeups is set to maxint.
>>>
>>> The receivers are minimally trusted: they have read access to the
>>> files, so they cannot accidentally affect other processes use of the
>>> data. Requiring my files to be writeable by all clients would require
>>> a serious increase in the amount of software needing to be trusted.
>>
>> What's wrong with adding a FUTEX_WAIT_NOCONSUME flag then?  Your
>> program can use it to get exactly the semantics it wants and my
>> program can use it or not depending on which semantics it wants.
>>
>> Then we can document in the man page that, on kernels newer than
>> whichever version introduced the regression, read-only mappings of a
>> file cannot be used to interfere with futexes on that file.
>
> Hmm, I would actually call it "FUTEX_POLL", since that better reflects the
> operation being performed.
>
> Certainly you would want to avoid allowing FUTEX_POLL to "steal"
> limited wakeups from FUTEX_CMP_REQUEUE or whatever, so you
> also need a new "FUTEX_NOTIFY".  Alternatively I guess you could just
> special-case the FUTEX_WAKE && wakeups == INTMAX combination to
> also notify FUTEX_POLL processes.
>
> I almost wonder if long-term there might possibly be some decent way
> to integrate this with eventfds to allow a thread to wait for notifications from
> any number of memory addresses as well as other event sources.  This
> would be a similar extension to signalfd, only for futexes.
>
> Cheers,
> Kyle Moffett
>

Having a new call is inelegant from a futex(2) user perspective, as
the need for a change is due to the kernel implementation and/or mutex
requirements. The futex() system call, as documented, is ideal for a
single producer to signal multiple receivers of state updates.

If it is truly necessary to add new variants to futex() to protect
applications that allow untrusted applications read access to their
mutexes, I would avoid both the names suggested, as consumption of
wakeups is not an obvious issue to users, and POLL suggests waiting
for multiple entities as in poll(2) (which is not provided), or
returning immediately (which is orthogonally provided by the timeout
parameter). What is being provided from the user point of view is a
FUTEX_WAIT per the man page, which doesn't require write access. How
about FUTEX_WAIT_RDONLY?

Alternatively, use the current call and document that when process
performing a FUTEX_WAIT on read-only memory are woken, they do not
count towards the number reported as being woken.

Best, IMHO, would be to document that providing read access to mutexes
to untrusted software is unsafe behavior, and restore read only access
to readers of futexes.

-- 
Cheers!

David.


---------------------------------------------------------------
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.

Re: Change in functionality of futex() system call.

[Andrew Lutomirski]

On Wed, Jun 8, 2011 at 11:20 AM, David Oliver <david@rgmadvisors.com> wrote:
>
> Having a new call is inelegant from a futex(2) user perspective, as
> the need for a change is due to the kernel implementation and/or mutex
> requirements. The futex() system call, as documented, is ideal for a
> single producer to signal multiple receivers of state updates.
>
> If it is truly necessary to add new variants to futex() to protect
> applications that allow untrusted applications read access to their
> mutexes, I would avoid both the names suggested, as consumption of
> wakeups is not an obvious issue to users, and POLL suggests waiting
> for multiple entities as in poll(2) (which is not provided), or
> returning immediately (which is orthogonally provided by the timeout
> parameter). What is being provided from the user point of view is a
> FUTEX_WAIT per the man page, which doesn't require write access. How
> about FUTEX_WAIT_RDONLY?

That name sounds good.

>
> Alternatively, use the current call and document that when process
> performing a FUTEX_WAIT on read-only memory are woken, they do not
> count towards the number reported as being woken.

I don't see anything wrong with that, either.

>
> Best, IMHO, would be to document that providing read access to mutexes
> to untrusted software is unsafe behavior, and restore read only access
> to readers of futexes.

I think it should be safe, and it would be easy to make it be safe
(i.e. make no changes at all).

--Andy

Re: Change in functionality of futex() system call.

[Darren Hart]

On 06/08/2011 08:20 AM, David Oliver wrote:
> On Tue, Jun 7, 2011 at 5:26 PM, Kyle Moffett <kyle@moffetthome.net> wrote:
>> On Tue, Jun 7, 2011 at 15:19, Andrew Lutomirski <luto@mit.edu> wrote:
>>> On Tue, Jun 7, 2011 at 3:10 PM, David Oliver <david@rgmadvisors.com> wrote:
>>>> I have software which currently uses shared files for a one way
>>>> transfer of information, which is modeled precisely by the futex (as
>>>> contrasted to the mutex) model. In this case, the number of receivers
>>>> is undetermined, so the number of wakeups is set to maxint.
>>>>
>>>> The receivers are minimally trusted: they have read access to the
>>>> files, so they cannot accidentally affect other processes use of the
>>>> data. Requiring my files to be writeable by all clients would require
>>>> a serious increase in the amount of software needing to be trusted.
>>>
>>> What's wrong with adding a FUTEX_WAIT_NOCONSUME flag then?  Your
>>> program can use it to get exactly the semantics it wants and my
>>> program can use it or not depending on which semantics it wants.
>>>
>>> Then we can document in the man page that, on kernels newer than
>>> whichever version introduced the regression, read-only mappings of a
>>> file cannot be used to interfere with futexes on that file.
>>
>> Hmm, I would actually call it "FUTEX_POLL", since that better reflects the
>> operation being performed.
>>
>> Certainly you would want to avoid allowing FUTEX_POLL to "steal"
>> limited wakeups from FUTEX_CMP_REQUEUE or whatever, so you
>> also need a new "FUTEX_NOTIFY".  Alternatively I guess you could just
>> special-case the FUTEX_WAKE && wakeups == INTMAX combination to
>> also notify FUTEX_POLL processes.
>>
>> I almost wonder if long-term there might possibly be some decent way
>> to integrate this with eventfds to allow a thread to wait for notifications from
>> any number of memory addresses as well as other event sources.  This
>> would be a similar extension to signalfd, only for futexes.
>>
>> Cheers,
>> Kyle Moffett
>>
> 
> Having a new call is inelegant from a futex(2) user perspective, as
> the need for a change is due to the kernel implementation and/or mutex
> requirements. The futex() system call, as documented, is ideal for a
> single producer to signal multiple receivers of state updates.
> 
> If it is truly necessary to add new variants to futex() to protect
> applications that allow untrusted applications read access to their
> mutexes, I would avoid both the names suggested, as consumption of
> wakeups is not an obvious issue to users, and POLL suggests waiting
> for multiple entities as in poll(2) (which is not provided), or
> returning immediately (which is orthogonally provided by the timeout
> parameter). What is being provided from the user point of view is a
> FUTEX_WAIT per the man page, which doesn't require write access. How
> about FUTEX_WAIT_RDONLY?
> 
> Alternatively, use the current call and document that when process
> performing a FUTEX_WAIT on read-only memory are woken, they do not
> count towards the number reported as being woken.
> 
> Best, IMHO, would be to document that providing read access to mutexes
> to untrusted software is unsafe behavior, and restore read only access
> to readers of futexes.

I'm inclined to agree with this approach.

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

Re: Change in functionality of futex() system call.

[KOSAKI Motohiro]

[-- Attachment #1: Type: text/plain, Size: 2737 bytes --]

Hi

I'm sorry for the long delay.

>> Having a new call is inelegant from a futex(2) user perspective, as
>> the need for a change is due to the kernel implementation and/or mutex
>> requirements. The futex() system call, as documented, is ideal for a
>> single producer to signal multiple receivers of state updates.
>>
>> If it is truly necessary to add new variants to futex() to protect
>> applications that allow untrusted applications read access to their
>> mutexes, I would avoid both the names suggested, as consumption of
>> wakeups is not an obvious issue to users, and POLL suggests waiting
>> for multiple entities as in poll(2) (which is not provided), or
>> returning immediately (which is orthogonally provided by the timeout
>> parameter). What is being provided from the user point of view is a
>> FUTEX_WAIT per the man page, which doesn't require write access. How
>> about FUTEX_WAIT_RDONLY?
>>
>> Alternatively, use the current call and document that when process
>> performing a FUTEX_WAIT on read-only memory are woken, they do not
>> count towards the number reported as being woken.
>>
>> Best, IMHO, would be to document that providing read access to mutexes
>> to untrusted software is unsafe behavior, and restore read only access
>> to readers of futexes.
> 
> I'm inclined to agree with this approach.

Honestly says, I didn't thought RO mapping users are there in real world.
I'm sorry. I did hope to fix COW issue, not intended to break David Oliver's
workload.

And, I now also recognized this is hard to fix issue. Any fix might screw up
Perter's long time performance improvement effort. Hmm....

Personally, I also incline to agree with FUTEX_WAIT_RDONLY approach. But I
also hope to remove David's head pain. I now have big dilemma. Could you
please give me a chance to show my fixing idea once? If anybody dislike
my idea, I'll drop my opinion quickly.

Today, I've made one concept proof patch. The idea is

1) check the pte of the target address is RW attribute.
 2-1) if yes, it has no COW chance. we can safely use gup result.
 2-2) if no, we have to fallback slow vma walk. maybe, it's okey.
      both RO mappings and COW are rare situation.

I'm not futex expert and I really hope and have to get Darren't review.
I hope to hear somebody's comments.

0001-Revert-futexes-Remove-rw-parameter-from-get_futex_ke.patch
0002-make-slowpath.patch
	Alternative fixing idea. Just RFC.

0001-FUTEX_WAIT-read-only.patch
	testcase for Darren't futextest git tree.

performance.txt
	performance result of Darren't performance test in futextest git.
	This seems no significant performance loss.


Again, I have no strong opinion. If anyone put objection, I'll drop this
proposal immediately.


Thanks.




[-- Attachment #2: 0001-FUTEX_WAIT-read-only.patch --]
[-- Type: text/plain, Size: 4971 bytes --]

>From 7936ff2c763154f05588e0d35e0cd231774e8aa9 Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Date: Thu, 9 Jun 2011 20:20:17 +0900
Subject: [PATCH] FUTEX_WAIT read only

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 functional/Makefile        |    3 +-
 functional/futex_wait_ro.c |  152 ++++++++++++++++++++++++++++++++++++++++++++
 functional/run.sh          |    2 +
 3 files changed, 156 insertions(+), 1 deletions(-)
 create mode 100644 functional/futex_wait_ro.c

diff --git a/functional/Makefile b/functional/Makefile
index 6ecb42c..aec23e5 100644
--- a/functional/Makefile
+++ b/functional/Makefile
@@ -10,7 +10,8 @@ TARGETS := \
 	futex_requeue_pi_signal_restart \
 	futex_requeue_pi_mismatched_ops \
 	futex_wait_uninitialized_heap \
-	futex_wait_private_mapped_file
+	futex_wait_private_mapped_file \
+	futex_wait_ro
 
 .PHONY: all clean
 all: $(TARGETS)
diff --git a/functional/futex_wait_ro.c b/functional/futex_wait_ro.c
new file mode 100644
index 0000000..59bf2e7
--- /dev/null
+++ b/functional/futex_wait_ro.c
@@ -0,0 +1,152 @@
+/******************************************************************************
+ *
+ * Copyright FUJITSU LIMITED 2011
+ * Copyright KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
+ *
+ *   This program is free software;  you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
+ *   the GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program;  if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * NAME
+ *      futex_wait_ro.c
+ *
+ * DESCRIPTION
+ *
+ * AUTHOR
+ *      KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
+ *
+ * HISTORY
+ *      2011-Jun-9: Initial version by KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
+ *
+ *****************************************************************************/
+
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <syscall.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <errno.h>
+#include <linux/futex.h>
+#include <libgen.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+#include "logging.h"
+#include "futextest.h"
+
+#define WAIT_US 500000
+
+static int child_blocked = 1;
+static int child_ret;
+
+
+void usage(char *prog)
+{
+	printf("Usage: %s\n", prog);
+	printf("  -c	Use color\n");
+	printf("  -h	Display this help message\n");
+	printf("  -v L	Verbosity level: %d=QUIET %d=CRITICAL %d=INFO\n",
+	       VQUIET, VCRITICAL, VINFO);
+}
+
+void *wait_thread(void *arg)
+{
+	int res;
+	int fd = (int)(unsigned long)arg;
+	futex_t *futex;
+	int i;
+
+	futex = mmap(0, sizeof(int), PROT_READ, MAP_SHARED, fd, 0);
+	child_ret = RET_PASS;
+
+	res = futex_wait(futex, *futex, NULL, 0);
+	child_blocked = 0;
+
+	if (res != 0) {
+		error("futex failure\n", errno);
+		child_ret = RET_ERROR;
+	}
+	pthread_exit(NULL);
+}
+
+
+int main(int argc, char **argv)
+{
+	int c, ret = RET_PASS;
+	long page_size;
+	pthread_t thr;
+	futex_t *futex;
+	int fd;
+
+	while ((c = getopt(argc, argv, "chv:")) != -1) {
+		switch(c) {
+		case 'c':
+			log_color(1);
+			break;
+		case 'h':
+			usage(basename(argv[0]));
+			exit(0);
+		case 'v':
+			log_verbosity(atoi(optarg));
+			break;
+		default:
+			usage(basename(argv[0]));
+			exit(1);
+		}
+	}
+
+	page_size = sysconf(_SC_PAGESIZE);
+
+	fd = open("/tmp/futex_test", O_RDWR|O_CREAT, 0644);
+	write(fd, "\1\1\1\1", 4);
+
+
+	printf("%s: Test FUTEX_WAIT read only mapping futex \n",
+	       basename(argv[0]));
+
+	ret = pthread_create(&thr, NULL, wait_thread, (void*)(unsigned long)fd);
+	if (ret) {
+		error("pthread_create\n", errno);
+		ret = RET_ERROR;
+		goto out;
+	}
+
+	usleep(1000);
+
+	futex = mmap(NULL, page_size, PROT_READ|PROT_WRITE,
+		   MAP_SHARED, fd, 0);
+	if (futex == (void *)-1) {
+		error("mmap\n", errno);
+		exit(1);
+	}
+	*futex = 0;
+	futex_wake(futex, 1, 0);
+
+	info("waiting %dus for child to return\n", WAIT_US);
+	usleep(WAIT_US);
+
+	if (child_blocked) {
+		fail("child blocked in kernel\n");
+		ret = RET_FAIL;
+	} else {
+		ret = child_ret;
+	}
+
+ out:
+	print_result(ret);
+	return ret;
+}
diff --git a/functional/run.sh b/functional/run.sh
index 571a4a4..b82c32d 100755
--- a/functional/run.sh
+++ b/functional/run.sh
@@ -90,3 +90,5 @@ echo
 ./futex_wait_uninitialized_heap $COLOR
 ./futex_wait_private_mapped_file $COLOR
 
+echo
+./futex_wait_ro $COLOR
\ No newline at end of file
-- 
1.7.3.1


[-- Attachment #3: 0001-Revert-futexes-Remove-rw-parameter-from-get_futex_ke.patch --]
[-- Type: text/plain, Size: 4460 bytes --]

>From 7c44963ec734f9bf04135ce68dfe0768f75e6daf Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Date: Wed, 8 Jun 2011 09:21:59 +0900
Subject: [PATCH 1/2] Revert "futexes: Remove rw parameter from get_futex_key()"

This reverts commit 7485d0d3758e8e6491a5c9468114e74dc050785d.

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 kernel/futex.c |   27 +++++++++++++++------------
 1 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index fe28dc2..1a91ea2 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)
  *
  * Returns a negative error code or 0
  * The key words are stored in *key on success.
@@ -229,7 +231,7 @@ 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;
@@ -252,7 +254,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key)
 	 *        but access_ok() should be faster than find_vma()
 	 */
 	if (!fshared) {
-		if (unlikely(!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))))
+		if (unlikely(!access_ok(rw, uaddr, sizeof(u32))))
 			return -EFAULT;
 		key->private.mm = mm;
 		key->private.address = address;
@@ -261,7 +263,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key)
 	}
 
 again:
-	err = get_user_pages_fast(address, 1, 1, &page);
+	err = get_user_pages_fast(address, 1, rw == VERIFY_WRITE, &page);
 	if (err < 0)
 		return err;
 
@@ -940,7 +942,7 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)
 	if (!bitset)
 		return -EINVAL;
 
-	ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &key);
+	ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &key, VERIFY_READ);
 	if (unlikely(ret != 0))
 		goto out;
 
@@ -986,10 +988,10 @@ futex_wake_op(u32 __user *uaddr1, unsigned int flags, u32 __user *uaddr2,
 	int ret, op_ret;
 
 retry:
-	ret = get_futex_key(uaddr1, flags & FLAGS_SHARED, &key1);
+	ret = get_futex_key(uaddr1, flags & FLAGS_SHARED, &key1, VERIFY_READ);
 	if (unlikely(ret != 0))
 		goto out;
-	ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2);
+	ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2, VERIFY_WRITE);
 	if (unlikely(ret != 0))
 		goto out_put_key1;
 
@@ -1243,10 +1245,11 @@ retry:
 		pi_state = NULL;
 	}
 
-	ret = get_futex_key(uaddr1, flags & FLAGS_SHARED, &key1);
+	ret = get_futex_key(uaddr1, flags & FLAGS_SHARED, &key1, VERIFY_READ);
 	if (unlikely(ret != 0))
 		goto out;
-	ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2);
+	ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2,
+			    requeue_pi ? VERIFY_WRITE : VERIFY_READ);
 	if (unlikely(ret != 0))
 		goto out_put_key1;
 
@@ -1790,7 +1793,7 @@ static int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
 	 * while the syscall executes.
 	 */
 retry:
-	ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &q->key);
+	ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &q->key, VERIFY_READ);
 	if (unlikely(ret != 0))
 		return ret;
 
@@ -1941,7 +1944,7 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int flags, int detect,
 	}
 
 retry:
-	ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &q.key);
+	ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &q.key, VERIFY_WRITE);
 	if (unlikely(ret != 0))
 		goto out;
 
@@ -2060,7 +2063,7 @@ retry:
 	if ((uval & FUTEX_TID_MASK) != vpid)
 		return -EPERM;
 
-	ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &key);
+	ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &key, VERIFY_WRITE);
 	if (unlikely(ret != 0))
 		goto out;
 
@@ -2249,7 +2252,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 	debug_rt_mutex_init_waiter(&rt_waiter);
 	rt_waiter.task = NULL;
 
-	ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2);
+	ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2, VERIFY_WRITE);
 	if (unlikely(ret != 0))
 		goto out;
 
-- 
1.7.3.1


[-- Attachment #4: 0002-make-slowpath.patch --]
[-- Type: text/plain, Size: 2676 bytes --]

>From afce8c6e41e215c7e3ba2272c8431a9acf71305a Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Date: Thu, 9 Jun 2011 16:19:33 +0900
Subject: [PATCH 2/2] make slowpath

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 kernel/futex.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 1a91ea2..5382c05 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -213,6 +213,58 @@ static void drop_futex_key_refs(union futex_key *key)
 	}
 }
 
+static noinline int
+get_futex_key_slowpath(unsigned long address, union futex_key *key, int rw)
+{
+	int err;
+	struct mm_struct *mm = current->mm;
+	struct page *page;
+	struct vm_area_struct *vma;
+
+	down_read(&mm->mmap_sem);
+
+	vma = find_extend_vma(mm, address);
+	err = -EFAULT;
+	if (unlikely(!vma))
+		goto err_out;
+
+	err = -EACCES;
+	if (!(vma->vm_flags & (rw==VERIFY_READ ? VM_READ : VM_WRITE)))
+		goto err_out;
+
+	err = 0;
+	if (likely(!(vma->vm_flags & VM_MAYSHARE))) {
+		key->both.offset |= FUT_OFF_MMSHARED;
+		key->private.mm = mm;
+		key->private.address = address;
+		goto out;
+	}
+
+	if (likely(!(vma->vm_flags & VM_NONLINEAR))) {
+		key->shared.inode = vma->vm_file->f_path.dentry->d_inode;
+		key->both.offset |= FUT_OFF_INODE;
+		key->shared.pgoff = (((address - vma->vm_start) >> PAGE_SHIFT)
+				     + vma->vm_pgoff);
+		goto out;
+	}
+
+	err = get_user_pages(current, mm, address, 1, 0, 0, &page, NULL);
+	if (err < 0)
+		goto err_out;
+
+	key->shared.inode = vma->vm_file->f_path.dentry->d_inode;
+	key->both.offset |= FUT_OFF_INODE;
+	key->shared.pgoff = page->index << (PAGE_CACHE_SHIFT - PAGE_SHIFT);
+	put_page(page);
+
+ out:
+	get_futex_key_refs(key);
+ err_out:
+	up_read(&current->mm->mmap_sem);
+
+	return err;
+}
+
 /**
  * get_futex_key() - Get parameters which are the keys for a futex
  * @uaddr:	virtual address of the futex
@@ -236,7 +288,7 @@ 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 ret;
 
 	/*
 	 * The futex address must be "naturally" aligned.
@@ -263,9 +315,9 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, int rw)
 	}
 
 again:
-	err = get_user_pages_fast(address, 1, rw == VERIFY_WRITE, &page);
-	if (err < 0)
-		return err;
+	ret = __get_user_pages_fast(address, 1, 1, &page);
+	if (unlikely(!ret))
+		return get_futex_key_slowpath(address, key, rw);
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 	page_head = page;
-- 
1.7.3.1


[-- Attachment #5: performance.txt --]
[-- Type: text/plain, Size: 4312 bytes --]

<without patch>

futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=1
Result: 39683 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=2
Result: 12804 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=3
Result: 12626 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=4
Result: 12453 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=5
Result: 12453 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=6
Result: 12484 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=8
Result: 12531 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=10
Result: 12392 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=12
Result: 12610 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=16
Result: 12469 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=24
Result: 12422 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=32
Result: 12500 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=64
Result: 12346 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=128
Result: 12484 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=256
Result: 12330 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=512
Result: 11628 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=1024
Result: 8285 Kiter/s


<with patch>

futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=1
Result: 39683 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=2
Result: 12500 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=3
Result: 13055 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=4
Result: 12438 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=5
Result: 12453 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=6
Result: 12755 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=8
Result: 12547 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=10
Result: 12516 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=12
Result: 12407 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=16
Result: 12469 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=24
Result: 12453 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=32
Result: 12453 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=64
Result: 12330 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=128
Result: 12407 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=256
Result: 12346 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=512
Result: 11876 Kiter/s
futex_wait: Measure FUTEX_WAIT operations per second
        Arguments: iterations=100000000 threads=1024
Result: 9107 Kiter/s

Re: Change in functionality of futex() system call.

[Peter Zijlstra]

On Thu, 2011-06-09 at 20:37 +0900, KOSAKI Motohiro wrote:
> 1) check the pte of the target address is RW attribute.
>  2-1) if yes, it has no COW chance. we can safely use gup result.
>  2-2) if no, we have to fallback slow vma walk. maybe, it's okey.
>       both RO mappings and COW are rare situation. 

Not so, clean file pages are RO, even if the vma is writable. We use the
fault to track dirty state with.

Re: Change in functionality of futex() system call.

[Peter Zijlstra]

On Thu, 2011-06-09 at 14:05 +0200, Peter Zijlstra wrote:
> On Thu, 2011-06-09 at 20:37 +0900, KOSAKI Motohiro wrote:
> > 1) check the pte of the target address is RW attribute.
> >  2-1) if yes, it has no COW chance. we can safely use gup result.
> >  2-2) if no, we have to fallback slow vma walk. maybe, it's okey.
> >       both RO mappings and COW are rare situation. 
> 
> Not so, clean file pages are RO, even if the vma is writable. We use the
> fault to track dirty state with.

Why can't something like
http://marc.info/?l=linux-kernel&m=130737669810421 work once you restore
the rw argument?

Re: Change in functionality of futex() system call.

[KOSAKI Motohiro]

(2011/06/10 2:58), Peter Zijlstra wrote:
> On Thu, 2011-06-09 at 14:05 +0200, Peter Zijlstra wrote:
>> On Thu, 2011-06-09 at 20:37 +0900, KOSAKI Motohiro wrote:
>>> 1) check the pte of the target address is RW attribute.
>>>  2-1) if yes, it has no COW chance. we can safely use gup result.
>>>  2-2) if no, we have to fallback slow vma walk. maybe, it's okey.
>>>       both RO mappings and COW are rare situation. 
>>
>> Not so, clean file pages are RO, even if the vma is writable. We use the
>> fault to track dirty state with.
> 
> Why can't something like
> http://marc.info/?l=linux-kernel&m=130737669810421 work once you restore
> the rw argument?

Yeah, I agree this is best way. Private RO mappings is not sane and no real
world users.

Re: Change in functionality of futex() system call.

[KOSAKI Motohiro]

(2011/06/09 21:05), Peter Zijlstra wrote:
> On Thu, 2011-06-09 at 20:37 +0900, KOSAKI Motohiro wrote:
>> 1) check the pte of the target address is RW attribute.
>>  2-1) if yes, it has no COW chance. we can safely use gup result.
>>  2-2) if no, we have to fallback slow vma walk. maybe, it's okey.
>>       both RO mappings and COW are rare situation. 
> 
> Not so, clean file pages are RO, even if the vma is writable. We use the
> fault to track dirty state with.

Right you are. Sigh. Please forget my previous mail.

Re: Change in functionality of futex() system call.

[Joel Becker]

On Mon, Jun 06, 2011 at 08:11:38PM +0200, Eric Dumazet wrote:
> Le lundi 06 juin 2011 à 10:53 -0700, Darren Hart a écrit :
> > 
> 
> > If I understand the problem correctly, RO private mapping really doesn't
> > make any sense and we should probably explicitly not support it, while
> > special casing the RO shared mapping in support of David's scenario.
> > 
> 
> We supported them in 2.6.18 kernels, apparently. This might sounds
> stupid but who knows ?

	Trying to come up with a strawman for this sort of operation.
What about a process that creates a private mapping and then creates
threads with CLONE_VM?  Would we CoW in that case?

Joel

-- 

	Pitchers and catchers report.

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: Change in functionality of futex() system call.

[Peter Zijlstra]

On Mon, 2011-06-06 at 20:11 +0200, Eric Dumazet wrote:

> Now, what if other software uses a MAP_PRIVATE ro mapping ?
> 
> It was working in previous kernels as well.
> 
> We can say its stupid, but IMHO its not.
> 
> In other words, this program should work, if process never touches
> (writes) into first page.
> 
> This program on previous kernels gave :
> rc=-1 errno=11
> (allowing to wait for a value change and a futex_WAKE)
> 
> With new kernel :
> rc=-1 errno=14  [ no sleep allowed ]

Yeah, so friggin what? I can write a similar program that is broken with
the old behaviour. The fact that the semantics are just crap (wtf does
it mean to FUTEX_WAIT on a address that changes mapping on COW) should
be enough to convince you that the old behaviour simply doesn't make
sense.

Once the COW happened you shouldn't be able to observe the old page, yet
that's exactly what the still pending FUTEX_WAIT will do.

The fact that this can happen, means the behaviour is broken, no if else
or buts about it.

Re: Change in functionality of futex() system call.

[KOSAKI Motohiro]

>> Urgh,. maybe something like the below but with more conditionals that
>> enable the extra logic only for FUTEX_WAIT..
>>
>> The idea is to try a RO gup() when the RW gup() fails so as not to slow
>> down the common path of writable anonymous maps and bail when we used
>> the RO path on anonymous memory.
>>
>> ---
>> diff --git a/kernel/futex.c b/kernel/futex.c
>> index fe28dc2..11f2ad1 100644
>> --- a/kernel/futex.c
>> +++ b/kernel/futex.c
>> @@ -234,7 +234,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key)
>>  	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 +262,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) {
>> +		err = get_user_pages_fast(address, 1, 0, &page);
>> +		ro = 1;
>> +	}
>>  	if (err < 0)
>>  		return err;
>>  
>> @@ -316,6 +320,11 @@ again:
>>  	 * the object not the particular process.
>>  	 */
>>  	if (PageAnon(page_head)) {
>> +		if (ro) {
>> +			err = -EFAULT;
>> +			goto out;
>> +		}
>> +
>>  		key->both.offset |= FUT_OFF_MMSHARED; /* ref taken on mm */
>>  		key->private.mm = mm;
>>  		key->private.address = address;
>> @@ -327,9 +336,10 @@ again:
>>  
>>  	get_futex_key_refs(key);
>>  

Need err=0 here. (note: get_user_pages_fast() return 1) Other than that looks
good to me and this patch passed my test.
	Reviewed-and-tested-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

>> +out:
>>  	unlock_page(page_head);
>>  	put_page(page_head);
>> -	return 0;
>> +	return err;
>>  }
>>  
>>  static inline void put_futex_key(union futex_key *key)
>>
> 
> Hmm, wouldn't that still be susceptible to the zero-page thing if: we
> create a writable private file map of a sparse file, touch a page and
> then remap the thing RO?

After while thinking, I've conclude this is ok. Because 1) as Andrew and
Kyle described, RO mapping usage is not so sane. We need to care it for
only compatibility. 2) David Oliver's case is real compatibility issue.
but I doubt such mprotect() vs futex() race is happen on real world.
3) Anyway, overkill compatibility care might make code slower perhaps.

Off topic: current futex documentations are near terribly unclear and
many futex op are completely undocumented. They are one of root cause
that every change can make compatibility issue. (;_;

Re: Change in functionality of futex() system call.

[Darren Hart]

On 06/10/2011 05:10 AM, KOSAKI Motohiro wrote:

> 
> Off topic: current futex documentations are near terribly unclear and
> many futex op are completely undocumented. They are one of root cause
> that every change can make compatibility issue. (;_;

What documentation are you referring to? The futex man page is a wreck,
and I'm not sure what to do with it since glibc removed the futex()
call. You now have to wrap the syscall manually anyway.

If you are referring to the futex.c file itself, I have been documenting
functions as I modify them. If you found any of those lacking, please
let me know which ones and I'll try to clean them up. If you're
referring to those that remain undocumented, please send a doc patch and
I'll review and help get it upstream. I'd like to see this improved as well.

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

Re: Change in functionality of futex() system call.

[KOSAKI Motohiro]

>> Off topic: current futex documentations are near terribly unclear and
>> many futex op are completely undocumented. They are one of root cause
>> that every change can make compatibility issue. (;_;
> 
> What documentation are you referring to? The futex man page is a wreck,
> and I'm not sure what to do with it since glibc removed the futex()
> call. You now have to wrap the syscall manually anyway.

Honestly, I don't know linux man pages policy at all. example, gettid(2)
also need to be wrap syscall manually. and it's documented and NOTES section
describe "Glibc does not provide a wrapper for this system call; call it
using syscall(2)".

Or, if nobody want to update the doc, shouldn't we just remove futex(2) man
pages? out date docs are often wrong than nothing. I dunno.


> If you are referring to the futex.c file itself, I have been documenting
> functions as I modify them. If you found any of those lacking, please
> let me know which ones and I'll try to clean them up. If you're
> referring to those that remain undocumented, please send a doc patch and
> I'll review and help get it upstream. I'd like to see this improved as well.

No. I think the comments of futex.c are very good, at least, than a lot of mm code. ;)

Re: Change in functionality of futex() system call.

[Darren Hart]

On 06/12/2011 07:11 PM, KOSAKI Motohiro wrote:
>>> Off topic: current futex documentations are near terribly unclear and
>>> many futex op are completely undocumented. They are one of root cause
>>> that every change can make compatibility issue. (;_;
>>
>> What documentation are you referring to? The futex man page is a wreck,
>> and I'm not sure what to do with it since glibc removed the futex()
>> call. You now have to wrap the syscall manually anyway.
> 
> Honestly, I don't know linux man pages policy at all. example, gettid(2)
> also need to be wrap syscall manually. and it's documented and NOTES section
> describe "Glibc does not provide a wrapper for this system call; call it
> using syscall(2)".

Something like that would be good. I don't have any experience pushing
man-page updates. This page seems to document the process:

http://www.kernel.org/doc/man-pages/

> 
> Or, if nobody want to update the doc, shouldn't we just remove futex(2) man
> pages? out date docs are often wrong than nothing. I dunno.

I'd prefer to see it updated as I know of several users of the interface
outside of glibc.

> 
> 
>> If you are referring to the futex.c file itself, I have been documenting
>> functions as I modify them. If you found any of those lacking, please
>> let me know which ones and I'll try to clean them up. If you're
>> referring to those that remain undocumented, please send a doc patch and
>> I'll review and help get it upstream. I'd like to see this improved as well.
> 
> No. I think the comments of futex.c are very good, at least, than a lot of mm code. ;)
> 
> 

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

Re: Change in functionality of futex() system call.

[Shawn Bohrer]

On Fri, Jun 10, 2011 at 09:10:03PM +0900, KOSAKI Motohiro wrote:
> >> Urgh,. maybe something like the below but with more conditionals that
> >> enable the extra logic only for FUTEX_WAIT..
> >>
> >> The idea is to try a RO gup() when the RW gup() fails so as not to slow
> >> down the common path of writable anonymous maps and bail when we used
> >> the RO path on anonymous memory.
> >>
> >> ---
> >> diff --git a/kernel/futex.c b/kernel/futex.c
> >> index fe28dc2..11f2ad1 100644
> >> --- a/kernel/futex.c
> >> +++ b/kernel/futex.c
> >> @@ -234,7 +234,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key)
> >>  	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 +262,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) {
> >> +		err = get_user_pages_fast(address, 1, 0, &page);
> >> +		ro = 1;
> >> +	}
> >>  	if (err < 0)
> >>  		return err;
> >>  
> >> @@ -316,6 +320,11 @@ again:
> >>  	 * the object not the particular process.
> >>  	 */
> >>  	if (PageAnon(page_head)) {
> >> +		if (ro) {
> >> +			err = -EFAULT;
> >> +			goto out;
> >> +		}
> >> +
> >>  		key->both.offset |= FUT_OFF_MMSHARED; /* ref taken on mm */
> >>  		key->private.mm = mm;
> >>  		key->private.address = address;
> >> @@ -327,9 +336,10 @@ again:
> >>  
> >>  	get_futex_key_refs(key);
> >>  
> 
> Need err=0 here. (note: get_user_pages_fast() return 1) Other than that looks
> good to me and this patch passed my test.
> 	Reviewed-and-tested-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> 
> >> +out:
> >>  	unlock_page(page_head);
> >>  	put_page(page_head);
> >> -	return 0;
> >> +	return err;
> >>  }
> >>  
> >>  static inline void put_futex_key(union futex_key *key)
> >>

I've reviewed and tested Peter's change with KOSAKI's addition against
2.6.32.41 and this passes our tests using FUTEX_WAIT with read only
shared mappings.

Reviewed-and-tested-by: Shawn Bohrer <sbohrer@rgmadvisors.com>

--
Shawn


---------------------------------------------------------------
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.

Re: Change in functionality of futex() system call.

[Darren Hart]

On 06/15/2011 11:50 AM, Shawn Bohrer wrote:
> On Fri, Jun 10, 2011 at 09:10:03PM +0900, KOSAKI Motohiro wrote:
>>>> Urgh,. maybe something like the below but with more conditionals that
>>>> enable the extra logic only for FUTEX_WAIT..
>>>>
>>>> The idea is to try a RO gup() when the RW gup() fails so as not to slow
>>>> down the common path of writable anonymous maps and bail when we used
>>>> the RO path on anonymous memory.
>>>>
>>>> ---
>>>> diff --git a/kernel/futex.c b/kernel/futex.c
>>>> index fe28dc2..11f2ad1 100644
>>>> --- a/kernel/futex.c
>>>> +++ b/kernel/futex.c
>>>> @@ -234,7 +234,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key)
>>>>  	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 +262,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) {
>>>> +		err = get_user_pages_fast(address, 1, 0, &page);
>>>> +		ro = 1;
>>>> +	}
>>>>  	if (err < 0)
>>>>  		return err;
>>>>  
>>>> @@ -316,6 +320,11 @@ again:
>>>>  	 * the object not the particular process.
>>>>  	 */
>>>>  	if (PageAnon(page_head)) {
>>>> +		if (ro) {
>>>> +			err = -EFAULT;
>>>> +			goto out;
>>>> +		}
>>>> +
>>>>  		key->both.offset |= FUT_OFF_MMSHARED; /* ref taken on mm */
>>>>  		key->private.mm = mm;
>>>>  		key->private.address = address;
>>>> @@ -327,9 +336,10 @@ again:
>>>>  
>>>>  	get_futex_key_refs(key);
>>>>  
>>
>> Need err=0 here. (note: get_user_pages_fast() return 1) Other than that looks
>> good to me and this patch passed my test.
>> 	Reviewed-and-tested-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>>
>>>> +out:
>>>>  	unlock_page(page_head);
>>>>  	put_page(page_head);
>>>> -	return 0;
>>>> +	return err;
>>>>  }
>>>>  
>>>>  static inline void put_futex_key(union futex_key *key)
>>>>
> 
> I've reviewed and tested Peter's change with KOSAKI's addition against
> 2.6.32.41 and this passes our tests using FUTEX_WAIT with read only
> shared mappings.
> 
> Reviewed-and-tested-by: Shawn Bohrer <sbohrer@rgmadvisors.com>


Would someone care to roll this all together and send a patch with
commit log clearly documenting the issues and which are addressed with
the patch?

Kosaki, do you have updated futextest patches or should I look at the
ones you send previously?

--
Darren


> 
> --
> Shawn
> 
> 
> ---------------------------------------------------------------
> 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

Re: Change in functionality of futex() system call.

[Shawn Bohrer]

On Wed, Jun 15, 2011 at 11:54:46AM -0700, Darren Hart wrote:
> 
> 
> On 06/15/2011 11:50 AM, Shawn Bohrer wrote:
> > On Fri, Jun 10, 2011 at 09:10:03PM +0900, KOSAKI Motohiro wrote:
> >>>> Urgh,. maybe something like the below but with more conditionals that
> >>>> enable the extra logic only for FUTEX_WAIT..
> >>>>
> >>>> The idea is to try a RO gup() when the RW gup() fails so as not to slow
> >>>> down the common path of writable anonymous maps and bail when we used
> >>>> the RO path on anonymous memory.
> >>>>
> >>>> ---
> >>>> diff --git a/kernel/futex.c b/kernel/futex.c
> >>>> index fe28dc2..11f2ad1 100644
> >>>> --- a/kernel/futex.c
> >>>> +++ b/kernel/futex.c
> >>>> @@ -234,7 +234,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key)
> >>>>  	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 +262,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) {
> >>>> +		err = get_user_pages_fast(address, 1, 0, &page);
> >>>> +		ro = 1;
> >>>> +	}
> >>>>  	if (err < 0)
> >>>>  		return err;
> >>>>  
> >>>> @@ -316,6 +320,11 @@ again:
> >>>>  	 * the object not the particular process.
> >>>>  	 */
> >>>>  	if (PageAnon(page_head)) {
> >>>> +		if (ro) {
> >>>> +			err = -EFAULT;
> >>>> +			goto out;
> >>>> +		}
> >>>> +
> >>>>  		key->both.offset |= FUT_OFF_MMSHARED; /* ref taken on mm */
> >>>>  		key->private.mm = mm;
> >>>>  		key->private.address = address;
> >>>> @@ -327,9 +336,10 @@ again:
> >>>>  
> >>>>  	get_futex_key_refs(key);
> >>>>  
> >>
> >> Need err=0 here. (note: get_user_pages_fast() return 1) Other than that looks
> >> good to me and this patch passed my test.
> >> 	Reviewed-and-tested-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> >>
> >>>> +out:
> >>>>  	unlock_page(page_head);
> >>>>  	put_page(page_head);
> >>>> -	return 0;
> >>>> +	return err;
> >>>>  }
> >>>>  
> >>>>  static inline void put_futex_key(union futex_key *key)
> >>>>
> > 
> > I've reviewed and tested Peter's change with KOSAKI's addition against
> > 2.6.32.41 and this passes our tests using FUTEX_WAIT with read only
> > shared mappings.
> > 
> > Reviewed-and-tested-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
> 
> 
> Would someone care to roll this all together and send a patch with
> commit log clearly documenting the issues and which are addressed with
> the patch?

Since it doesn't appear that anyone is jumping on this opportunity I
can do it.

Peter, is it alright if I add your signed off by on the previous
patch?

--
Shawn


---------------------------------------------------------------
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.

Re: Change in functionality of futex() system call.

[KOSAKI Motohiro]

> Kosaki, do you have updated futextest patches or should I look at the
> ones you send previously?

I'm sorry. I missed your mail. I don't have any update. Please look at
privious one.