Re: [PATCH v2] futex: remove rw parameter from get_futex_key()

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

[-- Attachment #1: Type: text/plain, Size: 3026 bytes --]

> 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@jp.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.


[-- Attachment #2: 0001-futextest-Add-two-testcase.patch --]
[-- Type: application/octet-stream, Size: 7607 bytes --]

From 36b04190325209aea5d02ab1d5d5572cd6c8280a Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Date: Wed, 6 Jan 2010 11:07:08 +0900
Subject: [PATCH] futextest: Add two testcase

Recently, we have discussed zero-page and private file mappings bug.
thus such test case will be added.

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 functional/Makefile                         |    4 +-
 functional/futex_wait_private_mapped_file.c |  112 +++++++++++++++++++++++++++
 functional/futex_wait_uninitialized_heap.c  |   85 ++++++++++++++++++++
 functional/run.sh                           |    7 ++
 4 files changed, 207 insertions(+), 1 deletions(-)
 create mode 100644 functional/futex_wait_private_mapped_file.c
 create mode 100644 functional/futex_wait_uninitialized_heap.c

diff --git a/functional/Makefile b/functional/Makefile
index 69356fa..6ecb42c 100644
--- a/functional/Makefile
+++ b/functional/Makefile
@@ -8,7 +8,9 @@ TARGETS := \
 	futex_wait_wouldblock \
 	futex_requeue_pi \
 	futex_requeue_pi_signal_restart \
-	futex_requeue_pi_mismatched_ops
+	futex_requeue_pi_mismatched_ops \
+	futex_wait_uninitialized_heap \
+	futex_wait_private_mapped_file
 
 .PHONY: all clean
 all: $(TARGETS)
diff --git a/functional/futex_wait_private_mapped_file.c b/functional/futex_wait_private_mapped_file.c
new file mode 100644
index 0000000..fb6467a
--- /dev/null
+++ b/functional/futex_wait_private_mapped_file.c
@@ -0,0 +1,112 @@
+/******************************************************************************
+ *
+ * Copyright FUJITSU LIMITED 2010
+ * Copyright KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
+ *
+ *   This program is free software;  you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
+ *   the GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program;  if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * NAME
+ *      futex_wait_private_mapped_file.c
+ *
+ * DESCRIPTION
+ *	Internally, Futex has two handling mode, anon and file. The private file
+ *	mapping is special. At first it behave as file, but after write anything
+ *	it behave as anon. This test is intent to test such case.
+ *
+ * AUTHOR
+ *      KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
+ *
+ * HISTORY
+ *      2010-Jan-6: Initial version by KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
+ *
+ *****************************************************************************/
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <syscall.h>
+#include <unistd.h>
+#include <errno.h>
+#include <linux/futex.h>
+#include <pthread.h>
+#include <libgen.h>
+
+#include "logging.h"
+#include "futextest.h"
+
+#define PAGE_SZ 4096
+
+char pad[PAGE_SZ] = {1};
+futex_t val = 1;
+char pad2[PAGE_SZ] = {1};
+
+void* thr_futex_wait(void *arg)
+{
+	int ret;
+
+	info("futex wait\n");
+	ret = futex_wait(&val, 1, NULL, 0);
+	if (ret != 0 && errno != EWOULDBLOCK) {
+		perror("futex error.\n");
+		print_result(RET_ERROR);
+		exit(RET_ERROR);
+	}
+	info("futex_wait: ret = %d, errno = %d\n", ret, errno);
+
+	return NULL;
+}
+
+int main(int argc, char **argv)
+{
+	pthread_t thr;
+	int ret = RET_PASS;
+	int res;
+	int c;
+
+	while ((c = getopt(argc, argv, "c")) != -1) {
+		switch(c) {
+		case 'c':
+			log_color(1);
+			break;
+		default:
+			exit(1);
+		}
+	}
+
+	printf("%s: Test the futex value of private file mappings in FUTEX_WAIT\n",
+	       basename(argv[0]));
+
+	ret = pthread_create(&thr, NULL, thr_futex_wait, NULL);
+	if (ret < 0) {
+		fprintf(stderr, "pthread_create error\n");
+		goto error;
+	}
+
+	info("wait a while");
+	sleep(3);
+	val = 2;
+	res = futex_wake(&val, 1, 0);
+	info("futex_wake %d", res);
+
+	info("join");
+	pthread_join(thr, NULL);
+
+	print_result(ret);
+	return ret;
+
+error:
+	ret = RET_ERROR;
+	print_result(ret);
+	return ret;
+}
diff --git a/functional/futex_wait_uninitialized_heap.c b/functional/futex_wait_uninitialized_heap.c
new file mode 100644
index 0000000..cd7e5f6
--- /dev/null
+++ b/functional/futex_wait_uninitialized_heap.c
@@ -0,0 +1,85 @@
+/******************************************************************************
+ *
+ * Copyright FUJITSU LIMITED 2010
+ * Copyright KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
+ *
+ *   This program is free software;  you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
+ *   the GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program;  if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * NAME
+ *      futex_wait_uninitialized_heap.c
+ *
+ * DESCRIPTION
+ *      Wait on uninitialized heap. It shold be zero and FUTEX_WAIT should return
+ *      immediately. This test is intent to test zero page handling in futex.
+ *
+ * AUTHOR
+ *      KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
+ *
+ * HISTORY
+ *      2010-Jan-6: Initial version by KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
+ *
+ *****************************************************************************/
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <syscall.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <errno.h>
+#include <linux/futex.h>
+#include <libgen.h>
+
+#include "logging.h"
+#include "futextest.h"
+
+int main(int argc, char **argv)
+{
+	long page_size;
+	int res;
+	void *buf;
+	int ret = RET_PASS;
+	int c;
+
+	while ((c = getopt(argc, argv, "c")) != -1) {
+		switch(c) {
+		case 'c':
+			log_color(1);
+			break;
+		default:
+			exit(1);
+		}
+	}
+
+	page_size = sysconf(_SC_PAGESIZE);
+
+	buf = mmap(NULL, page_size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
+	if (buf == (void *)-1) {
+		perror("mmap error.\n");
+		exit(1);
+	}
+
+	printf("%s: Test the uninitialized futex value in FUTEX_WAIT\n",
+	       basename(argv[0]));
+
+	res = futex_wait(buf, 1, NULL, 0);
+	if (res != 0 && errno != EWOULDBLOCK) {
+		ret = RET_FAIL;
+	}
+	print_result(ret);
+
+	return ret;
+}
diff --git a/functional/run.sh b/functional/run.sh
index 959b389..411c20f 100755
--- a/functional/run.sh
+++ b/functional/run.sh
@@ -29,6 +29,8 @@
 #
 # HISTORY
 #      2009-Nov-9: Initial version by Darren Hart <dvhltc@us.ibm.com>
+#      2010-Jan-6: Add futex_wait_uninitialized_heap and futex_wait_private_mapped_file 
+#                  by KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
 #
 ###############################################################################
 
@@ -83,3 +85,8 @@ echo
 
 echo
 ./futex_wait_wouldblock $COLOR
+
+echo
+./futex_wait_uninitialized_heap
+./futex_wait_private_mapped_file
+
-- 
1.6.5.2