Re: [PATCH] futex: Fix ZERO_PAGE cause infinite loop

[Peter Zijlstra <peterz@infradead.org>]

Added tglx and dvhart to the CC.

On Thu, 2009-12-24 at 18:29 +0900, KOSAKI Motohiro wrote:
> This patch need both futex and zero-page developer's ack.
> ==========================================================
> 
> commit a13ea5b7 (mm: reinstate ZERO_PAGE) made the unfortunate regression.
> following test program never finish and waste 100% cpu time.
> 
> At the making commit 38d47c1b7 (rely on get_user_pages() for shared
> futexes). There isn't zero page in linux kernel. then, futex developers
> thought gup retry is safe. but we reinstated zero page later...
> 
> This patch fixes it.
> 
> futex-zero.c
> ---------------------------------------------------------------------
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <sys/mman.h>
>  #include <syscall.h>
>  #include <sys/time.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <errno.h>
>  #include <linux/futex.h>
>  #include <pthread.h>
> 
> int main(int argc, char **argv)
> {
>         long page_size;
>         int ret;
>         void *buf;
> 
>         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);
>         }
> 
>         fprintf(stderr, "futex wait\n");
>         ret = syscall( SYS_futex, buf, FUTEX_WAIT, 1, NULL, NULL, NULL);
>         if (ret != 0 && errno != EWOULDBLOCK) {
>                 perror("futex error.\n");
>                 exit(1);
>         }
>         fprintf(stderr, "futex_wait: ret = %d, errno = %d\n", ret, errno);
> 
>         return 0;
> }
> ---------------------------------------------------------------------
> 
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Nick Piggin <npiggin@suse.de>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Ingo Molnar <mingo@elte.hu>
> ---
>  include/linux/mm.h |   16 ++++++++++++++++
>  kernel/futex.c     |    3 +++
>  mm/memory.c        |   14 --------------
>  3 files changed, 19 insertions(+), 14 deletions(-)


> diff --git a/kernel/futex.c b/kernel/futex.c
> index 8e3c3ff..79b89cc 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -254,6 +254,8 @@ again:
>  
>  	page = compound_head(page);
>  	lock_page(page);
> +	if (is_zero_pfn(page_to_pfn(page)))
> +		goto anon_key;
>  	if (!page->mapping) {
>  		unlock_page(page);
>  		put_page(page);
> @@ -268,6 +270,7 @@ again:
>  	 * the object not the particular process.
>  	 */
>  	if (PageAnon(page)) {
> + anon_key:
>  		key->both.offset |= FUT_OFF_MMSHARED; /* ref taken on mm */
>  		key->private.mm = mm;
>  		key->private.address = address;

Doesn't it make more sense to simply fail the futex op?

What I mean is, all (?) futex ops assume userspace actually wrote
something to the address in question to begin with, therefore it can
never be the zero page.

So it being the zero page means someone goofed up, and we should bail,
no?