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

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: <4DF0B072.8060204@jp.fujitsu.com>
Date:	Thu, 09 Jun 2011 20:37:22 +0900
From:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To:	dvhart@...ux.intel.com
CC:	david@...advisors.com, kyle@...fetthome.net, luto@....edu,
	eric.dumazet@...il.com, peterz@...radead.org,
	linux-kernel@...r.kernel.org, sbohrer@...advisors.com,
	zvonler@...advisors.com, hughd@...gle.com, tglx@...utronix.de,
	mingo@...e.hu
Subject: Re: Change in functionality of futex() system call.

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.




View attachment "0001-FUTEX_WAIT-read-only.patch" of type "text/plain" (4971 bytes)

View attachment "0001-Revert-futexes-Remove-rw-parameter-from-get_futex_ke.patch" of type "text/plain" (4460 bytes)

View attachment "0002-make-slowpath.patch" of type "text/plain" (2676 bytes)

View attachment "performance.txt" of type "text/plain" (4313 bytes)