linux-kernel - Re: [PATCH v2] futex: remove rw parameter from get_futex

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]

Date:	Wed,  6 Jan 2010 11:27:08 +0900 (JST)
From:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To:	Darren Hart <dvhltc@...ibm.com>
Cc:	kosaki.motohiro@...fujitsu.com,
	Hugh Dickins <hugh.dickins@...cali.co.uk>,
	Peter Zijlstra <peterz@...radead.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Nick Piggin <npiggin@...e.de>, Ingo Molnar <mingo@...e.hu>,
	LKML <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ulrich Drepper <drepper@...il.com>,
	"Hansen, Dave" <haveblue@...ibm.com>
Subject: Re: [PATCH v2] futex: remove rw parameter from get_futex_key()

> Hugh Dickins wrote:
> > On Tue, 5 Jan 2010, KOSAKI Motohiro wrote:
> 
> >> From c3e2dfdff84b9b720e646fd6dd3c38fff293c7e6 Mon Sep 17 00:00:00 2001
> >> From: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
> >> Date: Tue, 5 Jan 2010 11:33:00 +0900
> >> Subject: [PATCH] futex: remove rw parameter from get_futex_key()
> >>
> >> Currently, futex have two problem.
> >>
> >> A) current futex doesn't handle private file mappings properly.
> >>
> >> get_futex_key() use PageAnon() to distinguish file and anon. it can
> >> makes following bad scenario.
> >>
> >>   1) thread-A call futex(private-mapping, FUTEX_WAIT). it makes to
> >>      sleep on file mapping object.
> >>   2) thread-B write a variable and it makes cow.
> >>   3) thread-B call futex(private-mapping, FUTEX_WAKE). it wake up
> >>      sleeped thread on the anonymous page. (but it's nothing)
> >>
> 
> Excellent test case, thank you! Would you consider preparing a patch to 
> futextest?
> 
> http://git.kernel.org/?p=linux/kernel/git/dvhart/futextest.git;a=summary

Patch attached. you can feel free any modify such file. thanks.


> I did some experimentation here and found that:
> 
> o The test works if the *_PRIVATE op codes are used.
>    This is because the futex keys are generated using only the virtual
>    address of the page, which doesn't change on a COW.
> o If the waiter writes to the val first, it works.
>    This forces the COW before the waiter generates it's futex key.

True.

> So the waiter's key is generated based on the page cache page address 
> for shared futexes when the value hasn't been written to prior to wait.
> 
> The only scenario where I could think of wanting this behavior is if 
> another process were to try and wake the waiter via the same file backed 
> page. However, as I understand it, the re-use of the same page for 
> unwritten-to private pages is an optimization and can't be relied upon. 
> So this scenario is out.  Another would be to use the futex as a very 
> simple wait queue where the value is never changed. In this case 
> however, the implementation is racy as the value check is effectively 
> negated, so this use case is also out.
> 
> As such, I see no reason not to always use VERIFY_WRITE and force a COW 
> prior to generating the futex_key for shared futexes. It is not 
> necessary for private futexes however as they use only the virtual address.
> 
> I am not sure on whether or not it makes sense to avoid the VERIFY_WRITE 
> on the private futexes. Could be it is just more code for negligible 
> benefit. Thoughts?

I think it's no problem. because,

as performance view:
	access_ok() of almost arch (included x86) ignore VERIFY_WRITE.
	then, this change doesn't cause performance loss of course.
	(current futex mainly handle ro-mapping problem by fault_in_user_writeable)

as consistency view:
	This patch have better consistency without FUTEX_PRIVATE_FLAG case.

as usability view:
	Nobody want to use ro-mappings for private futex. it's obviously meaningless
	and useless.


Download attachment "0001-futextest-Add-two-testcase.patch" of type "application/octet-stream" (7607 bytes)