Re: [PATCH RFC] futex: Fix regression with read only mappings

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

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

Hi

I have no objection. However I'd like to explain why I decided to
prefer to refuse RO private mappings and used prefault.

private mapping semantics is, to write access makes process private pages
(ie PageAnon=1).

So, this patch introduce following another corner case.

Thread-A: call futex(FUTEX_WAIT, memory-region-A).
          get_futex_key() return inode based key.
          sleep on the key
Thread-B: call mprotect(PROT_READ|PROT_WRITE, memory-region-A)
Thread-B: write memory-region-A.
          COW happen. This process's memory-region-A become related
          to new COWed private (ie PageAnon=1) page.
Thread-B: call futex(FUETX_WAKE, memory-region-A).
          get_futex_key() return mm based key.
          IOW, we fail to wake up Thread-A.

It's unclear real world issue or not. But I hope everybody realize it.
So, Probably it would be great if you add some comments for this issue.

2.6.18 had an another trick. It used vma walk for avoiding this issue.
and, unfortunately, it's slow.


> 
>>  Where my tests on 2.6.18 show that this used to wait
>> indefinitely.  Performing a FUTEX_WAIT on a RW private mapping waits
>> indefinitely in 2.6.18, 3.0.0, and with this patch applied.  It is
>> unclear to me if this is a good thing or a bug.
>>
>>  kernel/futex.c |   38 ++++++++++++++++++++++++++------------
>>  1 files changed, 26 insertions(+), 12 deletions(-)