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

[Darren Hart <dvhart@linux.intel.com>]

On 06/29/2011 04:38 PM, Thomas Gleixner wrote:
> On Wed, 29 Jun 2011, Shawn Bohrer wrote:
>>
>> While fixing the regression this patch opens up a possible bad
>> scenarios as identified by KOSAKI Motohiro:
>>
>> This patch also allows FUTEX_WAIT on RO private mappings which have
>> the following corner case.
> 
> These two sentences make no sense at all. We really need a very
> accurate description of this change. That code is subtle and we really
> want to have a very clear and understandable changelog.
> 
> Your changelog fails the basic test by mentioning "corner case" simply
> because the whole futex code consists only of corner cases.
> 
> Thanks,
> 
> 	tglx

Yeah, those messages are quotes from Kosaki, but that isn't apparent without
having all the context. They are confusing. The language needs to be cleaned up
a bit as well.

Shawn, I apologize for this as it was my idea to begin with, but after rereading
all of the previous patches from Kosaki, I realized that the rw parameter was
part of the original design (and not newly introduced by your patch) and that
cramming the FLAGS_RO flag into the flags variables muddies the meaning of
flags. flags is meant to modify a particular futex call and ensure that call is
correctly restarted after a signal. The RO/RW bit pertains to the calling
function and does not vary from call to call. We should revert back to the rw
parameter using VERIFY_READ and VERIFY_WRITE.

How about this for a header (I'll leave it to Shawn to
incorporate, adjust, and resend):

commit 7485d0d3758e8e6491a5c9468114e74dc050785d (futexes: Remove rw parameter
from get_futex_key()) in 2.6.33 introduced a user-mode regression in that it
broke futex operations on read-only memory maps in addition to preventing a loop
when encountering a ZERO_PAGE.  For example, this breaks workloads that have one
or more reader processes doing a FUTEX_WAIT on a futex within a read only shared
file mapping, and a writer processes that has a writable mapping issuing the
FUTEX_WAKE.

This fixes the regression for valid futex operations on RO mappings by trying a
RO get_user_pages_fast() when the RW get_user_pages_fast() fails. This change
makes it necessary to also check for invalid use cases, such as anonymous RO
mappings (which can never change) and the ZERO_PAGE which the commit referenced
above was written to address.

This patch does restore the original behavior with RO private mappings which
suffer from the following corner case.

  Thread-A: call futex(FUTEX_WAIT, memory-region-A).
            get_futex_key() returns an inode based key.
            sleep on the key
  Thread-B: call mprotect(PROT_READ|PROT_WRITE, memory-region-A)
  Thread-B: write memory-region-A.
            This process's memory-region-A gets remapped to a
            COWed PageAnon=1 page.
  Thread-B: call futex(FUETX_WAKE, memory-region-A).
            get_futex_key() returns an mm based key.
            Thread-A is never woken as it is waiting on a different key.

Checking for a private mapping requires walking the vmas and was deemed too
costly to avoid a userspace hang in a nonsensical use case.

This Patch is based on Peter Zijlstra's initial patch with modifications to only
allow RO mappings for futex operations that need VERIFY_READ access.


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