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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4E0BF945.4080102@linux.intel.com>
Date:	Wed, 29 Jun 2011 21:19:17 -0700
From:	Darren Hart <dvhart@...ux.intel.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	Shawn Bohrer <sbohrer@...advisors.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	peterz@...radead.org, eric.dumazet@...il.com,
	david@...advisors.com, linux-kernel@...r.kernel.org,
	zvonler@...advisors.com, hughd@...gle.com, mingo@...e.hu
Subject: Re: [PATCH v4] futex: Fix regression with read only mappings

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/